Avoid binding lambda body if fully typed #69505
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs
Outdated
Show resolved
Hide resolved
| public static void F1(this A a, object x, Func<int, int> f) => Console.Write("B"); | ||
| } | ||
| """; | ||
| CompileAndVerify(source, expectedOutput: "B").VerifyDiagnostics(); |
There was a problem hiding this comment.
@cston wasn't 100% sure this would be the result after he reflected on the proposed change. Wanted to make sure he saw this.
There was a problem hiding this comment.
Yes, this looks correct to me - no change in behavior. (I took a second look at this case and realized my initial thought was incomplete.)
There was a problem hiding this comment.
Should the test call the ClassifyConversion API here? Including a case where the lambda body is of the wrong type but explicit return is the expected type?
There was a problem hiding this comment.
Also, should we add a new EndToEnd test which shows that the change results in "viable" perf in the relevant scenario?
There was a problem hiding this comment.
Also, should we add a new EndToEnd test which shows that the change results in "viable" perf in the relevant scenario?
Thanks, trying to write such test made me realize this might not be a complete fix (or maybe not easily fixable at all).
For example, when type inference is involved, lambdas are bound later anyway for error recovery
or simply when overload resolution binds the arguments:
Hence the change currently in this PR doesn't seem to improve anything in my testing. Will investigate more.
There was a problem hiding this comment.
Also, should we add a new EndToEnd test which shows that the change results in "viable" perf in the relevant scenario?
#59733 includes a change to track the number of unbound lambdas and number of times lambda expressions are bound. Would something similar be useful here for measuring the impact?
Co-authored-by: Jared Parsons <[email protected]>
| var targetType = comp.GetMember<MethodSymbol>("C.M2").Parameters.Single().Type.GetPublicSymbol(); | ||
| Assert.Equal("System.Func<System.Int32>", targetType.ToTestDisplayString()); | ||
| var conversion = model.ClassifyConversion(lambdaSyntax, targetType); | ||
| Assert.True(conversion.IsValid); |
There was a problem hiding this comment.
Note: this would be false without the change in this PR.
Resolves #69093.