Method group natural type: only use methods that can be fully substituted and pass constraint checks #69226

jcouv · 2023-07-26T04:55:55Z

Speclet: https://github.com/dotnet/csharplang/blob/main/proposals/method-group-natural-type-improvements.md

There's three parts to this change:

we check whether generic extension methods can be fully substituted instead of checking for arity
we check for type constraints
both of those checks occur early (which allows pruning candidates within a given scope)

Relates to test plan #69432

cston · 2023-11-07T19:59:58Z

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

@@ -1231,9 +1264,7 @@ partial class B : A
            yield return getData("internal static void F(this object x, ref object y) { }", "internal static void F(this object x, object y) { }", "this.F", "F"); // different parameter ref kind
            yield return getData("internal static object F(this object x) => throw null;", "internal static ref object F(this object x) => throw null;", "this.F", "F"); // different return ref kind
            yield return getData("internal static ref object F(this object x) => throw null;", "internal static object F(this object x) => throw null;", "this.F", "F"); // different return ref kind
-            yield return getData("internal static void F(this object x, object y) { }", "internal static void F<T>(this object x, T y) { }", "this.F", "F"); // different arity


yield return getData("internal static void F(this object x, object y) { }", "internal static void F(this object x, T y) { }", "this.F", "F");

Instead of removing this entry, consider changing the expected result, by adding the following arguments: ..., null, "A.F", "System.Action<System.Object>"); #ByDesign

Although the added tests are unfortunately verbose, I find them much more readable

cston · 2023-11-07T20:01:21Z

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

            yield return getData("internal static void F(this object x, object y) { }", "internal static void F<T>(this object x, T y) { }", "this.F<int>", "F<int>", null, "B.F", "System.Action<System.Int32>"); // different arity
-            yield return getData("internal static void F<T>(this object x) { }", "internal static void F(this object x) { }", "this.F", "F"); // different arity


yield return getData("internal static void F(this object x) { }", "internal static void F(this object x) { }", "this.F", "F");

Instead of removing this entry, consider changing the expected result, by adding the following arguments: ..., null, "B.F", "System.Action"); #ByDesign

cston · 2023-11-07T20:11:54Z

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

-                    // (5,34): error CS0123: No overload for 'F' matches delegate 'Action'
-                    //         System.Delegate d = this.F<int>;
-                    Diagnostic(ErrorCode.ERR_MethDelegateMismatch, "F<int>").WithArguments("F", "System.Action").WithLocation(5, 34)
-                 }); // different type parameter constraints


Instead of removing this entry, consider changing the expected result, by removing the diagnostic argument. #ByDesign

cston · 2023-11-07T20:16:00Z

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

-                    // (6,34): error CS0123: No overload for 'F' matches delegate 'Action'
-                    //         System.Delegate d = this.F<int>;
-                    Diagnostic(ErrorCode.ERR_MethDelegateMismatch, "F<int>").WithArguments("F", "System.Action").WithLocation(6, 34)
-                 }); // different type parameter constraints


Consider changing the expected result, by removing the diagnostic argument. #ByDesign

cston · 2023-11-07T20:43:09Z

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

