Do not create a cache field for lambda if it depends on caller's type argument #44939

MykolaBalakin · 2020-06-08T14:55:31Z

… argument Fix for dotnet#44720

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenClosureLambdaTests.cs

AlekseyTs · 2020-06-08T16:59:11Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenClosureLambdaTests.cs

+ {
+ var message = string.Empty;
+
+ for (int i = 0; i < 1; i++)


for (int i = 0; i < 1; i++) [](start = 8, length = 27)

Is there an affected scenario that doesn't involve a loop? #Closed

According to the condition (shouldCacheForStaticMethod || shouldCacheInLoop) there should be a scenario involving a static method instead of a loop. Haven't tried though.

No, it seems to be reproducable only using a loop. Roslyn does not cache the delegate for static methods if the method is generic (wondering if that can be changed with the check I've added).

No, it seems to be reproducable only using a loop. Roslyn does not cache the delegate for static methods if the method is generic (wondering if that can be changed with the check I've added).

Is it possible that shouldCacheForStaticMethod is always set correctly and only shouldCacheInLoop is incorrect and we should adjust the place where it is calculated to do the same thing as we do while calculating shouldCacheForStaticMethod?

In reply to: 437690642 [](ancestors = 437690642)

I've dug into the logic behind the second part of the condition (caching into a local instead of a field), and it seams to be a dead code.

F.SynthesizedLocal is always invoked with syntax = null and kind = CachedAnonymousMethodDelegate (code) and that leads to failed debug assertion in SynthesizedLocal..ctor (code) or NullReferenceException in LambdaUtilities.GetDeclaratorPosition (code).

Considering that, the field is the only possible option to do caching, I can remove the dead code and move my check outside the if statement.

Would appreciate if you confirm if that code is a dead code.

Would appreciate if you confirm if that code is a dead code.

At the moment, we are really heads down trying to complete the remaining feature work. One way to confirm that your assumption is incorrect is to make a change to throw when the dead code is reached, run all tests and see if anything breaks. If nothing breaks, there is still a chance that there is a test gap. #Closed

Have already done that. None of the tests executes that branch. Haven't tried dogfooding the compiler into itself yet though.

I've tried to come up with a test case to cover that code. That's how I concluded that the code just cannot be executed successfully at all. 🙂

Have already done that. None of the tests executes that branch. Haven't tried dogfooding the compiler into itself yet though.

I've tried to come up with a test case to cover that code. That's how I concluded that the code just cannot be executed successfully at all.

I suggest opening an issue about that and capturing results of your investigation there. I don't think we should do anything about that in context of this PR because it doesn't appear to be related to the issue we are trying to address.

In reply to: 445836446 [](ancestors = 445836446)

AlekseyTs · 2020-06-11T17:31:19Z

        if (shouldCacheForStaticMethod || shouldCacheInLoop)

I would expect us to figure out if and what kind of caching should be performed by this point and the locals (shouldCacheForStaticMethod, shouldCacheInLoop) already reflecting that. #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs:1587 in 8801674. [](commit_id = 8801674, deletion_comment = False)

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

AlekseyTs · 2020-06-12T00:18:33Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenClosureLambdaTests.cs

+
+ for (int i = 0; i < 1; i++)
+ {
+ void LocalMethod<TLocal>(TLocal value)


void LocalMethod(TLocal value)

I think we should test more permutations and whether caching is used for them:

both the lambda and the loop are inside a local function

This local function is not generic and is inside another local function, the used type parameter belongs to the outer local function rather than to this one,

there is no local function, the delegate refers to the type parameter of the enclosing method

the type parameter of the local function is not used in the delegate type

etc.

Obviously, if you plan to stick with new HasTypeArgumentsFromReferencedMethod method, we would want to test more delegate types to cover full breadth of different type symbol kinds.

#Closed

AlekseyTs · 2020-06-12T00:19:21Z

Done with review pass (iteration 4) #Closed

agocke · 2020-06-12T19:58:00Z

I'm not sure this is the right fix. This change tries to avoid capturing the cache field in the case that we're capturing a type parameter from an outside scope (which would appear in the type of the cache field and thus be invalid).

Alternatively, we could track captured types as well as captured variables, and if we find one, we can introduce a closure environment that is parameterized on the captured type, which would allow us to successfully create a caching field of the captured type.

I think the second one is probably better.

AlekseyTs · 2020-06-12T23:30:01Z

@agocke

I'm not sure this is the right fix.

Are you saying the fix is incorrect, i.e. is going to cause incorrect behavior or a crash at runtime? Otherwise it looks like a viable alternative to the current behavior - an invalid code is emitted, the program cannot be executed. Perhaps the code will be not as optimal as it could potentially be, but at least it will not crash. We could file a follow up issue to optimize the rewrite later.

…aring a custom one

AlekseyTs · 2020-06-30T15:40:15Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

+ // and the parameter is not captured by the container.
+ if (TryGetGenericMethodTypeMap(referencedMethod.ConstructedFrom, out var typeMap))
+ {
+ cacheVariableType = typeMap.SubstituteType(cacheVariableType).Type;


cacheVariableType = typeMap.SubstituteType(cacheVariableType).Type;

Why is this needed? it doesn't look like we did this before. #Closed

After closer investigation, I am convinced that we are approaching this in a somewhat wrong way. After debugging the test, I realized that referencedMethod is the lambda that is being converted to the delegate, it doesn't really provide a context for the delegate type, the context comes from an enclosing method and enclosing types. In terms of the comment below: "We cannot reference type parameters from the parent method outside the method", the referencedMethod is not a parent method.
Even though the check below appears to provide expected value in our scenarios, this is not the question we should be asking. The better (and simpler) question to ask is whether the cacheVariableType as calculated above refers to any method type parameters (I mean from any method) after the substitution. If that is the case, we cannot use it for a field in any type. I don't think we have a ready to use helper to perform a check like that, but it should be pretty straight forward to add one next to ContainsTypeParameter used below following the same implementation strategy.

In reply to: 447782927 [](ancestors = 447782927)

The better (and simpler) question to ask is whether the cacheVariableType as calculated above refers to any method type parameters (I mean from any method) after the substitution.

Consider the following code. In this case, the lambda refers to another method type parameter, but there is nothing wrong to cache the delegate because the type parameter has a larger scope than the cache.

void GenericMethod<T>(T value) { for (int i = 0; i < 1; i++) { DoStuff<T>(() => value); } } void DoStuff<T>(Func<T> lambda) { Console.WriteLine(lambda()); }

I'm totally ok to implement the fix in any way you think is the best. But I'm worried about this kind of fix because the fix you offer eliminates the cache in that scenario hence may slow down some code that already works fine.

Consider the following code. In this case, the lambda refers to another method type parameter, but there is nothing wrong to cache the delegate because the type parameter has a larger scope than the cache.

The concept of a "larger scope" for a type parameter probably needs a clear definition. And I think the suggested fix can esily be adjusted to ignore type parameters like that. However, if we are talking about method type parameters that are becoming type type parameters, then, perhaps, the "after the substitution" is the important part in the suggestion. Is it possible that "after the substitution" they become type type parameters and, therefore, are no longer treated as "bad" by the suggested fix? It has been a long time since I looked at this PR, so it is quite possible I am missing something.

I've implemented the fix you offered. Please, take a look.

What about the case I mentioned, your fix handles it perfectly fine. The lambda is already "moved" into a display class, so from the perspective of the fix the type argument of GenericMethod is actually a type argument of the display class.
I've added a unit test to verify that such a case works fine after the fix.

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

AlekseyTs · 2020-06-30T20:57:11Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

@@ -1630,6 +1646,30 @@ private BoundNode RewriteLambdaConversion(BoundLambda node)
 }

 return result;
+
+ static bool TryGetGenericMethodTypeMap(MethodSymbol method, out TypeMap typeMap)


TryGetGenericMethodTypeMap

Based on my suggestion above, I think this method is not needed. #Closed

AlekseyTs · 2020-06-30T21:41:01Z

Done with review pass (iteration 6)

In reply to: 652060979

RikkiGibson · 2021-05-26T18:18:10Z

Hi @nbalakin, is anything we can do to help with this PR? We got a duplicate report of the same bug referenced in this PR's description, so it seems people are still hitting the issue from time to time.

MykolaBalakin · 2021-05-27T14:12:07Z

Hi @RikkiGibson. Yep, I can finish the fix.
Could you confirm if I understand your review processes well. All the review remarks that has "#Closed" in the first message are considered to be resolve by the remark author, aren't they? So the only unfixed remarks are those three by AlekseyTs:

Do not create a cache field for lambda if it depends on caller's type argument #44939 (comment)
Do not create a cache field for lambda if it depends on caller's type argument #44939 (comment)
Do not create a cache field for lambda if it depends on caller's type argument #44939 (comment)

Can I mark all the others as resolved in GitHub UI just to make the PR cleaner?

333fred · 2021-05-27T14:59:47Z

Can I mark them as the other as resolved in GitHub UI just to make the PR cleaner?

Yes, that's fine. CodeFlow, while a cleaner review UI than github's native tools, does leave things like that to be desired.

AlekseyTs · 2021-05-28T00:23:49Z

Could you confirm if I understand your review processes well. All the review remarks that has "#Closed" in the first message are considered to be resolve by the remark author, aren't they?

Can I mark all the others as resolved in GitHub UI just to make the PR cleaner?

Usually we prefer that only those who made a comment close it (this simplifies the book keeping and makes it easier to make sure that the concerns are indeed addressed). However, I believe there is a way to mark a comment as Resolved (rather than Closed) by adding "#Resolved" in the same place.

In reply to: 850027220

…c-lambda

…parameter

AlekseyTs · 2021-06-09T14:32:31Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs

@@ -1031,6 +1031,15 @@ public static bool ContainsTypeParameters(this TypeSymbol type, HashSet<TypePara
 private static readonly Func<TypeSymbol, HashSet<TypeParameterSymbol>, bool, bool> s_containsTypeParametersPredicate =
 (type, parameters, unused) => type.TypeKind == TypeKind.TypeParameter && parameters.Contains((TypeParameterSymbol)type);

+ public static bool ContainsTypeParameterFromSymbol(this TypeSymbol type, Func<Symbol, bool> containingSymbolFilter)


ContainsTypeParameterFromSymbol

It looks like there is only one usage of this helper. Generalization of this helper with a filter delegate doesn't feel necessary. I think a specialized helper targeting specific check we need will be prefered, something called "ContainsMethodTypeParameter" and the condition could be as simple as (type as TypeParameterSymbol)?.TypeParameterKind == TypeParameterKind.Method

I've made the method more specialised but I've left my implementation of the check (ContainingSymbol is MethodSymbol instead of TypeParameterKind == Method)

Not sure about correctness of my check if my check in terms of correct usage of code model, but the current implementation handles the case we discussed earlier. Consider the following:

Original code:

void GenericMethod<T>(T value) { for (int i = 0; i < 1; i++) { DoStuff<T>(() => value); } } void DoStuff<T>(Func<T> lambda) { }

Generated code:

private void GenericMethod<T>(T value) { DisplayClass<T> displayClass = new DisplayClass<T>(); displayClass.value = value; for (int i = 0; i < 1; i++) { DoStuff(new Func(displayClass.Lambda)); } }

When the compiler chooses if to cache the delegate, the type parameter symbol has TypeParameterKind == Method (as it is in the original parameter symbol) but the container is an instance of SynthesizedClosureEnvironment as the parameter is moved to the synthesised display class.

BTW, should I re-request a review via GitHub button when I think I fixed all the remarks? Or do you monitor all the updates made somehow else?

should I re-request a review via GitHub button when I think I fixed all the remarks? Or do you monitor all the updates made somehow else?

There is no requirement to do this, however you may re-request a review when you feel appropriate. We don't use any special system to track active PRs other than what GitHub provides. So, if there are many active PRs, a PR might fall of the radar. When this happens, it is recommended to bring our attention to the PR.

I've left my implementation of the check (ContainingSymbol is MethodSymbol instead of TypeParameterKind == Method)

That is fine, both forms should be equivalent.

When the compiler chooses if to cache the delegate, the type parameter symbol has TypeParameterKind == Method (as it is in the original parameter symbol) but the container is an instance of SynthesizedClosureEnvironment as the parameter is moved to the synthesised display class.

It sounds like you are saying that you have run into a case when a type type parameter has TypeParameterKind == TypeParameterKind.Method. If that is the case, this is a bug.

It sounds like you are saying that you have run into a case when a type type parameter has TypeParameterKind == TypeParameterKind.Method. If that is the case, this is a bug.

Yep, that is the case I have run into.
It happens because the original type parameter is wrapped into SynthesizedSubstitutedTypeParameterSymbol (code) that passes some properties through to the original type parameter symbol (e.g. TypeParameterKind, code), but some are replaced with a new implementation (e.g. ContainingSymbol, code).

I cannot judge if it is the correct behaviour, but if it is not the correct one we can create an issue and I can fix it. But to fix it I will need some help to decide what is the expected behaviour in such cases.

I cannot judge if it is the correct behaviour, but if it is not the correct one we can create an issue and I can fix it. But to fix it I will need some help to decide what is the expected behaviour in such cases.

I created a PR #53968 with asserts that represent expected behavior. The asserts are failing for some scenarios. I think the fix for SynthesizedSubstitutedTypeParameterSymbol is to override TypeParameterKind property as it is done in SynthesizedClonedTypeParameterSymbol for VB.

I'll use your branch and implement the fix.

AlekseyTs · 2021-06-09T14:55:21Z

Done with review pass (commit 11). Overall, the change looks good to me. However, we are still debating whether we would like to merge it and open a separate issue to enable proper caching for the affected scenarios, or would like to go with an alternative fix that fixes the caching rather than disables it (we don't have a PR for that yet).

…hodTypeParameter

AlekseyTs

LGTM (commit 13)

AlekseyTs · 2021-06-11T18:27:37Z

we are still debating whether we would like to merge it and open a separate issue to enable proper caching for the affected scenarios, or would like to go with an alternative fix that fixes the caching rather than disables it

Given that the fix is very targeted and affects only scenarios that would produce invalid metadata, we decided to merge it.

AlekseyTs · 2021-06-11T18:28:43Z

@dotnet/roslyn-compiler Need a second sign off on a community PR.

AlekseyTs · 2021-06-11T20:02:54Z

@nbalakin Thanks for the contribution.

…ures/interpolated-string * upstream/main: (95 commits) Update official build number in separate job Update Language Feature Status.md (#54015) Remove IRazorDocumentOptionsService inheritance interface (#54047) Fix comment Simplify Do not create a cache field for lambda if it depends on caller's type argument (#44939) Documentation Documentation Documentation Update test impls Just pass null Pull diagnostics should just request from the doc, not the whole project. Add test plan for file-scoped namespace (#54003) Add source build to official build Improved nullable 'is' analysis (#53311) Multi session service (#53762) Resolve Versions.props conflicts Revert "Revert "Require partial method signatures to match" (47576) (#47879)" (#53352) Broaden enforcement on prototype marker (#53886) Update Language Feature Status.md (#53926) ...

Do not create a cache field for lambda if it depends on caller's type…

5806838

… argument Fix for dotnet#44720

MykolaBalakin requested a review from a team as a code owner June 8, 2020 14:55

jaredpar assigned RikkiGibson Jun 8, 2020

jaredpar added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jun 8, 2020

AlekseyTs reviewed Jun 8, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 8, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated Show resolved Hide resolved

Test that lambda is cached inside a non-generic local function in a loop

5b7f83c

AlekseyTs reviewed Jun 8, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenClosureLambdaTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 8, 2020

View reviewed changes

MykolaBalakin added 2 commits June 9, 2020 23:09

Add some documentation for HasTypeArgumentsFromReferencedMethod method

2ccce78

Add the test description

8801674

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 11, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 12, 2020

View reviewed changes

Use TypeSymbol.ContainsTypeParameter extension method instead of decl…

c1a139b

…aring a custom one

MykolaBalakin marked this pull request as draft June 25, 2020 19:46

Retrieve TypeMap from a subtituted method

fc79b46

MykolaBalakin force-pushed the balakin/broken-generic-lambda branch from 9641750 to fc79b46 Compare June 25, 2020 20:54

AlekseyTs reviewed Jun 30, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs Show resolved Hide resolved

AlekseyTs reviewed Jun 30, 2020

View reviewed changes

Base automatically changed from master to main March 3, 2021 23:52

MykolaBalakin added 5 commits May 30, 2021 20:20

Merge remote-tracking branch 'origin/main' into balakin/broken-generi…

52c6957

…c-lambda

Fix code style convention violations

19247b8

Add more unit tests for caching lambdas inside loop

18e7b11

Do not create a cache field for lambda in case it uses a method type …

d8c2bdc

…parameter

Test if a lambda inside a generic method is cached

ef3dd58

MykolaBalakin marked this pull request as ready for review June 8, 2021 17:22

AlekseyTs reviewed Jun 9, 2021

View reviewed changes

Replace ContainsMethodTypeParameter with more specialized ContainsMet…

48e29a4

…hodTypeParameter

MykolaBalakin requested a review from AlekseyTs June 9, 2021 15:44

Mark unused parameter as nullable

0f9c601

AlekseyTs approved these changes Jun 11, 2021

View reviewed changes

RikkiGibson approved these changes Jun 11, 2021

View reviewed changes

AlekseyTs merged commit bc3212c into dotnet:main Jun 11, 2021

ghost added this to the Next milestone Jun 11, 2021

AlekseyTs mentioned this pull request Jun 11, 2021

Implement proper delegate caching for scenarios affected by https://github.com/dotnet/roslyn/pull/44939 #54042

Open

MykolaBalakin deleted the balakin/broken-generic-lambda branch June 23, 2021 16:03

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

Do not create a cache field for lambda if it depends on caller's type argument #44939

Do not create a cache field for lambda if it depends on caller's type argument #44939

Conversation

MykolaBalakin commented Jun 8, 2020

AlekseyTs Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 11, 2020 • edited Loading

AlekseyTs Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 12, 2020 • edited Loading

agocke commented Jun 12, 2020 • edited Loading

AlekseyTs commented Jun 12, 2020

AlekseyTs Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

MykolaBalakin May 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MykolaBalakin Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 30, 2020 • edited Loading

RikkiGibson commented May 26, 2021

MykolaBalakin commented May 27, 2021 • edited Loading

333fred commented May 27, 2021

AlekseyTs commented May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MykolaBalakin Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 9, 2021

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 11, 2021

AlekseyTs commented Jun 11, 2021

AlekseyTs commented Jun 11, 2021

AlekseyTs Jun 8, 2020 •

edited

Loading

AlekseyTs Jun 25, 2020 •

edited

Loading

AlekseyTs commented Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 12, 2020 •

edited

Loading

AlekseyTs commented Jun 12, 2020 •

edited

Loading

agocke commented Jun 12, 2020 •

edited

Loading

AlekseyTs Jun 30, 2020 •

edited

Loading

AlekseyTs Jun 30, 2020 •

edited

Loading

MykolaBalakin May 30, 2021 •

edited

Loading

MykolaBalakin Jun 7, 2021 •

edited

Loading

AlekseyTs Jun 30, 2020 •

edited

Loading

AlekseyTs commented Jun 30, 2020 •

edited

Loading

MykolaBalakin commented May 27, 2021 •

edited

Loading

AlekseyTs commented May 28, 2021 •

edited

Loading

MykolaBalakin Jun 9, 2021 •

edited

Loading