Resolve extension members in all non-invocation contexts #73239

jcouv · 2024-04-25T23:59:39Z

When we bind a member access expression there are two cases:

the expression is considered "invoked" (syntactically part of an invocation_expression), in which case the extension member lookup will filter out non-invocable members,
the expression is not considered "invoked", so if member lookup failed, we can go ahead and use normal extension member lookup (ie. not filtering out non-invocable members) to resolve the expression to a non-method extension member.

This PR implements the second case.

It also removes previous logic we had for resolving to a non-method extension member during a conversion. That logic relied on an incorrect understanding of the spec (that conversions to a delegate type would cause the expression to be treated as "invoked", so we had to wait until processing the conversion to perform the extension member lookup).

So the following scenario becomes an error:

var x = (System.Action)D.f; // binds to field, so error: string cannot convert to Action
System.Action a = D.f; // ditto

class D { }
 
implicit extension E1 for D
{
    public static string f = null;
}
 
implicit extension E2 for object
{
    public static void f() { }
}

Here are the relevant sections of the spec that justify this understanding:

The Member Lookup has concept of a member being "invoked":

"If the simple_name or member_access occurs as the primary_expression of an invocation_expression, the member is said to be invoked."
"Next, if the member is invoked, all non-invocable members are removed from the set."

But in Method group conversions, although treat the conversion as an invocation:

A single method M is selected corresponding to a method invocation of the form E(A) ... [using arguments constructed from the parameter types of delegate type]

The method group conversion only kicks in when we have already determined that we have a method group:

"An implicit conversion exists from a method group (§12.2) to a compatible delegate type (§20.4). If D is a delegate type, and E is an expression that is classified as a method group, ..."

In short, a conversion to a delegate type does not cause the expression to be treated as "invoked", although we process it somewhat like an invocation.

Relates to test plan #66722

jcouv · 2024-04-29T20:56:25Z

    // PROTOTYPE(static) the sorting mechanism for more specific extension members to hide less specific ones is insufficient and needs to be revised

📝 I will address this in my next PR.

The issue is that the sorting helper used in LookupImplicitExtensionMembersInSingleBinder assumes transitivity.
In some orderings of the source, it only performs the following comparisons to arrive at an order: E4 vs. E3 (E4 is not more specific than E3), E1 vs. E4 (E1 is not more specific than E4), E2 vs. E1 (E2 is not more specific than E1).
The resulting order (from more specific to less specific) that it computes is "E3, E4, E1, E2". That order is incorrect because it fails to capture the two key ordering relationships: E1 vs. E3, and E2 vs. E4.

Refers to: src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs:31130 in c4afd43. [](commit_id = c4afd43, deletion_comment = False)

AlekseyTs · 2024-04-30T16:30:09Z

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

+            // All other contexts can go ahead and resolve to a non-method extension member
+            if (!invoked)
+            {
+                result = ResolveToNonMethodExtensionMemberIfPossible(result, diagnostics);


ResolveToNonMethodExtensionMemberIfPossible

Is the name somewhat confusing? We may still resolve to methods, right? #Closed

Maybe I didn't quite understand the question. ResolveToNonMethodExtensionMemberIfPossible only returns non-methods.

AlekseyTs · 2024-04-30T16:38:30Z

            if (resolution.HasAnyErrors)

This isn't primarily related to the changes in this PR, but should we do something for extensions on this code path? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:1127 in bba7662. [](commit_id = bba7662, deletion_comment = False)

jcouv · 2024-04-30T18:46:05Z

            if (resolution.HasAnyErrors)

I think the code in HasCollectionExpressionApplicableAddMethod looks good as-is (we do ResolveMethodGroup above using analyzedArguments, so that'll resolve an instance method, classic extension methods and extension type methods).
But I'll add a comment to check.

Also, I already have a note in the test plan that we need to exclude extension type methods in some collection expression or params scenarios.

In reply to: 2085924348

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:1127 in bba7662. [](commit_id = bba7662, deletion_comment = False)

AlekseyTs · 2024-05-01T16:53:32Z

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

@@ -7560,7 +7543,7 @@ private BoundExpression ResolveToExtensionMemberIfPossible(BoundExpression expr,
            }
        }

-        private BoundExpression BindInstanceMemberAccess(
+        private BoundExpression BindMemberAccessWithBoundLeftInternal(


Internal

What are we trying to convey with the "Internal" suffix? It doesn't look meaningful to me. Consider dropping it. If your goal is as simple as to not share name with the other helper, numeric suffixes "BindMemberAccessWithBoundLeft1" and "BindMemberAccessWithBoundLeft2" will be better in my opinion. #Closed

Yes, my main goal is to remove the Instance portion (misleading) and not share names since we have a cref reference to it. How about "Core" suffix?

How about "Core" suffix?

I do not find this better. A numeric suffix is better in my opinion to avoid sharing the name. Unless we can come up with a meaningful differentiator.

Since you already changed the name, I guess I can live with it.

AlekseyTs · 2024-05-01T16:54:52Z

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

+
+                    if (!invoked)
+                    {
+                        var extensionResult = ResolveToNonMethodExtensionMemberIfPossible(result, diagnostics);


var extensionResult = ResolveToNonMethodExtensionMemberIfPossible(result, diagnostics);

result = ResolveToNonMethodExtensionMemberIfPossible(result, diagnostics);? As we do in tryBindMemberAccessWithBoundTypeLeft. #Closed

AlekseyTs · 2024-05-01T17:00:29Z

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

@@ -721,14 +721,15 @@ private bool HasApplicableMemberWithPossiblyExpandedNonArrayParamsCollection<TMe
                         (analyzedArguments.HasDynamicArgument ? OverloadResolution.Options.DynamicResolution : OverloadResolution.Options.None));
            diagnostics.Add(expression, useSiteInfo);

-            if (resolution.IsExtensionMember(out Symbol extensionMember))
+            if (resolution.IsNonMethodExtensionMember(out Symbol extensionMember))


if (resolution.IsNonMethodExtensionMember(out Symbol extensionMember))

Is this if statement still necessary? Is its body still reachable? Can you give an example of a scenario? #Closed

Yes, we can still get here. Below is an example.
When we have an invocation, we delay the resolution of extensions when binding a member access. We resolve the extensions when binding the invocation (ie. here). So we may find that our receiver is not a method group, but instead an invocable member (for example a delegate type field).
In such case, we bind that member access to the invocable member and invoke it with the given arguments.

public void ExtensionInvocation_OnlyDelegateFieldExists() { // Invocable fields are considered during extension invocation var source = """ E.Field = (i) => { System.Console.Write($"ran({i})"); }; C.Field(42); class C { } delegate void D(int i); implicit extension E for C { public static D Field; } """;

AlekseyTs · 2024-05-01T17:03:03Z

            if (resolution.HasAnyErrors)

I asked the question because I saw that BindMethodGroupInvocation does something special for non method extension members right about before similar place in code.

In reply to: 2086545538

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:1127 in bba7662. [](commit_id = bba7662, deletion_comment = False)

jcouv · 2024-05-01T17:17:26Z

@jjonescz for second review. Thanks

jcouv · 2024-05-01T17:35:00Z

            if (resolution.HasAnyErrors)

Thanks. I identified a scenario that hits that. Blocked it off for now and left a PROTOTYPE comment

In reply to: 2088772543

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:1127 in bba7662. [](commit_id = bba7662, deletion_comment = False)

AlekseyTs · 2024-05-01T18:25:14Z

    // PROTOTYPE(instance) We need to decide whether to allow this and adjust HasCollectionExpressionApplicableAddMethod and other downstream logic accordingly

I am not sure what are you suggesting to allow in this scenario. Especially that the error isn't "near" a collection expression. #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs:37490 in f33ffce. [](commit_id = f33ffce, deletion_comment = False)

AlekseyTs · 2024-05-01T18:26:27Z

MyCollection c = [42];

I would expect an error here, there is no suitable Add method. It must be a method, extensions do not change anything about this requirement. #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs:37477 in f33ffce. [](commit_id = f33ffce, deletion_comment = False)

AlekseyTs · 2024-05-01T18:36:40Z

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

@@ -953,25 +955,6 @@ private static UnboundLambda MakeQueryUnboundLambda(CSharpSyntaxNode node, Query
                    // Could not find an implementation of the query pattern for source type '{0}'.  '{1}' not found.
                    diagnostics.Add(ErrorCode.ERR_QueryNoProvider, node.Location, MessageID.IDS_AnonMethod.Localize(), methodName);
                }
-                else if (ultimateReceiver.Kind == BoundKind.MethodGroup)


else if (ultimateReceiver.Kind == BoundKind.MethodGroup)

I am not sure why this code is removed. The only difference from 'main' is the PROTOTYPE comment. Which means this code wasn't touched for extension types. I think we should keep it. #Closed

This code is unreachable. The case above for ultimateReceiver.HasAnyErrors intercepts all the cases that might be handled here.
The reason I prefer to remove the code rather than just leave it is that it calls ResolveMethodGroup which begs the question for how extensions should be handled. But anything I do here cannot be tested.

This code is unreachable. The case above for ultimateReceiver.HasAnyErrors intercepts all the cases that might be handled here.

Thanks for the explanation. I am good with removal of the code.

AlekseyTs · 2024-05-01T18:45:10Z

Just an FYI, I am done reviewing compiler changes for commit 6. Planning to review tests later today.

AlekseyTs · 2024-05-01T22:44:22Z

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

                        flags: BoundMethodGroupFlags.SearchExtensionMethods, node, typeArgumentsSyntax, diagnostics);
+
+                    if (!invoked)


if (!invoked)

I am not sure why do we do this conditionally. Is there a specific reason to keep non-methods unresolved for invocation scenarios? #Closed

We need arguments to resolve invocation scenarios, even if we're only interested in invocable non-method members.

Consider a classic extension method in an inner scope and an extension type with an invocable non-method member in an outer scope.
Depending on whether the extension method is applicable or not, the invocable non-method member may be found or not.
But to determine whether it is applicable, we need arguments.

From offline discussion, it would be possible to do this unconditionally, but it would not always perform the resolution to a non-extension member (sometimes that will be possible to do only once we have arguments to discard some extension methods that are in closer scopes) and it would probably not be more readable/understandable.

AlekseyTs · 2024-05-01T22:44:49Z

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

                        flags, node, typeArgumentsSyntax, diagnostics);
+
+                    if (!invoked)


if (!invoked)

Same question #Closed

AlekseyTs · 2024-05-01T22:54:47Z

It feels like there are way too many tests added given the small amount of actual code paths modified. Of course, having more tests doesn't hurt. However, consider if there is an excessive redundancy given the final state of the changes in this PR, and, perhaps, some scenarios no longer add real value. #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs:39614 in 0b41777. [](commit_id = 0b41777, deletion_comment = False)

AlekseyTs · 2024-05-01T22:57:42Z

Done with review pass (commit 8)

jcouv · 2024-05-02T03:53:34Z

Makes sense. I pruned some tests that were more relevant for a previous implementation (all scenarios calling BindValue)

In reply to: 2089257969

Refers to: src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs:39614 in 0b41777. [](commit_id = 0b41777, deletion_comment = False)

AlekseyTs · 2024-05-02T14:15:14Z

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

+        //   or than any method from an extension type), then that's the member being accessed.
+        //
+        // - if the extension member lookup finds a method (classic extension method compatible with the receiver or method in extension type;
+        //   closer than any non-method extension member), we don't need to touch the result for the member access (it's a method group already).


we don't need to touch the result for the member access (it's a method group already).

It looks like this part of the comment is no longer relevant because we are no longer producing and passing in the method group. #Closed

My intent is to add some documentation that relates to the callers (which are unfortunately split across two different methods). I'll try to clarify comment

AlekseyTs · 2024-05-02T14:15:51Z

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

+        //
+        // - if the extension member lookup is ambiguous, then we'll use an error symbol as the result of the member access.
+        //
+        // - if the extension member lookup finds nothing, then we don't need to touch the result for the member access.


then we don't need to touch the result for the member access.

This part as well #Closed

AlekseyTs · 2024-05-02T14:33:38Z

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

-                    return expr;
+                if (!memberAccess.HasErrors && typeArgumentsSyntax.Any(SyntaxKind.OmittedTypeArgument))
+                {
+                    Error(diagnostics, ErrorCode.ERR_OmittedTypeArgument, syntax);


Error(diagnostics, ErrorCode.ERR_OmittedTypeArgument, syntax);

Do we have a test covering this code path? Including scenario when the member is a type #Closed

Yes, ExtensionMemberLookup_MatchingExtendedType_GenericType_GenericMember_OmittedTypeArgument (a generic type scenario) and ExtensionInvocation_TypeReceiver_TypeArguments_Omitted and ExtensionInvocation_InstanceReceiver_TypeArguments_Omitted (a generic method, with either a type or an instance receiver).

AlekseyTs

LGTM (commit 12)

AlekseyTs

LGTM (commit 13)

jcouv added Area-Compilers New Language Feature - Roles Roles labels Apr 25, 2024

jcouv self-assigned this Apr 25, 2024

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

jcouv force-pushed the best-type2 branch 5 times, most recently from b1d86df to c4afd43 Compare April 29, 2024 20:42

jcouv force-pushed the best-type2 branch from c4afd43 to 22fc819 Compare April 29, 2024 21:47

Resolve extension members in all non-invocation contexts

bba7662

jcouv force-pushed the best-type2 branch from 22fc819 to bba7662 Compare April 29, 2024 23:19

jcouv marked this pull request as ready for review April 29, 2024 23:19

jcouv requested a review from a team as a code owner April 29, 2024 23:19

jcouv requested review from jjonescz and AlekseyTs April 30, 2024 00:05

AlekseyTs reviewed Apr 30, 2024

View reviewed changes

jcouv added 2 commits April 30, 2024 22:49

Address feedback

e13078b

Tweak

a5d2a0b

jcouv requested a review from AlekseyTs May 1, 2024 14:36

Tweak a test

7ae3ae9

AlekseyTs reviewed May 1, 2024

View reviewed changes

Address feedback

f4f96c6

Address feedback

f33ffce

AlekseyTs reviewed May 1, 2024

View reviewed changes

jcouv added 2 commits May 1, 2024 14:06

Fix collection scenario and test

db623f9

Remove PROTOTYPE comment

0b41777

AlekseyTs reviewed May 1, 2024

View reviewed changes

Address feedback

b523dc2

jcouv added 2 commits May 1, 2024 21:00

Cosmetic

a942cc4

One more test

28770c1

jcouv requested a review from AlekseyTs May 2, 2024 13:33

Fix a test

e5ac37e

AlekseyTs reviewed May 2, 2024

View reviewed changes

AlekseyTs approved these changes May 2, 2024

View reviewed changes

jjonescz approved these changes May 2, 2024

View reviewed changes

Tweak comment

5770a19

jcouv requested a review from AlekseyTs May 2, 2024 19:46

AlekseyTs approved these changes May 2, 2024

View reviewed changes

jcouv merged commit 6b9b076 into dotnet:features/roles May 2, 2024
24 checks passed

jcouv deleted the best-type2 branch May 2, 2024 21:34

		flags: BoundMethodGroupFlags.SearchExtensionMethods, node, typeArgumentsSyntax, diagnostics);

		if (!invoked)

		flags, node, typeArgumentsSyntax, diagnostics);

		if (!invoked)