+        [Fact]
+        public void GenericExtensionMethod_Constraint()
+        {
+            // In C# 12, a method group that cannot be successfully substituted (ie. respecting constraints)


C# 12

C#13? #Resolved

cston · 2023-11-07T20:44:14Z

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

+        [Fact]
+        public void GenericInstanceMethod_Constraint()
+        {
+            // In C# 12, a method that cannot be successfully substituted (ie. respecting constraints)


C# 12

C#13? #Resolved

AlekseyTs · 2023-11-08T15:49:49Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

            {
-                return null;
-            }
+                foreach (var instanceMethod in node.Methods)


instanceMethod

The name feels somewhat confusing. The method can be a static method, right? "memberMethod" instead? #Closed

AlekseyTs · 2023-11-08T16:06:23Z

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

                return null;
            }

            // For the purpose of construction we use original type parameters in place of type arguments that we couldn't infer from the first argument.
            ImmutableArray<TypeWithAnnotations> typeArgsForConstruct = typeArgs;
-            if (typeArgs.Any(static t => !t.HasType))
+            wasFullyInferred = !typeArgs.Any(static t => !t.HasType);


!typeArgs.Any(static t => !t.HasType)

typeArgs.All(static t => t.HasType)? #Closed

AlekseyTs · 2023-11-08T16:11:00Z

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

+""";
+            var comp = CreateCompilation(new[] { source, s_utils }, parseOptions: TestOptions.RegularPreview, options: TestOptions.ReleaseExe);
+            CompileAndVerify(comp, expectedOutput: "System.Action");
+            comp.VerifyDiagnostics();


.VerifyDiagnostics()

Consider verifying diagnostics on result of CompileAndVerify #Closed

AlekseyTs · 2023-11-08T16:12:53Z

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

+    public void M()
+    {
+        System.Delegate d = F;
+        System.Console.Write(d.GetDelegateTypeName());


d.GetDelegateTypeName()

I think it will be useful to invoke the delegate and observe which method is invoked. #Closed

AlekseyTs · 2023-11-08T16:18:18Z

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

+}
+""";
+            var comp = CreateCompilation(new[] { source, s_utils }, parseOptions: TestOptions.RegularPreview, options: TestOptions.ReleaseExe);
+            comp.VerifyDiagnostics();


.VerifyDiagnostics();

Consider verifying diagnostics on result of CompileAndVerify instead. Applicable to other added tests as well. #Closed

AlekseyTs · 2023-11-08T16:25:15Z

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

@@ -1838,15 +1984,56 @@ static class B
        internal static void F(this object x) { }
    }
 }
