Implement Lookup APIs on extensions #72948

jcouv · 2024-04-09T04:59:50Z

... and fix nested generic extension type scenario

Note: only LookupStaticMembers and LookupNamespacesAndTypes are implemented. LookupSymbols will be implemented later (as we need to integrate the lookup for extension methods and extension type members before implementing it).

Relates to test plan #66722

jcouv · 2024-04-16T16:54:36Z

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

-                        syntax, right: syntax, diagnostics, ref wasError);
+                    if (!typeArgumentsOpt.IsDefault)
+                    {
+                        namedTypeSymbol = ConstructNamedTypeUnlessTypeArgumentOmitted(syntax, namedTypeSymbol, typeArgumentsSyntax, typeArgumentsOpt, diagnostics);


📝 This fixes a bug with a generic type in an extension. It's illustrated by ExtensionMemberLookup_MatchingExtendedType_GenericType_GenericMember amongst other tests.

jcouv · 2024-04-18T03:42:06Z

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

@@ -2368,6 +2370,7 @@ private BoundExpression BindNameofOperatorInternal(InvocationExpressionSyntax no
            CheckFeatureAvailability(node, MessageID.IDS_FeatureNameof, diagnostics);
            var argument = node.ArgumentList.Arguments[0].Expression;
            var boundArgument = BindExpression(argument, diagnostics);
+            boundArgument = ResolveToExtensionMemberIfPossible(boundArgument, diagnostics);


~~📝 BindToNaturalType (called below) also does this. I've been debating whether to remove the redundant call. We'll discuss~~

This problem goes away in my next PR. It will move ResolveToExtensionMemberIfPossible into BindExpression (for non-invocation cases) and remove it from BindToNaturalType. #Resolved

AlekseyTs · 2024-04-18T16:36:38Z

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

-                        syntax, right: syntax, diagnostics, ref wasError);
+                    if (!typeArgumentsOpt.IsDefault)
+                    {
+                        namedTypeSymbol = ConstructNamedTypeUnlessTypeArgumentOmitted(syntax, namedTypeSymbol, typeArgumentsSyntax, typeArgumentsOpt, diagnostics);


ConstructNamedTypeUnlessTypeArgumentOmitted

Are we testing scenario with omitted type arguments (<,>)? We probably should create an unbound generic type in that case. #Closed

Yes (see tests with _OmittedTypeArgument). Indeed, we produce a unbound generic symbol then.

AlekseyTs · 2024-04-18T16:38:26Z

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

-                        syntax, right: syntax, diagnostics, ref wasError);
+                    if (!typeArgumentsOpt.IsDefault)
+                    {
+                        namedTypeSymbol = ConstructNamedTypeUnlessTypeArgumentOmitted(syntax, namedTypeSymbol, typeArgumentsSyntax, typeArgumentsOpt, diagnostics);


namedTypeSymbol

When and where do we check constraints for this type? #Closed

The constraints get checked a couple of layers into ConstructNamedTypeUnlessTypeArgumentOmitted:
ConstructNamedTypeUnlessTypeArgumentOmitted -> ConstructNamedType -> CheckConstraintsForNamedType.
Example test: InferredVariable_TypeReceiver_GenericType_BrokenConstraint

AlekseyTs · 2024-04-18T16:40:26Z

src/Compilers/CSharp/Test/Emit3/ExtensionTypeTests.cs

+    }
+
+    [Fact]
+    public void ExtensionMemberLookup_MatchingExtendedType_GenericType_GenericMember()


ExtensionMemberLookup_MatchingExtendedType_GenericType_GenericMember

Are we testing similar scenario in type-only context? #Closed

Add ExtensionMemberLookup_MatchingExtendedType_GenericMember_TypeOnlyContext

AlekseyTs · 2024-04-18T16:46:43Z

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

        internal override void GetCandidateExtensionMethods(
            ArrayBuilder<MethodSymbol> methods,
-            string name,
+            string? name,


name

What is the scenario when we don't have a name for an extension method? #Closed

LookupSymbols with includeReducedExtensionMethods = true and name = null.

AlekseyTs · 2024-04-18T16:48:33Z

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

        /// </summary>
-        IncludeExtensionMethods = 1 << 10,
+        IncludeExtensionMethodsAndMembers = 1 << 10,


IncludeExtensionMethodsAndMembers

"IncludeExtensionMembers"? #Closed

AlekseyTs · 2024-04-18T17:10:29Z

src/Compilers/CSharp/Portable/BoundTree/BoundMethodGroup.cs

@@ -54,6 +54,9 @@ public SyntaxNode NameSyntax
            }
        }

+        public SeparatedSyntaxList<TypeSyntax> TypeArgumentsSyntax
+            => NameSyntax is GenericNameSyntax genericName ? genericName.TypeArgumentList.Arguments : default;


