Method type inference should make inference from explicit lambda return type by cston · Pull Request #56534 · dotnet/roslyn

cston · 2021-09-19T23:12:22Z

Update "type inference" to make an exact inference from an explicit lambda return type.

Proposed spec changes in bold:

The first phase

For each of the method arguments Ei:

If Ei is an anonymous function and Ti is a delegate type or expression tree type, an explicit parameter type inference (Explicit parameter type inferences) is made from Ei to Ti and an explicit return type inference is made from Ei to Ti.

Otherwise, if Ei has a type U and xi is a value parameter then a lower-bound inference is made from U to Ti.

Otherwise, if Ei has a type U and xi is a ref or out parameter then an exact inference is made from U to Ti.

Otherwise, no inference is made for this argument.

Explicit return type inference

An explicit return type inference is made from an expression E to a type T in the following way:

If E is an anonymous function with explicit return type Ur and T is a delegate type or expression tree type with return type Vr then an exact inference (Exact inferences) is made from Ur to Vr.

See LDM notes.

Fixes #54257

Relates to test plan #52192

…rn type

AlekseyTs · 2021-09-21T18:07:12Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

+            var delegateReturnType = delegateInvokeMethod.ReturnTypeWithAnnotations;
+            if (!delegateReturnType.HasType)
+            {
+                return;


return;

Is this code path reachable? #Closed

AlekseyTs · 2021-09-21T18:10:43Z

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

        {
            var invokeMethod = n.DelegateInvokeMethod;
-            return invokeMethod.Parameters.SequenceEqual(l.Parameters,
+            return invokeMethod!.Parameters.SequenceEqual(l.Parameters,


invokeMethod!

Assert instead? #Closed

An assert seems redundant since the dereference will fail at runtime regardless.

An assert seems redundant since the dereference will fail at runtime regardless.

I think we prefer using asserts to inform nullable analysis about expectations rather than suppressing diagnostics with !.

Chuck swayed me on this in some previous PR, but I don't think we circled back to Jared to update guidelines. If the code would clearly crash, then a suppression seems fine to me (assertion wouldn't add much).
Chuck, let's bring this up to team meeting or with Jared to confirm and capture.

AlekseyTs · 2021-09-21T18:20:35Z

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

+    static void F2<T>(Expression<Func<T, T>> x, T y) { Console.WriteLine(x.GetType()); }
+    static void Main()
+    {
+        F1(int (i) => i, (object)1);


F1(int (i) => i, (object)1);

Do we have a test for a scenario when a conflicting types is coming from parameter type of the lambda? #Closed

AlekseyTs · 2021-09-21T18:27:10Z

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

+        // (see https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-06-21.md).
+        [WorkItem(54257, "https://github.com/dotnet/roslyn/issues/54257")]
+        [Fact]
+        public void TypeInference_ExplicitReturnType_06()


TypeInference_ExplicitReturnType_06

Consider adding a test where the target return type isn't a T, but a generic type closed over T. #Closed

AlekseyTs · 2021-09-21T18:27:46Z

Done with review pass (commit 1)

AlekseyTs

LGTM (commit 2)

jcouv · 2021-09-24T03:45:42Z

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

+
+            comp = CreateCompilation(source);
+            comp.VerifyDiagnostics(expectedDiagnostics);
+        }


Consider adding a test with the explicit return type having a nullable annotation #Resolved

jcouv

LGTM Thanks (iteration 2)

jcouv · 2021-09-24T03:46:32Z

@cston Thanks for the spec sections in OP. Are you copying those back into the feature spec as well?

cston · 2021-09-24T05:20:44Z

Are you copying those back into the feature spec as well?

dotnet/csharplang#5217

jcouv

LGTM Thanks (iteration 4)

ghost added the Area-Compilers label Sep 19, 2021

cston force-pushed the 54257 branch from 0e25c4c to 90cc297 Compare September 20, 2021 01:04

cston changed the title ~~Method type inference should make inference from explicit return type~~ Method type inference should make inference from explicit lambda return type Sep 20, 2021

Method type inference should make inference from explicit lambda retu…

b99838b

…rn type

cston force-pushed the 54257 branch from 313c99e to b99838b Compare September 20, 2021 23:16

cston marked this pull request as ready for review September 20, 2021 23:19

cston requested a review from a team as a code owner September 20, 2021 23:19

cston added the Feature - Lambda Improvements label Sep 21, 2021

cston added this to the 17.0 milestone Sep 21, 2021

AlekseyTs reviewed Sep 21, 2021

View reviewed changes

jcouv mentioned this pull request Sep 21, 2021

Test plan for lambda improvements #52192

Closed

AlekseyTs reviewed Sep 21, 2021

View reviewed changes

PR feedback

2daa9e9

AlekseyTs approved these changes Sep 23, 2021

View reviewed changes

jcouv self-assigned this Sep 24, 2021

jcouv reviewed Sep 24, 2021

View reviewed changes

jcouv approved these changes Sep 24, 2021

View reviewed changes

cston added 2 commits September 23, 2021 21:55

Add nullable tests

ece508a

Update spec comment in MethodTypeInferrer

a010267

jcouv mentioned this pull request Sep 24, 2021

Lambda improvements: include 'Explicit return type inference' spec section dotnet/csharplang#5217

Merged

jcouv approved these changes Sep 24, 2021

View reviewed changes

cston merged commit 2761372 into dotnet:main Sep 24, 2021

cston deleted the 54257 branch September 24, 2021 15:06

ghost modified the milestones: 17.0, Next Sep 24, 2021

Cosifne modified the milestones: Next, 17.0.P5 Sep 27, 2021

Conversation

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

Uh oh!

The first phase

Explicit return type inference

Uh oh!

AlekseyTs Sep 21, 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 Sep 21, 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 Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 21, 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 Sep 21, 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 Sep 21, 2021

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 24, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Sep 24, 2021

Uh oh!

cston commented Sep 24, 2021

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cston commented Sep 19, 2021 •

edited

Loading

AlekseyTs Sep 21, 2021 •

edited

Loading

AlekseyTs Sep 21, 2021 •

edited

Loading

AlekseyTs Sep 21, 2021 •

edited

Loading

AlekseyTs Sep 21, 2021 •

edited

Loading

jcouv Sep 24, 2021 •

edited by cston

Loading