Ignore function types in return type inference by cston · Pull Request #57713 · dotnet/roslyn

cston · 2021-11-11T19:25:15Z

With C#10 currently, there is a potential breaking change from C#9 in type inference when inferring from the return type of a lambda expression when the return value is another lambda expression or a method group.

See examples in #57517 (comment) and #57630.

To avoid the breaking change, type inference is updated to not infer from the return type of a lambda if the return value is another lambda or method group. This effectively reverts the type inference behavior for those cases to the C#9 behavior.

Fixes #57630.

See also https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1437668.

cston · 2021-11-12T18:49:29Z

src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs

-            var comp = CreateCompilation(source);
-            comp.VerifyDiagnostics(
+
+            var expectedDiagnostics = new[]


var expectedDiagnostics = new[]

There are no changes to the behavior of OverloadResolution_17, _18, _19, _20. The tests have simply been expanded to test -langversion:9 and -langversion:10 as well.

cston · 2021-11-13T17:36:38Z

@dotnet/roslyn-compiler, please review this fix for 17.0, thanks.

CameronAavik · 2021-11-13T23:34:10Z

src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs

+                Diagnostic(ErrorCode.WRN_UnreferencedLocalFunction, "Test4").WithArguments("Test4").WithLocation(7, 6),
+                // (11,1): error CS0411: The type arguments for method 'Test3<T>(Func<Func<T>>)' cannot be inferred from the usage. Try specifying the type arguments explicitly.
+                // Test3(() => () => 3);
+                Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "Test3").WithArguments("Test3<T>(System.Func<System.Func<T>>)").WithLocation(11, 1),


I'm not familiar at all with the rules/specifications around this so feel free to ignore, but is it expected that this case shouldn't support inference? In my bug report (57517) I mentioned that Func<Func<T>> supported inference of T and it seems this change is going in the opposite direction and undoing something that was working? When I made the bug report I was thinking that the outcome would be that inference of T on Func<Expression<Func<T>>> would start working.

Thanks @CameronAavik for catching this and for logging #57517.

