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

Method group conversion caching redo #57882

Closed
wants to merge 47 commits into from
Closed

Conversation

pawchen
Copy link
Contributor

@pawchen pawchen commented Nov 19, 2021

This is to help picking up #6642

@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 19, 2021
@AlekseyTs
Copy link
Contributor

@pawchen Ping me once this PR is ready for a look.

@pawchen
Copy link
Contributor Author

pawchen commented Dec 8, 2021

@AlekseyTs Thanks, it's near. This time I dropped module scoped caching, reducing the complexities by not dealing with the module builders. I run into a few issues here though:

  1. ExpressionCompiler tests are not run in CI (ExpressionEvaluator.*.UnitTests are not run in CI #58030) and they are already failing without my changes, what's worse, the failing tests involve delegates
  2. Have some trouble specifying the C# version in ExpressionCompiler tests, despite I created the Compilation with preview version of C#, the C# version was set back to 10.

TODOs

  • Recently anonymous delegate was introduced, not sure how to dig out the type parameters in it, need to experiment a little
  • Local functions?

I'll close this PR and create another one with cleaner commit history once those issues are clear.

@pawchen
Copy link
Contributor Author

pawchen commented Dec 8, 2021

OK, because synthesized anonymous delegates are synthesized according to the target method, despite we cannot dig the type parameters out of the delegate symbol, if the target method contains type parameters, we can still detect them there.

@pawchen pawchen closed this Dec 12, 2021
@pawchen pawchen deleted the mgcMerge branch January 13, 2022 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants