Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

function implicit conversion the same way as fun x #17487

Merged
merged 22 commits into from
Aug 10, 2024

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Aug 4, 2024

Description

Fixes #7401

Checklist

  • Test cases added
  • Release notes entry updated

Copy link
Contributor

github-actions bot commented Aug 4, 2024

❗ 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 force-pushed the fix-function-implicit-convertion branch from 0b1e680 to 725e8cb Compare August 4, 2024 17:32
@edgarfgp edgarfgp force-pushed the fix-function-implicit-convertion branch from ec5e55b to bfcd2d8 Compare August 4, 2024 19:51
@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
edgarfgp and others added 2 commits August 5, 2024 09:02
Copy link
Member

@psfinaki psfinaki left a comment

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
Copy link
Member

psfinaki commented Aug 5, 2024

/azp run

@edgarfgp edgarfgp requested review from abonie and psfinaki August 6, 2024 18:37
@brianrourkeboll
Copy link
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
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
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
Copy link
Contributor Author

edgarfgp commented Aug 9, 2024

@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