With C#10 currently, there is a potential breaking change from C#9 in type inference when inferring from the return type of a lambda expression when the return value is another lambda expression or a method group
(see examples in #57517 (comment) and #57630).

We've decided to fix the breaking change for now by not inferring from the return type of a lambda if the return value is another lambda or method group. This effectively reverts the type inference behavior for those cases to the C#9 behavior.

(I've added this to the PR description above.)

In your examples above, as you noted, this means that inference fails for the calls to Test3() and Test4(), as in C#9.

Separately, we will investigate whether we can improve type inference (in a subsequent release) for these cases by inferring from the function type of the returned lambda or method group. If so, that should allow inference for Test3() and Test4().

I will log a separate issue for that investigation.

cston · 2021-11-15T17:45:15Z

@dotnet/roslyn-compiler, please review, thanks.

AlekseyTs · 2021-11-15T17:59:05Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

        }

-        public TypeWithAnnotations GetInferredReturnType(ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
+        public TypeWithAnnotations GetInferredReturnType(ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, bool allowInferredFromFunctionType)


allowInferredFromFunctionType

The name sounds awkward. Would "allowInferenceFromFunctionType" be better? #Closed

AlekseyTs · 2021-11-15T19:15:24Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

            {
                // Note: so long as we have a best type, we can proceed.
-                var bestTypeWithObliviousAnnotation = TypeWithAnnotations.Create(bestType);
+                var unwrappedType = bestType is FunctionTypeSymbol functionType ?


var unwrappedType = bestType is FunctionTypeSymbol functionType ?

Would it be better to pass unwrapFunctionType: true above? #Closed

Based on your suggestion offline, I've changed the various methods to return a TypeSymbol and a bool rather than requiring callers to pass in the correct value.

AlekseyTs · 2021-11-15T19:17:08Z

            inferredType = TypeWithAnnotations.Create(bestType, BestTypeInferrer.GetNullableAnnotation(resultTypes));

It looks like we can end up with FunctionTypeSymbol here. Is this intentional? #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3569 in 692a53b. [](commit_id = 692a53b, deletion_comment = False)

AlekseyTs · 2021-11-15T19:31:33Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+
+            if (!allowInferredFromFunctionType && inferredReturnType.InferredFromFunctionType)
+            {
+                return default;


return default;

I would expect a much simpler change where a function type wouldn't be simply added to the set of candidates. That would revert behavior to C# 9 behavior, I think. #Closed

AlekseyTs · 2021-11-15T19:32:06Z

Done with review pass (commit 2)

AlekseyTs

LGTM (commit 4)

jcouv · 2021-11-15T22:21:36Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

                case 1:
                    if (conversions.IncludeNullability)
                    {
+                        inferredFromFunctionType = false;


I didn't follow, why is inferredFromFunctionType always false here?

It's assumed that the caller has populated the returns array with the actual types of the corresponding BoundExpression (although if the caller is NullableWalker, the types may differ by inferred nullability). That assumption is unchanged with this PR since callers did not expect this method to return a function type previously.

jcouv · 2021-11-15T22:30:48Z

Do we have a follow-up issue to either make further adjustments for 17.1, or to reify the current behavior into the spec (if we choose to stick with this behavior)?

jcouv

LGTM Thanks (iteration 4)

jcouv

LGTM Thanks (iteration 4)

cston · 2021-11-15T23:05:38Z

Logged #57779 to investigate inferring from the function type instead.

ghost added the Area-Compilers label Nov 11, 2021

cston force-pushed the 57630 branch from b9196aa to 286a168 Compare November 11, 2021 20:27

cston commented Nov 12, 2021

View reviewed changes

cston marked this pull request as ready for review November 12, 2021 18:50

cston requested a review from a team as a code owner November 12, 2021 18:50

cston changed the base branch from main to release/dev17.0-vs-deps November 12, 2021 20:34

cston requested review from a team as code owners November 12, 2021 20:34

cston requested a review from a team November 12, 2021 20:34

cston added 2 commits November 12, 2021 12:37

Ignore function types in return type inference

54412a0

Update test results

692a53b

cston force-pushed the 57630 branch from 286a168 to 692a53b Compare November 12, 2021 20:38

cston removed request for a team November 12, 2021 20:38

cston added this to the 17.0 milestone Nov 13, 2021

CameronAavik reviewed Nov 13, 2021

View reviewed changes

AlekseyTs reviewed Nov 15, 2021

View reviewed changes

cston added 2 commits November 15, 2021 11:52

Rename parameters

0ecb420

Return TypeSymbol and bool

927c27c

AlekseyTs approved these changes Nov 15, 2021

View reviewed changes

jcouv reviewed Nov 15, 2021

View reviewed changes

jcouv approved these changes Nov 15, 2021

View reviewed changes

cston mentioned this pull request Nov 15, 2021

Consider inferring from the function type of a lambda expression that returns a lambda or method group #57779

Open

jcouv approved these changes Nov 15, 2021

View reviewed changes

cston added the Needs Shiproom Approval label Nov 15, 2021

Include nullable

3259462

runfoapp bot mentioned this pull request Nov 16, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

cston merged commit 6ab6601 into dotnet:release/dev17.0-vs-deps Nov 18, 2021

jcouv added Approved to merge and removed Needs Shiproom Approval labels Nov 18, 2021

controlflow mentioned this pull request Nov 22, 2021

Lambdas + custom delegate types + type inference = breaking change in C# 10 #57916

Closed

svick mentioned this pull request Nov 22, 2021

C# 9 => C# 10 upgrade causing CS0411 with an extension method #57630

Closed

cston mentioned this pull request Jan 5, 2022

A part of lambda improvements feature doesn't work with latest SDK 6.0.101 #58619

Closed

CameronAavik mentioned this pull request Jan 18, 2022

Type inference fails for Func<Expression<Func<T>>> #57517

Closed

cston mentioned this pull request Feb 23, 2022

Regression of type argument detection in 6.0.101 (CS0411) #59699

Closed

cston deleted the 57630 branch March 15, 2024 18:20

Conversation

cston commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented Nov 13, 2021

Uh oh!

CameronAavik Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

cston Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented Nov 15, 2021

Uh oh!

AlekseyTs Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 15, 2021

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

cston Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 15, 2021

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cston commented Nov 11, 2021 •

edited

Loading

cston Nov 12, 2021 •

edited

Loading

cston Nov 14, 2021 •

edited

Loading

AlekseyTs Nov 15, 2021 •

edited

Loading

AlekseyTs Nov 15, 2021 •

edited

Loading

AlekseyTs commented Nov 15, 2021 •

edited

Loading

AlekseyTs Nov 15, 2021 •

edited

Loading