Resolve extension members in all non-invocation contexts #73239

Resolve extension members in all non-invocation contexts #73239

Conversation

jcouv commented Apr 25, 2024 • edited

jcouv commented Apr 29, 2024 • edited

AlekseyTs Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 30, 2024 • edited

jcouv commented Apr 30, 2024

AlekseyTs May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 1, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 1, 2024

jcouv commented May 1, 2024

jcouv commented May 1, 2024

AlekseyTs commented May 1, 2024 • edited

AlekseyTs commented May 1, 2024 • edited

AlekseyTs May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 1, 2024

AlekseyTs May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 1, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs commented May 1, 2024 • edited

AlekseyTs commented May 1, 2024

jcouv commented May 2, 2024

AlekseyTs May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 2, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Apr 25, 2024 •

edited

jcouv commented Apr 29, 2024 •

edited

AlekseyTs Apr 30, 2024 •

edited

AlekseyTs commented Apr 30, 2024 •

edited

AlekseyTs May 1, 2024 •

edited

AlekseyTs May 1, 2024 •

edited

AlekseyTs May 1, 2024 •

edited

AlekseyTs commented May 1, 2024 •

edited

AlekseyTs commented May 1, 2024 •

edited

AlekseyTs May 1, 2024 •

edited

AlekseyTs May 1, 2024 •

edited

AlekseyTs May 1, 2024 •

edited

AlekseyTs commented May 1, 2024 •

edited

AlekseyTs May 2, 2024 •

edited

AlekseyTs May 2, 2024 •

edited

AlekseyTs May 2, 2024 •

edited