+""";
+            var comp = CreateCompilation(new[] { source, s_utils }, parseOptions: useCSharp13 ? TestOptions.RegularNext : TestOptions.RegularPreview);
+            comp.VerifyDiagnostics();


comp.VerifyDiagnostics();

It looks like this becomes a success scenario. We should verify it as such, i.e. execute and observe the behavior #Closed

AlekseyTs · 2023-11-08T16:27:53Z

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

-                // var z = new C().M;
-                Diagnostic(ErrorCode.ERR_CannotInferDelegateType, "new C().M").WithLocation(4, 9)
-                );
+            comp.VerifyDiagnostics();


comp.VerifyDiagnostics();

Verify as success scenario? Applicable to other modified tests as well #Closed

AlekseyTs · 2023-11-08T16:37:13Z

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

+                // var x = new object().F;
+                Diagnostic(ErrorCode.ERR_CannotInferDelegateType, "new object().F").WithLocation(1, 9));
+
+            CreateCompilation(source, parseOptions: TestOptions.RegularNext).VerifyDiagnostics(


RegularNext

What about the latest version? #Closed

It looks like this is applicable to other tests as well.

AlekseyTs · 2023-11-08T16:38:59Z

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

+        [Fact]
+        public void GenericExtensionMethod_ArityCountsInSignature()
+        {
+            // Arity counts as part of "signature" so we have two different signatures even after reducing extension methods


// Arity counts as part of "signature" so we have two different signatures even after reducing extension methods

This doesn't feel right. Since all type arguments are supplied, they shouldn't matter. #Closed

Thanks, that makes sense

AlekseyTs · 2023-11-08T16:43:13Z

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

+    internal static void F(this object x) { }
+}
+""";
+            CreateCompilation(source, parseOptions: TestOptions.Regular11).VerifyDiagnostics(


Regular11

Regular12? It looks like this is applicable to other tests as well. #Closed

AlekseyTs · 2023-11-08T16:48:56Z

Done with review pass (commit 1)

jcouv · 2023-11-14T21:47:34Z

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

@@ -338,6 +338,7 @@ internal sealed class MemberSignatureComparer : IEqualityComparer<Symbol>
            considerTypeConstraints: false,
            refKindCompareMode: RefKindCompareMode.ConsiderDifferences,
            considerCallingConvention: false,
+            considerArity: false,


📝 although the legacy logic uses this comparer, it is not affected by this change since it only performs substitution/reduction once a unique signature is found. #Closed

AlekseyTs · 2023-11-15T15:05:41Z

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

@@ -338,6 +338,7 @@ internal sealed class MemberSignatureComparer : IEqualityComparer<Symbol>
            considerTypeConstraints: false,
            refKindCompareMode: RefKindCompareMode.ConsiderDifferences,
            considerCallingConvention: false,
+            considerArity: false,


false

although the legacy logic uses this comparer, it is not affected by this change since it only performs substitution/reduction once a unique signature is found.

Let's suppose we have two generic methods with different arity, but with the same signature. For example, none of them refer to their type parameters in the signature. Isn't this change going to affect their comparison before substitution?
It feels to me, that keeping a separate comparer for the legacy logic is a small enough price to pay for a peace of mind that the logic isn't broken. #Closed

You're right, the difference is visible in that scenario. Added a test and a legacy comparer. Thanks!

AlekseyTs · 2023-11-15T15:17:06Z

Done with review pass (commit 4)

AlekseyTs

LGTM (commit 5)

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 26, 2023

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 26, 2023

jcouv self-assigned this Jul 26, 2023

jcouv force-pushed the unique-sig2 branch from ba4a47d to 7dca82c Compare July 26, 2023 05:19

jcouv added this to the 17.8 milestone Jul 26, 2023

jcouv force-pushed the unique-sig2 branch 6 times, most recently from dc31635 to b2afd51 Compare July 27, 2023 21:58

jcouv modified the milestones: 17.8, 17.9 Aug 1, 2023

jcouv force-pushed the unique-sig2 branch 2 times, most recently from 3d83756 to 4ed1916 Compare November 3, 2023 19:38

Only use methods that are fully substituted and pass constraint checks

9b14b54

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 3, 2023

jcouv force-pushed the unique-sig2 branch from 4ed1916 to 9b14b54 Compare November 3, 2023 22:10

jcouv marked this pull request as ready for review November 6, 2023 19:13

jcouv requested a review from a team as a code owner November 6, 2023 19:13

jcouv requested review from AlekseyTs and cston November 6, 2023 19:13

cston reviewed Nov 7, 2023

View reviewed changes

AlekseyTs reviewed Nov 8, 2023

View reviewed changes

Address feedback

b674271

cston approved these changes Nov 14, 2023

View reviewed changes

jcouv commented Nov 14, 2023

View reviewed changes

tweak

4af7b83

jcouv requested a review from AlekseyTs November 14, 2023 21:48

ILVerify

6d3a371

AlekseyTs reviewed Nov 15, 2023

View reviewed changes

Address feedback

60891a2

AlekseyTs approved these changes Nov 16, 2023

View reviewed changes

jcouv enabled auto-merge (squash) November 16, 2023 01:17

jcouv merged commit f82627c into dotnet:main Nov 16, 2023
25 checks passed

ghost modified the milestones: 17.9, Next Nov 16, 2023

allisonchou mentioned this pull request Nov 17, 2023

[Automated] PRs inserted in VS build main-34317.88 #70885

Closed

RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Method group natural type: only use methods that can be fully substituted and pass constraint checks #69226

Method group natural type: only use methods that can be fully substituted and pass constraint checks #69226

jcouv commented Jul 26, 2023 •

edited

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

jcouv Nov 14, 2023

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023

AlekseyTs Nov 8, 2023 •

edited

Loading

jcouv Nov 14, 2023

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs commented Nov 8, 2023

jcouv Nov 14, 2023 •

edited

Loading

AlekseyTs Nov 15, 2023 •

edited

Loading

jcouv Nov 15, 2023

AlekseyTs commented Nov 15, 2023

AlekseyTs left a comment

Method group natural type: only use methods that can be fully substituted and pass constraint checks #69226

Method group natural type: only use methods that can be fully substituted and pass constraint checks #69226

Conversation

jcouv commented Jul 26, 2023 • edited Loading

cston Nov 7, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 14, 2023

Choose a reason for hiding this comment

cston Nov 7, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 7, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 14, 2023

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 8, 2023

jcouv Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 15, 2023

Choose a reason for hiding this comment

AlekseyTs commented Nov 15, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Jul 26, 2023 •

edited

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

cston Nov 7, 2023 •

edited by jcouv

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

AlekseyTs Nov 8, 2023 •

edited

Loading

jcouv Nov 14, 2023 •

edited

Loading

AlekseyTs Nov 15, 2023 •

edited

Loading