Avoid several breaking changes in overload resolution from inferred types of lambda expressions and method groups by cston · Pull Request #56341 · dotnet/roslyn

cston · 2021-09-11T23:27:47Z

Update "better function member" to prefer members where none of the conversions and none of the type arguments involved inferred types from lambda expressions or method groups.

Better function member

...
Given an argument list A with a set of argument expressions {E1, E2, ..., En} and two applicable function members Mp and Mq with parameter types {P1, P2, ..., Pn} and {Q1, Q2, ..., Qn}, Mp is defined to be a better function member than Mq if

for each argument, the implicit conversion from Ex to Px is not a function_type_conversion, and

Mp is a non-generic method or Mp is a generic method with type parameters {X1, X2, ..., Xp} and for each type parameter Xi the type argument is inferred from an expression or from a type other than a function_type, and

for at least one argument, the implicit conversion from Ex to Qx is a function_type_conversion, or Mq is a generic method with type parameters {Y1, Y2, ..., Yq} and for at least one type parameter Yi the type argument is inferred from a function_type, or

for each argument, the implicit conversion from Ex to Qx is not better than the implicit conversion from Ex to Px, and for at least one argument, the conversion from Ex to Px is better than the conversion from Ex to Qx.

Update "better conversion from expression" to prefer conversions that did not involve inferred types from lambda expressions or method groups.

Better conversion from expression

Given an implicit conversion C1 that converts from an expression E to a type T1, and an implicit conversion C2 that converts from an expression E to a type T2, C1 is a better conversion than C2 if:

C1 is not a function_type_conversion and C2 is a function_type_conversion, or

E is a non-constant interpolated_string_expression, C1 is an implicit_string_handler_conversion, T1 is an applicable_interpolated_string_handler_type, and C2 is not an implicit_string_handler_conversion, or

E does not exactly match T2 and at least one of the following holds:

E exactly matches T1 (Exactly matching Expression)

T1 is a better conversion target than T2 (Better conversion target)

The changes resolve several breaking changes between C#9 and C#10.

Fixes #55691
Fixes #56167
Fixes #56319

Relates to test plan #52192

…not a function conversion

…nce did not infer type arguments from inferred type of lambdas or method groups

…s or argument conversion

jcouv · 2021-09-13T22:34:34Z

Regarding the proposed spec change:

a conversion from the inferred type of an anonymous method or a method group

Can this be simplified to "a function type conversion"?

Similarly, we can use the "function type" label as a shorthand in the third group.
Merging the bullets to reflect top-level OR with 3 groups.

In reply to: 918633815

jcouv · 2021-09-14T02:45:25Z

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

+    internal readonly struct MethodTypeInferenceResult
    {
        public readonly ImmutableArray<TypeWithAnnotations> InferredTypeArguments;
+        public readonly bool InferredFromFunctionType;


nit: Consider adding a comment here and in MemberResolutionResult
nit: Consider aligning names between those two structures (TypeArgumentsFromFunctionType here?) #Closed

jcouv · 2021-09-14T02:47:23Z

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

            if (formalParameterTypes.Length == 0)
            {
-                return new MethodTypeInferenceResult(false, default(ImmutableArray<TypeWithAnnotations>));
+                return new MethodTypeInferenceResult(false, default, false);


nit: consider naming arguments #Closed

jcouv · 2021-09-14T02:53:51Z

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

                }
            }

+            switch ((conv1.Kind, conv2.Kind))


The proposed spec changes covers "better function member" and is implemented by BetterFunctionMember above. But I don't a spec change for better conversion from expression. I think this change is sensible, but let's spec it too. #Resolved

jcouv · 2021-09-14T02:57:05Z

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

+
+        [WorkItem(4674, "https://github.com/dotnet/csharplang/issues/4674")]
+        [Fact]
+        public void OverloadResolution_01A()


consider adding one more variation on this: void M<T>(T, Func<object>) vs. void M<T>(Func<object>, T) (remains error) #Closed

jcouv · 2021-09-14T03:06:22Z

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

+
+        [WorkItem(4674, "https://github.com/dotnet/csharplang/issues/4674")]
+        [Fact]
+        public void OverloadResolution_01D()


