Adjust semantic model for method group conversion #75719

jcouv · 2024-11-04T20:54:35Z

In the scenario, we have a user-defined conversion for cast, stacked with a method group conversion.

333fred · 2024-11-07T21:03:48Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -4294,6 +4294,12 @@ private OneOrMany<Symbol> GetMethodGroupSemanticSymbols(
                        // we want to get the symbol that overload resolution chose for M, not the whole method group M.
                        var conversion = (BoundConversion)boundNodeForSyntacticParent;

+                        if (conversion.ConversionKind is not ConversionKind.MethodGroup && conversion.Operand is BoundConversion nestedConversion)


Nit:

Suggested change

if (conversion.ConversionKind is not ConversionKind.MethodGroup && conversion.Operand is BoundConversion nestedConversion)

if (conversion is { ConversionKind: not ConversionKind.MethodGroup, Operand: BoundConversion nestedConversion })

``` #Resolved

AlekseyTs · 2024-11-08T15:30:45Z

@jcouv Was this a regression?

AlekseyTs · 2024-11-08T15:32:08Z

src/Compilers/CSharp/Test/Symbol/Symbols/ConversionTests.cs

@@ -425,6 +426,112 @@ static void Main()
            Assert.True(conversion.IsNumeric);
        }

+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/36377")]
+        public void GetSymbolInfo_ExplicitCastOnMethodGroup()


Consider adding a similar test for VB #Closed

AlekseyTs · 2024-11-08T15:34:47Z

src/Compilers/CSharp/Test/Symbol/Symbols/ConversionTests.cs

+
+    public static int Test() => 1;
+
+    public static explicit operator C(System.Func<int> intDelegate)


explicit

Consider testing with implicit operator, including a scenario when the cast operator, (C), is not present in code. #Closed

Done. There are some existing unexpected behaviors there. Filing issues for those

AlekseyTs · 2024-11-08T15:38:56Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -4294,6 +4294,12 @@ private OneOrMany<Symbol> GetMethodGroupSemanticSymbols(
                        // we want to get the symbol that overload resolution chose for M, not the whole method group M.
                        var conversion = (BoundConversion)boundNodeForSyntacticParent;

+                        if (conversion.ConversionKind is not ConversionKind.MethodGroup && conversion.Operand is BoundConversion nestedConversion)


nestedConversion

It feels like we should also check how this node related to the conversion syntactically #Closed

AlekseyTs · 2024-11-08T15:44:41Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -4294,6 +4294,12 @@ private OneOrMany<Symbol> GetMethodGroupSemanticSymbols(
                        // we want to get the symbol that overload resolution chose for M, not the whole method group M.
                        var conversion = (BoundConversion)boundNodeForSyntacticParent;

+                        if (conversion.ConversionKind is not ConversionKind.MethodGroup && conversion.Operand is BoundConversion nestedConversion)
+                        {
+                            Debug.Assert(conversion.ConversionKind is ConversionKind.NoConversion || conversion.ExplicitCastInCode);


Debug.Assert(conversion.ConversionKind is ConversionKind.NoConversion || conversion.ExplicitCastInCode);

It is not obvious why is it reasonable to make this assumption. And more importantly why do we need to make it. If we are going after a specific bound tree shape, I think we should be checking for that instead. For example, we might want to confirm that the nested conversion is the immediate conversion for the BoundMethodGroup boundNode. If that is not the case, switching the conversion node doesn't feel appropriate. #Closed

AlekseyTs · 2024-11-08T15:47:47Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

+                        if (conversion.ConversionKind is not ConversionKind.MethodGroup && conversion.Operand is BoundConversion nestedConversion)
+                        {
+                            Debug.Assert(conversion.ConversionKind is ConversionKind.NoConversion || conversion.ExplicitCastInCode);
+                            conversion = nestedConversion;


conversion = nestedConversion;

Consider adding a comment about why/when we need to do this transformation. #Closed

AlekseyTs · 2024-11-08T15:48:42Z

Done with review pass (commit 1)

AlekseyTs · 2024-11-08T16:04:09Z

                        Debug.Assert(conversion.ConversionKind == ConversionKind.MethodGroup);

Perhaps this should be converted into a real check? Basically, we are interested in a method group conversion and we should check for that rather than assume that this is what we have. #Closed

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs:4306 in 03f70f7. [](commit_id = 03f70f7, deletion_comment = False)

jcouv · 2024-11-08T17:16:52Z

@jcouv Was this a regression?

Nothing indicates that in the linked issue (filed in 2019).

AlekseyTs · 2024-11-08T19:29:21Z

src/Compilers/CSharp/Test/Symbol/Symbols/ConversionTests.cs

+            var tree = comp.SyntaxTrees.Single();
+            var model = comp.GetSemanticModel(tree);
+            var memberAccess = GetSyntax<MemberAccessExpressionSyntax>(tree, "C.Test");
+            Assert.Equal("C C.op_Implicit(System.Func<System.Int32> intDelegate)", model.GetSymbolInfo(memberAccess).Symbol.ToTestDisplayString());  // Unexpected: Should be "void C.Test()"


// Unexpected: Should be "void C.Test()"

Are we planning to address this? #Closed

Opened #75833

AlekseyTs · 2024-11-08T19:59:48Z

src/Compilers/VisualBasic/Test/Semantic/Semantics/Conversions.vb

+        Dim x As C = DirectCast(AddressOf C.Test, C)
+                                ~~~~~~~~~~~~~~~~
+</expected>)
+        End Sub


End Sub

It doesn't look like we are testing SemanticModel behavior #Closed

AlekseyTs · 2024-11-08T20:07:01Z

Done with review pass (commit 2)

AlekseyTs

LGTM (commit 4)

jcouv added the Area-Compilers label Nov 4, 2024

jcouv self-assigned this Nov 4, 2024

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 4, 2024

jcouv force-pushed the method-conversion branch 4 times, most recently from 146f4ac to 4edb67b Compare November 6, 2024 20:45

build-analysis bot mentioned this pull request Nov 6, 2024

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

Adjust semantic model for method group conversion

03f70f7

jcouv force-pushed the method-conversion branch from 4edb67b to 03f70f7 Compare November 7, 2024 18:40

jcouv marked this pull request as ready for review November 7, 2024 19:49

jcouv requested a review from a team as a code owner November 7, 2024 19:49

333fred approved these changes Nov 7, 2024

View reviewed changes

AlekseyTs reviewed Nov 8, 2024

View reviewed changes

Address feedback

90f20a3

AlekseyTs reviewed Nov 8, 2024

View reviewed changes

jcouv added 2 commits November 8, 2024 12:10

Add few more tests for existing issue and clarify expectations

ff61253

Semantic check in VB error scenario too

29f5823

jcouv requested a review from 333fred November 8, 2024 20:18

jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 11, 2024

jcouv requested a review from AlekseyTs November 11, 2024 17:01

AlekseyTs approved these changes Nov 11, 2024

View reviewed changes

333fred approved these changes Nov 11, 2024

View reviewed changes

jcouv merged commit 32f7283 into dotnet:main Nov 11, 2024
24 checks passed

jcouv deleted the method-conversion branch November 11, 2024 22:30

dotnet-policy-service bot added this to the Next milestone Nov 11, 2024

This was referenced Nov 15, 2024

[Automated] PRs inserted in VS build main-35514.35 #75927

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35514.242 #75931

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35518.109 #75965

Closed

jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust semantic model for method group conversion #75719

Adjust semantic model for method group conversion #75719

jcouv commented Nov 4, 2024 •

edited

Loading

333fred Nov 7, 2024 •

edited by jcouv

Loading

AlekseyTs commented Nov 8, 2024

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

jcouv Nov 8, 2024

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs commented Nov 8, 2024

AlekseyTs commented Nov 8, 2024 •

edited

Loading

jcouv commented Nov 8, 2024

AlekseyTs Nov 8, 2024 •

edited

Loading

jcouv Nov 8, 2024

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs commented Nov 8, 2024

AlekseyTs left a comment

	if (conversion.ConversionKind is not ConversionKind.MethodGroup && conversion.Operand is BoundConversion nestedConversion)
	if (conversion is { ConversionKind: not ConversionKind.MethodGroup, Operand: BoundConversion nestedConversion })
	``` #Resolved


		public static int Test() => 1;

		public static explicit operator C(System.Func<int> intDelegate)

Adjust semantic model for method group conversion #75719

Adjust semantic model for method group conversion #75719

Conversation

jcouv commented Nov 4, 2024 • edited Loading

333fred Nov 7, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 8, 2024

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2024

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 8, 2024

AlekseyTs commented Nov 8, 2024 • edited Loading

jcouv commented Nov 8, 2024

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 8, 2024

Choose a reason for hiding this comment

AlekseyTs Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 8, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Nov 4, 2024 •

edited

Loading

333fred Nov 7, 2024 •

edited by jcouv

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs commented Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading

AlekseyTs Nov 8, 2024 •

edited

Loading