Skip to content

function implicit conversion the same way as fun x#17487

Merged
vzarytovskii merged 22 commits into
dotnet:mainfrom
edgarfgp:fix-function-implicit-convertion
Aug 10, 2024
Merged

function implicit conversion the same way as fun x#17487
vzarytovskii merged 22 commits into
dotnet:mainfrom
edgarfgp:fix-function-implicit-convertion

Conversation

@edgarfgp

@edgarfgp edgarfgp commented Aug 4, 2024

Copy link
Copy Markdown
Contributor

Description

Fixes #7401

Checklist

  • Test cases added
  • Release notes entry updated

@github-actions

github-actions Bot commented Aug 4, 2024

Copy link
Copy Markdown
Contributor

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.100.md

@edgarfgp edgarfgp marked this pull request as ready for review August 4, 2024 20:02
@edgarfgp edgarfgp requested a review from a team as a code owner August 4, 2024 20:02
Comment thread src/Compiler/Checking/Expressions/CheckExpressions.fs Outdated
edgarfgp and others added 2 commits August 5, 2024 09:02

@psfinaki psfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you Edgar!

@psfinaki

psfinaki commented Aug 5, 2024

Copy link
Copy Markdown
Contributor

/azp run

@edgarfgp edgarfgp requested review from abonie and psfinaki August 6, 2024 18:37
@brianrourkeboll

Copy link
Copy Markdown
Contributor

Two comments on the approach used in 581be73:

  1. I think that it follows the spirit of the existing code in that it simply looks at available syntactic information.
  2. If we are worried about additional heap allocations (tuples, continuations), we can likely reduce them, perhaps at the expense of some code complexity.

@vzarytovskii

Copy link
Copy Markdown
Member

Two comments on the approach used in 581be73:

  1. I think that it follows the spirit of the existing code in that it simply looks at available syntactic information.

  2. If we are worried about additional heap allocations (tuples, continuations), we can likely reduce them, perhaps at the expense of some code complexity.

Can we measure the diff between old and new allocation-wise in long-running scenarios?

One-off compilation won't be a concern, continuous allocations in tooling might be. Service currently is not exactly known to be memory-friendly, probably don't want make it worse (I doubt it will be with the change, since nothing will likely escape gen0, but just want a sanity check).

@brianrourkeboll

Copy link
Copy Markdown
Contributor

@vzarytovskii

  1. If we use struct tuples, then I think there should be no additional allocations in the common case (method calls taking anything other than lambdas, address-of, or quotes).
  2. Even for method calls taking lambdas, the additional allocations should effectively be equal to the number of curried arguments in the lambda sequence, i.e., one continuation closure each, as in cont = new loopExpr@24-2<a, b>(cont);.

@edgarfgp edgarfgp requested a review from vzarytovskii August 7, 2024 15:17
@edgarfgp

edgarfgp commented Aug 9, 2024

Copy link
Copy Markdown
Contributor Author

@vzarytovskii

  1. If we use struct tuples, then I think there should be no additional allocations in the common case (method calls taking anything other than lambdas, address-of, or quotes).
  2. Even for method calls taking lambdas, the additional allocations should effectively be equal to the number of curried arguments in the lambda sequence, i.e., one continuation closure each, as in cont = new loopExpr@24-2<a, b>(cont);.

@vzarytovskii Does Brian's changes cover your expectations ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

"function" cannot be implicitly converted the same way "fun x" does.

5 participants