Could we add a variant of this scenario with M<T, U>(Func<T>, U) versus M<T, U>(T, U)? I think it will be ambiguous, but may be worth discussing (should we keep a per-argument rule like before rather than adding an aggregate rule where any function type conversion means "worse"?) #Closed

jcouv · 2021-09-14T03:07:04Z

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

-            comp = CreateCompilation(source);
-            comp.VerifyDiagnostics(expectedDiagnostics10AndLater);
+            CompileAndVerify(source, parseOptions: TestOptions.Regular10);
+            CompileAndVerify(source);


Let's verify which overload got picked. Ditto in tests below that are no longer breaks #Closed

jcouv

Done with review pass (iteration 9)

jcouv · 2021-09-14T04:59:57Z

This PR fixes a few recently opened issues. I think #56167 is one of them

cston · 2021-09-15T00:01:11Z

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

+            {
+                // (10,11): error CS0428: Cannot convert method group 'F' to non-delegate type 'Expression'. Did you intend to invoke the method?
+                //         M(F);
+                Diagnostic(ErrorCode.ERR_MethGrpToNonDel, "F").WithArguments("F", "System.Linq.Expressions.Expression").WithLocation(10, 11)


Ideally, overload resolution should choose M(object) rather than M(Expression).

jcouv · 2021-09-15T04:56:14Z

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

+operator+(A a, Func<int> f)
+");
+
+            // Breaking change from C#9.


Let's update the breaking change doc along with this and review that we've documented other breaks we've accepted. Could be done in a separate PR, but feels like it would fit well along with this. #Closed

jcouv

Done with review pass (iteration 12)

- This tests a build of dotnet/roslyn#56341

jcouv · 2021-09-22T20:40:43Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 6.md

+    ```
+
+2. In C# 10, lambda expressions and method groups with inferred type are implicitly convertible to `System.MulticastDelegate`, and bases classes and interfaces of `System.MulticastDelegate` including `object`,
+and lambda expressions are implicitly convertible to `System.Linq.Expressions.Expression` and `System.Linq.Expressions.LambdaExpression`.


lambda expressions

I'd suggest saying "lambda expressions and method groups" here, and convert bullet 3 to be bullet 2c. #Resolved

jcouv

LGTM Thanks (iteration 21) with a suggestion to consolidate breaking change bullet 3 into breaking change bullet 2.

AlekseyTs · 2021-09-22T20:45:10Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 6.md

+
+        static void Main()
+        {
+            F(() => () => 1, 2); // C#9: ok; C#10: ambiguous


ok

"Ok" doesn't really explain the previous behavior. What overload is chosen? #Closed

AlekseyTs · 2021-09-22T20:54:58Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 6.md

+
+        static void Main()
+        {
+            F(() => () => 1, 2); // C#9: F(Func<Func<object>>, int); C#10: ambiguous


// C#9: F(Func<Func>, int); C#10: ambiguous

This behavior change feels unexpected to me, perhaps we should follow up on it. #Resolved

AlekseyTs

LGTM (commit 23)

- This tests a build of dotnet/roslyn#56341

jnm2 · 2021-10-11T15:44:43Z

So many people have noticed the breaks in RC1 that I wonder if this change is worth a mention in dotnet/core#6570.

cston · 2021-10-11T16:12:07Z

I wonder if this change is worth a mention in dotnet/core#6570.

Thanks @jnm2, I've added an entry there.

- This tests a build of dotnet/roslyn#56341

cston added 4 commits September 11, 2021 16:14

Add tests

eec2e0d

Prefer overloads in 'better function member' where the conversion is …

a0909a1

…not a function conversion

Add tests

6afe3d8

Prefer overloads in 'better function member' where method type infere…

1bc6a4e

…nce did not infer type arguments from inferred type of lambdas or method groups

ghost added the Area-Compilers label Sep 11, 2021

cston added 2 commits September 11, 2021 18:43

Misc.

d7463d0

Prefer overloads that did not require inferred type for type argument…

f1c2eb1

…s or argument conversion

cston marked this pull request as ready for review September 13, 2021 17:27

cston requested a review from a team as a code owner September 13, 2021 17:27

jcouv self-assigned this Sep 13, 2021

cston added 3 commits September 13, 2021 11:28

Simplify MethodTypeInferrer

6691280

Misc.

98b8923

Revert

50608e2

cston added this to the 17.0 milestone Sep 13, 2021

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

jcouv reviewed Sep 14, 2021

View reviewed changes

cston added 3 commits September 14, 2021 12:19

PR feedback

75ea2bd

More tests

56bb01f

More tests

49ac253

cston commented Sep 15, 2021

View reviewed changes

jcouv reviewed Sep 15, 2021

View reviewed changes

cston added 4 commits September 17, 2021 09:31

Move several tests - no code change

eea530e

Add test

cc7505a

Move test

9f55dd5

Update breaking changes doc

22e3187

halter73 mentioned this pull request Sep 20, 2021

Verify new roslyn overload resolution changes don't break minimal API scenarios dotnet/aspnetcore#36776

Closed

halter73 added a commit to dotnet/aspnetcore that referenced this pull request Sep 22, 2021

Verify new roslyn changes don't break us

505b314

- This tests a build of dotnet/roslyn#56341

halter73 mentioned this pull request Sep 22, 2021

Verify new roslyn changes don't break us dotnet/aspnetcore#36850

Closed

Add indexer example

b9bbbdb

jcouv reviewed Sep 22, 2021

View reviewed changes

jcouv approved these changes Sep 22, 2021

View reviewed changes

PR feedback

c4abad9

AlekseyTs reviewed Sep 22, 2021

View reviewed changes

Update comment

1a38ee6

AlekseyTs reviewed Sep 22, 2021

View reviewed changes

AlekseyTs approved these changes Sep 22, 2021

View reviewed changes

cston mentioned this pull request Sep 22, 2021

Investigate lambda return type inference behavior for nested lambdas #56623

Closed

cston merged commit 7cce6c7 into dotnet:main Sep 22, 2021

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

cston deleted the breaking-changes branch September 22, 2021 23:13

jcouv mentioned this pull request Sep 24, 2021

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

Merged

cston mentioned this pull request Sep 24, 2021

Correct resolving of delegates and values parameters #56697

Closed

cston pushed a commit to cston/aspnetcore that referenced this pull request Sep 24, 2021

Verify new roslyn changes don't break us

bfe1190

- This tests a build of dotnet/roslyn#56341

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

cston mentioned this pull request Sep 28, 2021

CS0121 Compilation error with .NET 6 and delegates #56807

Closed

cston mentioned this pull request Oct 11, 2021

What's new in .NET 6 RC2 [WIP] dotnet/core#6570

Closed

cston pushed a commit to cston/aspnetcore that referenced this pull request Nov 13, 2021

Verify new roslyn changes don't break us

351bab8

- This tests a build of dotnet/roslyn#56341

Conversation

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

Uh oh!

Better function member

Better conversion from expression

Uh oh!

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

Uh oh!

Uh oh!

jcouv Sep 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!

jcouv Sep 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!

jcouv Sep 14, 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 Sep 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!

jcouv Sep 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!

jcouv Sep 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Sep 14, 2021

Uh oh!

cston Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 22, 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!

AlekseyTs Sep 22, 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 22, 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!

cston Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

cston commented Oct 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

cston commented Sep 11, 2021 •

edited

Loading

jcouv commented Sep 13, 2021 •

edited by cston

Loading

jcouv Sep 14, 2021 •

edited

Loading

jcouv Sep 14, 2021 •

edited

Loading

jcouv Sep 14, 2021 •

edited by cston

Loading

jcouv Sep 14, 2021 •

edited

Loading

jcouv Sep 14, 2021 •

edited

Loading

jcouv Sep 14, 2021 •

edited

Loading

jcouv Sep 15, 2021 •

edited

Loading

jcouv Sep 22, 2021 •

edited by cston

Loading

AlekseyTs Sep 22, 2021 •

edited

Loading

AlekseyTs Sep 22, 2021 •

edited by cston

Loading

jnm2 commented Oct 11, 2021 •

edited

Loading