NameSyntax is GenericNameSyntax genericName ? genericName.TypeArgumentList.Arguments : default;

This looks fragile. If we need this syntax, we probably should add it to the BoundMethodGroup as a real property set at construction. #Closed

AlekseyTs · 2024-04-18T17:22:35Z

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


-                options |= LookupOptions.AllMethodsOnArityZero;
-                options &= ~LookupOptions.MustBeInstance;


options &= ~LookupOptions.MustBeInstance;

Why not keep this? It looks more reliable than an assert. #Closed

AlekseyTs · 2024-04-18T17:31:19Z

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

+
+                        // We use a Lookup API (which uses a CheckViability check) instead of an AddLookup API (which uses a CanAddLookupSymbolInfo check),
+                        // but that is fine since we filter symbols that cannot be referenced by name below anyways.
+                        scope.Binder.LookupImplicitExtensionMembersInSingleBinder(


scope.Binder.LookupImplicitExtensionMembersInSingleBinder(

It looks like we are adding to the same lookupResult for all scopes. How is this going to interact with most specific shadowing? #Closed

AlekseyTs · 2024-04-18T17:38:35Z

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

-            useSiteInfo.MergeAndClear(ref tempUseSiteInfo);
+            if (tempUseSiteInfo.AccumulatesDiagnostics)
+            {
+                useSiteInfo.MergeAndClear(ref tempUseSiteInfo);


MergeAndClear

It might be better to adjust behavior of MergeAndClear. However, I do not suggest doing this in context of this PR. Consider opening an issue to follow up during quality milestone #Closed

Filled #73188

AlekseyTs · 2024-04-18T17:50:11Z

            MergeExtensionLookupResultsHidingLessSpecific(result, tempResult, basesBeingResolved, ref useSiteInfo);

How is this going to behave when name is null? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs:210 in d45a281. [](commit_id = d45a281, deletion_comment = False)

AlekseyTs · 2024-04-18T17:52:01Z

Done with review pass (commit 1), tests are not looked at.

AlekseyTs · 2024-04-25T20:32:39Z

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

        /// <summary>
        /// Lookup extension methods by name and arity in the given binder and
        /// check viability in this binder. The lookup is performed on a single
        /// binder because extension method search stops at the first applicable
        /// method group from the nearest enclosing namespace.
        /// </summary>
-        private void LookupExtensionMethodsInSingleBinder(ExtensionScope scope, LookupResult result, string name, int arity, LookupOptions options, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
+        internal void LookupExtensionMethodsInSingleBinder(ExtensionScope scope, LookupResult result, string? name, int arity, LookupOptions options, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)


?

Why is this change necessary? I didn't see any of the call sites changed. #Closed

AlekseyTs · 2024-04-25T20:36:16Z

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

@@ -925,14 +943,13 @@ internal virtual bool SupportsExtensions
        /// </summary>
        internal virtual void GetCandidateExtensionMethods(
            ArrayBuilder<MethodSymbol> methods,
-            string name,
+            string? name,


?

Why is this change necessary? I didn't see any of the call sites changed/added #Closed

This is annotating the method as currently implemented

AlekseyTs · 2024-04-25T20:43:25Z

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

        // Lookup member in an extension type
        private void LookupMembersInExtension(
            LookupResult current,
            NamedTypeSymbol type,
-            string name,
+            string? name,


string? name,

I think it would be better to not use this method to collect all members for semantic model purposes. I am not confident that it is going to behave reasonably without the name. #Closed

AlekseyTs · 2024-04-25T20:58:20Z

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

                    this.AddMemberLookupSymbolsInfoInTypeParameter(result, (TypeParameterSymbol)type, options, originalBinder);
                    break;

                case TypeKind.Interface:
-                    this.AddMemberLookupSymbolsInfoInInterface(result, type, options, originalBinder, type);
+                    accessThroughType ??= type;


accessThroughType ??= type;

This looks suspicious. The accessThroughType parameter of AddMemberLookupSymbolsInfoInInterface is used for accessibility check. So, it is not clear to me why would we want to use an extension type for that. #Closed

AlekseyTs · 2024-04-25T20:59:43Z

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

                case TypeKind.Class:
                case TypeKind.Struct:
                case TypeKind.Enum:
                case TypeKind.Delegate:
                case TypeKind.Array:
                case TypeKind.Dynamic:
                case TypeKind.Submission:
-                    this.AddMemberLookupSymbolsInfoInClass(result, type, options, originalBinder, type);
+                    accessThroughType ??= type;


accessThroughType ??= type;

Similar comment #Closed

AlekseyTs · 2024-04-25T21:01:12Z

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

+
+                if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
+                {
+                    AddMemberLookupSymbolsInfoInType(result, extendedType, options, originalBinder, accessThroughType: type);


accessThroughType: type

Similar comment. It is not obvious to me that this is the right thing to do. #Closed

AlekseyTs · 2024-04-26T00:10:54Z

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

                    this.AddMemberLookupSymbolsInfoInTypeParameter(result, (TypeParameterSymbol)type, options, originalBinder);
                    break;

                case TypeKind.Interface:
-                    this.AddMemberLookupSymbolsInfoInInterface(result, type, options, originalBinder, type);
+                    accessThroughType ??= type;


accessThroughType ??= type

Do we actually need to assign? Can we simply pass accessThroughType ?? type as the argument below? #Closed

AlekseyTs · 2024-04-26T17:06:24Z

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

        /// <summary>
        /// Lookup extension methods by name and arity in the given binder and
        /// check viability in this binder. The lookup is performed on a single
        /// binder because extension method search stops at the first applicable
        /// method group from the nearest enclosing namespace.
        /// </summary>
-        private void LookupExtensionMethodsInSingleBinder(ExtensionScope scope, LookupResult result, string name, int arity, LookupOptions options, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
+        internal void LookupExtensionMethodsInSingleBinder(ExtensionScope scope, LookupResult result, string? name, int arity, LookupOptions options, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)


internal

Is accessibility change necessary? I do not see any new call sites #Closed

AlekseyTs · 2024-04-26T17:31:48Z

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

+        internal void AddImplicitExtensionMemberLookupSymbolsInfoForType(LookupSymbolsInfo result, TypeSymbol type, LookupOptions options, Binder originalBinder)
+        {
+            var accessThroughType = type;
+            if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)


if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)

I am not sure why are we continuing otherwise. Could you elaborate, please. #Closed

Perhaps even a better question: "Why would we ever call this method with an extension type?"

Extension types get the benefit of members from other extension types too.

class C { } implicit extension E for C { } implicit extension E2 for C { void Member() { } }

When trying to list all the extension members that are applicable to type E, we should find E2.Member.
Note: we also do this during binding (in LookupImplicitExtensionMembersInSingleBinder).

AlekseyTs · 2024-04-26T17:42:06Z

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

-                options |= LookupOptions.AllMethodsOnArityZero;
-                options &= ~LookupOptions.MustBeInstance;
+                    // PROTOTYPE(static) confirm that we want to exclude type parameters from extension member resolution or leave a comment
+                    if (container is TypeSymbol typeSymbol && !typeSymbol.IsTypeParameter())


if (container is TypeSymbol typeSymbol && !typeSymbol.IsTypeParameter())

Should we check that container is not an extension? Are we looking for extensions on an extension in regular binding scenarios? #Closed

AlekseyTs · 2024-04-26T17:52:40Z

internal static class TypeUnification

Are changes in this file related to lookup changes? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeUnification.cs:14 in 90e907a. [](commit_id = 90e907a, deletion_comment = False)

AlekseyTs · 2024-04-26T18:16:36Z

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

+
+        internal void AddImplicitExtensionMemberLookupSymbolsInfoForType(LookupSymbolsInfo result, TypeSymbol type, LookupOptions options, Binder originalBinder)
+        {
+            var accessThroughType = type;


var accessThroughType = type;

Do we have tests demonstrating impact of this value? #Closed

No, I don't think this will be observable until we have inheritance in extensions

No, I don't think this will be observable until we have inheritance in extensions

Are protected API declared in an extension accessible through extended type today? If they are not, then passing this accessThroughType value might make a difference.

They are not. Added a Lookup test for this and there was no observable impact

AlekseyTs · 2024-04-26T18:16:59Z

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

+
+            void addMemberLookupSymbolsInfoInExtension(LookupSymbolsInfo result, TypeSymbol type, LookupOptions options, Binder originalBinder)
+            {
+                AddMemberLookupSymbolsInfoWithoutInheritance(result, type, options, originalBinder, accessThroughType: type);


accessThroughType: type

Do we have tests demonstrating impact of this value? #Closed

No, I don't think this will be observable until we have inheritance in extensions

AlekseyTs · 2024-04-26T18:17:07Z

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

+
+                if (type.ExtendedTypeNoUseSiteDiagnostics is { } extendedType)
+                {
+                    AddMemberLookupSymbolsInfoInType(result, extendedType, options, originalBinder, accessThroughType: type);


accessThroughType: type

Do we have tests demonstrating impact of this value? #Closed

Yes, Lookup_UnderlyingTypeMembers_Private and Lookup_UnderlyingTypeMembers_Protected

AlekseyTs · 2024-04-26T18:17:44Z

Done with review pass (commit 8)

jcouv · 2024-04-26T19:48:48Z

internal static class TypeUnification

The PR description also includes a fix for nested generic extension type scenario, which includes the change here and the handling of type arguments in GetExtensionMemberAccess.

In reply to: 2079830450

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeUnification.cs:14 in 90e907a. [](commit_id = 90e907a, deletion_comment = False)

AlekseyTs · 2024-04-26T22:53:12Z

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

+
+                foreach (NamedTypeSymbol extension in compatibleExtensions)
+                {
+                    AddMemberLookupSymbolsInfoWithoutInheritance(result, extension, options, scope.Binder, accessThroughType: null);


null

Even if the difference is unobservable right now, I think we were passing the right value, given the meaning of the parameter, #Closed

Restored. Thanks

AlekseyTs · 2024-04-26T22:55:29Z

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

+
+            void addMemberLookupSymbolsInfoInExtension(LookupSymbolsInfo result, TypeSymbol type, LookupOptions options, Binder originalBinder)
+            {
+                AddMemberLookupSymbolsInfoWithoutInheritance(result, type, options, originalBinder, accessThroughType: null);


null

Even if the difference is unobservable right now, I think we were passing the right value, given the meaning of the parameter #Closed

AlekseyTs · 2024-04-26T23:04:05Z

Done with review pass (commit 10)

AlekseyTs

LGTM (commit 11)

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

jcouv self-assigned this Apr 9, 2024

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

jcouv force-pushed the roles-lookup branch from 4e29cb0 to 5e5d1f5 Compare April 16, 2024 16:44

jcouv commented Apr 16, 2024

View reviewed changes

jcouv force-pushed the roles-lookup branch 3 times, most recently from b7f9f4b to 8adbfc9 Compare April 16, 2024 23:36

Lookup APIs and fix nested generic extension type scenario

d45a281

jcouv force-pushed the roles-lookup branch from 8adbfc9 to d45a281 Compare April 17, 2024 01:04

jcouv marked this pull request as ready for review April 17, 2024 05:16

jcouv requested a review from a team as a code owner April 17, 2024 05:16

jcouv requested review from jjonescz and AlekseyTs April 17, 2024 05:16

jcouv commented Apr 18, 2024

View reviewed changes

AlekseyTs reviewed Apr 18, 2024

View reviewed changes

jcouv added 2 commits April 22, 2024 14:38

Add BoundMethodGroup.TypeArgumentsSyntax

712c164

Address feedback

55fbdff

jcouv mentioned this pull request Apr 23, 2024

Adjust CompoundUseSiteInfo<TAssemblySymbol>.MergeAndClear for discarding diagnostic bag #73188

Open

AlekseyTs reviewed Apr 25, 2024

View reviewed changes

AlekseyTs reviewed Apr 26, 2024

View reviewed changes

Address feedback

90e907a

AlekseyTs reviewed Apr 26, 2024

View reviewed changes

jcouv added 2 commits April 26, 2024 13:04

Address feedback

7bce4cb

Missing annotation

657ec34

jcouv requested a review from AlekseyTs April 26, 2024 20:07

AlekseyTs reviewed Apr 26, 2024

View reviewed changes

Restore accessThroughType

24b8f2d

jcouv requested a review from AlekseyTs April 27, 2024 01:07

AlekseyTs approved these changes Apr 27, 2024

View reviewed changes

jcouv requested a review from jjonescz April 27, 2024 01:53

jjonescz approved these changes Apr 29, 2024

View reviewed changes

jcouv merged commit a33cedf into dotnet:features/roles Apr 29, 2024
24 checks passed

jcouv deleted the roles-lookup branch April 29, 2024 14:39


		options \|= LookupOptions.AllMethodsOnArityZero;
		options &= ~LookupOptions.MustBeInstance;

Implement Lookup APIs on extensions #72948

Implement Lookup APIs on extensions #72948

Conversation

jcouv commented Apr 9, 2024 • edited

Choose a reason for hiding this comment

jcouv Apr 18, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 18, 2024 • edited

AlekseyTs commented Apr 18, 2024

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs commented Apr 26, 2024 • edited

AlekseyTs Apr 26, 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 Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 26, 2024

jcouv commented Apr 26, 2024

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 26, 2024 • edited

Choose a reason for hiding this comment

AlekseyTs commented Apr 26, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Apr 9, 2024 •

edited

jcouv Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs Apr 18, 2024 •

edited

AlekseyTs commented Apr 18, 2024 •

edited

AlekseyTs Apr 25, 2024 •

edited

AlekseyTs Apr 25, 2024 •

edited

AlekseyTs Apr 25, 2024 •

edited

AlekseyTs Apr 25, 2024 •

edited

AlekseyTs Apr 25, 2024 •

edited

AlekseyTs Apr 25, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs commented Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited

AlekseyTs Apr 26, 2024 •

edited