[WIP] Use cached delegate instances for method group conversions

[pawchen]

See #5835

This changes C# but not yet VB, vb files are changed to allow compile.

[tmat]

Requires thorough EnC, EE and scripting testing and review.

[pawchen]

Thanks for the comments @tmat, I'll fix the generated names.

I do aware this needs a lot of testing, but I wanted to submit this early so I can get feedback and see if it get strong objection about how the caching is done. So far I don't see any opinions about that, so I assume it's worth continuing and will add more tests and get back.

[tmat]

@DiryBoy Sounds good. I'd wait for compiler folks @dotnet/roslyn-compiler to review first before adding more tests to avoid throwaway work.

[pawchen]

@tmat Thanks, I'll wait then.

[AlekseyTs]

Type casts cannot be used to convert symbols to CCI interfaces, symbols should be translated. Our symbol types claim to implement many different CCI interfaces for efficiency, but it doesn't mean the implementation is correct for all situations, therefore, there is a translation layer that decides if it is appropriate for a certain symbol to implement some interface directly or requires a wrapper, etc.

[pawchen]

Thanks for the info, I just copied that method from above, line 383~391

[gafter]

TypeParameterUsageChecker [](start = 19, length = 25)

See TypeSymbolExtensions.ContainsTypeParameter to see how this could be done in one line.

[marksmeltzer]

I am still waiting with baited breath for this to get merged.

In the meantime, I think this particular issue could be solved by a Rosslyn code analyzer that provides a code hint automated conversion to the already statically cachable lamda form.

[AlekseyTs]

Here is a brief summary of what we have in this PR at the moment.

Delegate instances produced by a method group conversion resolved to a static method (reduced extension methods are excluded) are cached. Delegate instances produced by an object creation expression are not cached. Caching is not performed within static constructors.

A delegate instance is cached on a module level when all the following conditions are met:

• The delegate type is accessible on a module level, and
• Neither the delegate type, nor the target method are closed over type parameters of the enclosing methods and types.

Otherwise, the delegate instance is cached on the immediate enclosing type level when
neither the delegate type, nor the target method are closed over type parameters of the enclosing methods.

Otherwise, the delegate instance is cached on the enclosing method level.

Tracked TODOs:

• Caching of delegates targeting static local functions.
• At the moment caching is not performed inside local functions (It appears that the implementation doesn’t catch the case of caching inside a lambda inside a local function. The check is performed only for the immediate enclosing method).

Caching on the module level is performed as follows:

• For each distinct delegate type (different constructions are distinct), a non-generic static type is synthesized in the global namespace.
• For each distinct target method a field to store the cached delegate instance is added to the type.

Caching on the type level is performed as follows:

• A nested non-generic static type is synthesized.
• For each distinct combination of a delegate type and a target method a field to store the cached delegate instance is added to the type.

Caching on the enclosing method level is performed as follows:

• A nested generic static type is synthesized within the enclosing type.
• The arity matches the arity of the top-most enclosing method (note that local functions are not supported yet). Type’s type parameters correspond to type parameters of the top-most enclosing method.
• For each distinct combination of a delegate type and a target method a field to store the cached delegate instance is added to the type.

We should consider the following changes to the design/implementation:

• Should we disable caching in module initializers?
• Switch to a single synthesized type per module rather than one per delegate type?
• We already have support for caching dynamic call-sites in fields of synthesized nested types (see DynamicSiteContainer), it handles caching within local functions as well. We should investigate if it is possible to reuse/share that infrastructure.

[pawchen]

Thank you @AlekseyTs for the summary and pointers.

Question, can we block the compiled binary from loading in the .Net Framework 4.8 and older, or, do we have this kind of mechanism today? If so I think we can continue, as C# spec was changed to allow caching (saw it mentioned from some comments in the GitHub, need to confirm.) EDIT: See #5835 (comment)

Also noticed local functions can be declared static in the code ensuring no captures in a recent C# version. I think there will be different behaviors from what I observed at the time from the old codebase, and need to re-check.

The Roslyn code is evolving fast, a lot of changes in the repo since my last submission. There's no doubt I need some time to go thru the related areas again and rebase the commits properly if to continue on this. So need to be cautious about the blockers ahead.

[pawchen]

The document https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version says,

C# 9 is supported only on .NET 5 and newer versions.
C# 10 is supported only on .NET 6 and newer versions.

Can we just enable the caching for C# 9/10 or above?

[RikkiGibson]

Can we just enable the caching for C# 9/10 or above?

This might be ok, but I'd be curious to know what motivates doing this versus just doing the caching the same way for all language versions?

[pawchen]

Can we just enable the caching for C# 9/10 or above?

This might be ok, but I'd be curious to know what motivates doing this versus just doing the caching the same way for all language versions?

The main reason is to avoid targeting the old .Net Framework 4.8 or lower as security concerns discussed previously. Another reason is that, I guess, the old spec did not permit caching so that's why the linked issue #5835 said it's being modified to permit this.

[jaredpar]

Can we just enable the caching for C# 9/10 or above?

Yes we can do that. In fact I think this is likely what we should be doing but for C# 11 and above.

The reasoning is simply compatibility. This will break compatibility for anyone who depended on method group conversions to return new instances. The number of customers that did this is likely to be small but it's also almost certainly greater than zero.

When we were first considering this change we hadn't really leaned into using /langversion as a tool to help with compat sensitive changes. Lately though we've started doing this with record parsing, lambda inference changes, il verification, etc ... So long as there is a simple way to tie a compat change to /langversion then I'd lean towards doing so.

There's no doubt I need some time to go thru the related areas again and rebase the commits properly if to continue on this. So need to be cautious about the blockers ahead.

I want to also say that we are very willing to help with finishing the change. We are very thankful for all the work you did to get this change moving but we understand we paused this change for a long time. We were uncertain if you would want to pick it back up again or not. We are happy to continue helping review your work here or take it over and finish it up (or something in between). Any of these work for us.

Again though I do want to thank you for the work you've done thus far. We really appreciate it!

[pawchen]

Thanks @jaredpar for the big Yes, and so yes I'm excited that we are able to pick it back up again! My plan is to use the weekends to do the major things while checking for review feedbacks regularly during weekdays.

We are happy to continue helping review your work here or take it over and finish it up (or something in between).

I'm happy you are willing to take it over, before that, I do want to make this PR good for review again, at least, then we can decide the directions afterwards, depending on the issues/feedbacks and how fast I could catch up with you guys.

[AlekseyTs]

I do want to make this PR good for review again

@pawchen I suggest leaving this PR and the branch as is to make it easier to refer to the existing comments, if need to. And doing the work to "make this PR good for review again" in a different branch (I assume that will involve rebasing on top of current main and squashing commits), then submitting a new PR.

[pawchen]

@AlekseyTs Good idea, thank you for the suggestions. I'll submit a new PR.