Extensions: Address follow-ups related to collection expressions

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

@@ -1467,11 +1467,25 @@ static ImmutableArray<MethodSymbol> filterOutBadGenericMethods(
                 {
                     // If the method is generic, skip it if the type arguments cannot be inferred.
                     var member = candidate.Member;
+
+                    // For new extension methods, we'll use the extension implementation method to determine inferrability
+                    if (member.GetIsNewExtensionMember())
+                    {
+                        if (member.TryGetCorrespondingExtensionImplementationMethod() is { } extensionImplementation)
+                        {
+                            member = extensionImplementation;
+                        }
+                        else
+                        {
+                            continue;
+                        }
+                    }
+
                     var typeParameters = member.TypeParameters;
 
                     if (!typeParameters.IsEmpty)
                     {
-                        if (resolution.IsExtensionMethodGroup) // Tracked by https://github.com/dotnet/roslyn/issues/78960 : we need to handle new extension methods
+                        if (resolution.IsExtensionMethodGroup)
                         {
                             // We need to validate an ability to infer type arguments as well as check conversion to 'this' parameter.
                             // Overload resolution doesn't check the conversion when 'this' type refers to a type parameter
@@ -1522,7 +1536,7 @@ static ImmutableArray<MethodSymbol> filterOutBadGenericMethods(
                                     parameterTypes,
                                     parameterRefKinds,
                                     ImmutableArray.Create<BoundExpression>(methodGroup.ReceiverOpt, new BoundValuePlaceholder(syntax, secondArgumentType) { WasCompilerGenerated = true }),
-                                    ref useSiteInfo); // Tracked by https://github.com/dotnet/roslyn/issues/78960 : we may need to override ordinals here
+                                    ref useSiteInfo);
 
                                 if (!inferenceResult.Success)
                                 {
@@ -1599,42 +1613,10 @@ static bool bindInvocationExpressionContinued(
                     return false;
                 }
 
-                // Otherwise, there were no dynamic arguments and overload resolution found a unique best candidate. 
-                // We still have to determine if it passes final validation.
-
-                var methodResult = result.ValidResult;
-                var method = methodResult.Member;
-
-                // Tracked by https://github.com/dotnet/roslyn/issues/78960: It looks like we added a bunch of code in BindInvocationExpressionContinued at this position
-                //            that specifically deals with new extension methods. It adjusts analyzedArguments, etc.
-                //            It is very likely we need to do the same here. 
-
-                // It is possible that overload resolution succeeded, but we have chosen an
-                // instance method and we're in a static method. A careful reading of the
-                // overload resolution spec shows that the "final validation" stage allows an
-                // "implicit this" on any method call, not just method calls from inside
-                // instance methods. Therefore we must detect this scenario here, rather than in
-                // overload resolution.
-
-                var receiver = methodGroup.Receiver;
-
-                // Note: we specifically want to do final validation (7.6.5.1) without checking delegate compatibility (15.2),
-                // so we're calling MethodGroupFinalValidation directly, rather than via MethodGroupConversionHasErrors.
-                // Note: final validation wants the receiver that corresponds to the source representation
-                // (i.e. the first argument, if invokedAsExtensionMethod).
-                var gotError = addMethodBinder.MemberGroupFinalValidation(receiver, method, expression, diagnostics, invokedAsExtensionMethod);
-
-                addMethodBinder.ReportDiagnosticsIfObsolete(diagnostics, method, node, hasBaseReceiver: false);
-                ReportDiagnosticsIfUnmanagedCallersOnly(diagnostics, method, node, isDelegateConversion: false);
-                ReportDiagnosticsIfDisallowedExtension(diagnostics, method, node);
-
-                // No use site errors, but there could be use site warnings.
-                // If there are any use site warnings, they have already been reported by overload resolution.
-                Debug.Assert(!method.HasUseSiteError, "Shouldn't have reached this point if there were use site errors.");
-                Debug.Assert(!method.IsRuntimeFinalizer());
-
-                addMethod = method;
-                return !gotError;
+                // Although this function is modelled after `BindInvocationExpressionContinued`,
+                // since `HasCollectionExpressionApplicableAddMethod` uses a placeholder element of type `dynamic`,
+                // only the first listed error case can be hit.
+                throw ExceptionUtilities.Unreachable();
             }
         }
 
@@ -1668,7 +1650,7 @@ internal static BoundExpression GetUnderlyingCollectionExpressionElement(BoundCo
                         // Add methods. This case can be hit for spreads and non-spread elements.
                         Debug.Assert(call.HasErrors);
                         Debug.Assert(call.Method.Name == "Add");
-                        return call.Arguments[call.InvokedAsExtensionMethod ? 1 : 0]; // Tracked by https://github.com/dotnet/roslyn/issues/78960: Add test coverage for new extensions
+                        return call.Arguments[call.InvokedAsExtensionMethod ? 1 : 0];
                     case BoundBadExpression badExpression:
                         Debug.Assert(false); // Add test if we hit this assert.
                         return badExpression;
@@ -1717,7 +1699,7 @@ internal bool TryGetCollectionIterationType(SyntaxNode syntax, TypeSymbol collec
                 out iterationType,
                 builder: out var builder);
             // Collection expression target types require instance method GetEnumerator.
-            if (result && builder.ViaExtensionMethod) // Tracked by https://github.com/dotnet/roslyn/issues/78960: Add test coverage for new extensions
+            if (result && builder.ViaExtensionMethod)
             {
                 iterationType = default;
                 return false;

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

@@ -4604,7 +4604,7 @@ private MemberAnalysisResult IsApplicable(
                     Debug.Assert(
                         !forExtensionMethodThisArg ||
                         (!conversion.IsDynamic ||
-                            (ignoreOpenTypes && parameters.ParameterTypes[argumentPosition].Type.ContainsTypeParameter(parameterContainer: (MethodSymbol)candidate))));
+                            (ignoreOpenTypes && TypeContainsTypeParameterFromContainer(candidate, parameters.ParameterTypes[argumentPosition].Type))));
 
                     if (forExtensionMethodThisArg && !conversion.IsDynamic && !Conversions.IsValidExtensionMethodThisArgConversion(conversion))
                     {
@@ -4702,7 +4702,7 @@ private Conversion CheckArgumentForApplicability(
             // - Then, any parameter whose type is open (i.e. contains a type parameter; see §4.4.2) is elided, along with its corresponding parameter(s).
             // and
             // - The modified parameter list for F is applicable to the modified argument list in terms of section §7.5.3.1
-            if (ignoreOpenTypes && parameterType.ContainsTypeParameter(parameterContainer: (MethodSymbol)candidate))
+            if (ignoreOpenTypes && TypeContainsTypeParameterFromContainer(candidate, parameterType))
             {
                 // defer applicability check to runtime:
                 return Conversion.ImplicitDynamic;
@@ -4748,6 +4748,16 @@ private Conversion CheckArgumentForApplicability(
             }
         }
 
+        private static bool TypeContainsTypeParameterFromContainer(Symbol container, TypeSymbol parameterType)
+        {
+            if (parameterType.ContainsTypeParameter(typeParameterContainer: container))
+            {
+                return true;
+            }
+
+            return container.GetIsNewExtensionMember() && parameterType.ContainsTypeParameter(typeParameterContainer: container.ContainingType);
+        }
+
         private static TMember GetConstructedFrom<TMember>(TMember member) where TMember : Symbol
         {
             switch (member.Kind)

src/Compilers/CSharp/Portable/Symbols/Source/ExtensionGroupingInfo.cs

@@ -222,7 +222,7 @@ internal static bool HaveSameILSignature(SourceNamedTypeSymbol extension1, Sourc
             TypeMap? typeMap1 = MemberSignatureComparer.GetTypeMap(extension1);
             TypeMap? typeMap2 = MemberSignatureComparer.GetTypeMap(extension2);
             if (extension1.Arity > 0
-                && !MemberSignatureComparer.HaveSameConstraints(extension1.TypeParameters, typeMap1, extension2.TypeParameters, typeMap2, TypeCompareKind.AllIgnoreOptions))
+                && !MemberSignatureComparer.HaveSameConstraints(extension1.TypeParameters, typeMap1, extension2.TypeParameters, typeMap2, TypeCompareKind.CLRSignatureCompareOptions))
             {
                 return false;
             }
@@ -236,7 +236,7 @@ internal static bool HaveSameILSignature(SourceNamedTypeSymbol extension1, Sourc
 
             if (!MemberSignatureComparer.HaveSameParameterType(parameter1, typeMap1, parameter2, typeMap2,
                 refKindCompareMode: MemberSignatureComparer.RefKindCompareMode.IgnoreRefKind,
-                considerDefaultValues: false, TypeCompareKind.AllIgnoreOptions))
+                considerDefaultValues: false, TypeCompareKind.CLRSignatureCompareOptions))
             {
                 return false;
             }

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

@@ -43,25 +43,20 @@ private SymbolEqualityComparer(TypeCompareKind comparison)
 
         internal static EqualityComparer<Symbol> Create(TypeCompareKind comparison)
         {
-            if (comparison == (TypeCompareKind.IgnoreDynamicAndTupleNames | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
+            switch (comparison)
             {
-                return IgnoringDynamicTupleNamesAndNullability;
+                case TypeCompareKind.IgnoreDynamicAndTupleNames | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes:
+                    return IgnoringDynamicTupleNamesAndNullability;
+                case TypeCompareKind.ConsiderEverything:
+                    return ConsiderEverything;
+                case TypeCompareKind.AllIgnoreOptionsPlusNullableWithObliviousMatchesAny:
+                    return AllIgnoreOptionsPlusNullableWithUnknownMatchesAny;
+                case TypeCompareKind.CLRSignatureCompareOptions:
+                    return CLRSignature;
+                default:
+                    Debug.Assert(false, "Consider optimizing as above when we need to handle a new type comparison kind.");
+                    return new SymbolEqualityComparer(comparison);
             }
-            else if (comparison == TypeCompareKind.ConsiderEverything)
-            {
-                return ConsiderEverything;
-            }
-            else if (comparison == TypeCompareKind.AllIgnoreOptions)
-            {
-                return AllIgnoreOptions;
-            }
-            else if (comparison == TypeCompareKind.AllIgnoreOptionsPlusNullableWithObliviousMatchesAny)
-            {
-                return AllIgnoreOptionsPlusNullableWithUnknownMatchesAny;
-            }
-
-            Debug.Assert(false, "Consider optimizing as above when we need to handle a new type comparison kind.");
-            return new SymbolEqualityComparer(comparison);
         }
 
         public override int GetHashCode(Symbol obj)

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

@@ -42,7 +42,7 @@ internal TypeMap(ImmutableArray<TypeParameterSymbol> from, ImmutableArray<TypeWi
             : base(ConstructMapping(from, to))
         {
             // mapping contents are read-only hereafter
-            Debug.Assert(allowAlpha || !from.Any(static tp => tp is SubstitutedTypeParameterSymbol));
+            Debug.Assert(allowAlpha || from.All(static tp => tp.IsDefinition));
         }
 
         // Only when the caller passes allowAlpha=true do we tolerate substituted (alpha-renamed) type parameters as keys

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

@@ -1205,16 +1205,16 @@ public static bool ContainsTypeParameter(this TypeSymbol type, TypeParameterSymb
         private static readonly Func<TypeSymbol, TypeParameterSymbol?, bool, bool> s_containsTypeParameterPredicate =
             (type, parameter, unused) => type.TypeKind == TypeKind.TypeParameter && (parameter is null || TypeSymbol.Equals(type, parameter, TypeCompareKind.ConsiderEverything2));
 
-        public static bool ContainsTypeParameter(this TypeSymbol type, MethodSymbol parameterContainer)
+        public static bool ContainsTypeParameter(this TypeSymbol type, Symbol typeParameterContainer)
         {
-            RoslynDebug.Assert((object)parameterContainer != null);
+            RoslynDebug.Assert((object)typeParameterContainer != null);
 
-            var result = type.VisitType(s_isTypeParameterWithSpecificContainerPredicate, parameterContainer);
+            var result = type.VisitType(s_isTypeParameterWithSpecificContainerPredicate, typeParameterContainer);
             return result is object;
         }
 
         private static readonly Func<TypeSymbol, Symbol, bool, bool> s_isTypeParameterWithSpecificContainerPredicate =
-             (type, parameterContainer, unused) => type.TypeKind == TypeKind.TypeParameter && (object)type.ContainingSymbol == (object)parameterContainer;
+             (type, typeParameterContainer, unused) => type.TypeKind == TypeKind.TypeParameter && (object)type.ContainingSymbol == (object)typeParameterContainer;
 
         public static bool ContainsTypeParameters(this TypeSymbol type, HashSet<TypeParameterSymbol> parameters)
         {

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

@@ -36378,12 +36378,12 @@ static class E
 """;
         var comp = CreateCompilation(src);
         comp.VerifyEmitDiagnostics(
-            // (2,1): error CS1929: 'object' does not contain a definition for 'M' and the best extension method overload 'E.extension<U>(U).M<T>(T)' requires a receiver of type 'U'
+            // (2,1): error CS1973: 'object' has no applicable method named 'M' but appears to have an extension method by that name. Extension methods cannot be dynamically dispatched. Consider casting the dynamic arguments or calling the extension method without the extension method syntax.
             // object.M(d);
-            Diagnostic(ErrorCode.ERR_BadInstanceArgType, "object").WithArguments("object", "M", "E.extension<U>(U).M<T>(T)", "U").WithLocation(2, 1),
-            // (3,1): error CS1929: 'object' does not contain a definition for 'M' and the best extension method overload 'E.extension<U>(U).M<T>(T)' requires a receiver of type 'U'
+            Diagnostic(ErrorCode.ERR_BadArgTypeDynamicExtension, "object.M(d)").WithArguments("object", "M").WithLocation(2, 1),
+            // (3,1): error CS0176: Member 'E.extension<U>(U).M<T>(T)' cannot be accessed with an instance reference; qualify it with a type name instead
             // new object().M(d);
-            Diagnostic(ErrorCode.ERR_BadInstanceArgType, "new object()").WithArguments("object", "M", "E.extension<U>(U).M<T>(T)", "U").WithLocation(3, 1));
+            Diagnostic(ErrorCode.ERR_ObjectProhibited, "new object().M").WithArguments("E.extension<U>(U).M<T>(T)").WithLocation(3, 1));
     }
 
     [Fact]
@@ -36432,9 +36432,9 @@ static class E
 """;
         var comp = CreateCompilation(src);
         comp.VerifyEmitDiagnostics(
-            // (2,1): error CS1929: 'object' does not contain a definition for 'M' and the best extension method overload 'E.extension<U>(U).M(object)' requires a receiver of type 'U'
+            // (2,1): error CS1973: 'object' has no applicable method named 'M' but appears to have an extension method by that name. Extension methods cannot be dynamically dispatched. Consider casting the dynamic arguments or calling the extension method without the extension method syntax.
             // object.M(d);
-            Diagnostic(ErrorCode.ERR_BadInstanceArgType, "object").WithArguments("object", "M", "E.extension<U>(U).M(object)", "U").WithLocation(2, 1),
+            Diagnostic(ErrorCode.ERR_BadArgTypeDynamicExtension, "object.M(d)").WithArguments("object", "M").WithLocation(2, 1),
             // (3,1): error CS1973: 'object' has no applicable method named 'M2' but appears to have an extension method by that name. Extension methods cannot be dynamically dispatched. Consider casting the dynamic arguments or calling the extension method without the extension method syntax.
             // new object().M2(d);
             Diagnostic(ErrorCode.ERR_BadArgTypeDynamicExtension, "new object().M2(d)").WithArguments("object", "M2").WithLocation(3, 1));
@@ -48528,7 +48528,7 @@ static class E
 {
     extension<T>(MyCollection<T> c)
     {
-        public void Add(T o) { }
+        public void Add(T o) { System.Console.Write(o is null ? "True " : "False "); }
     }
 }
 
@@ -48538,15 +48538,31 @@ public class MyCollection<T> : IEnumerable<T>
     IEnumerator IEnumerable.GetEnumerator() => throw null!;
 }
 """;
-        // https://github.com/dotnet/roslyn/issues/78960
         var comp = CreateCompilation(src);
-        comp.VerifyEmitDiagnostics(
-                // (7,26): error CS9215: Collection expression type 'MyCollection<object>' must have an instance or extension method 'Add' that can be called with a single argument.
-                // MyCollection<object> c = [oNull, oNotNull];
-                Diagnostic(ErrorCode.ERR_CollectionExpressionMissingAdd, "[oNull, oNotNull]").WithArguments("MyCollection<object>").WithLocation(7, 26),
-                // (7,26): error CS1929: 'MyCollection<object>' does not contain a definition for 'Add' and the best extension method overload 'E.extension<T>(MyCollection<T>).Add(T)' requires a receiver of type 'MyCollection<T>'
-                // MyCollection<object> c = [oNull, oNotNull];
-                Diagnostic(ErrorCode.ERR_BadInstanceArgType, "[oNull, oNotNull]").WithArguments("MyCollection<object>", "Add", "E.extension<T>(MyCollection<T>).Add(T)", "MyCollection<T>").WithLocation(7, 26));
+        CompileAndVerify(comp, expectedOutput: "True False").VerifyDiagnostics();
+
+        src = """
+#nullable enable
+using System.Collections;
+using System.Collections.Generic;
+
+object? oNull = null;
+object oNotNull = new object();
+MyCollection<object> c = [oNull, oNotNull];
+
+static class E
+{
+    public static void Add<T>(this MyCollection<T> c, T o) { System.Console.Write(o is null ? "True " : "False ");  }
+}
+
+public class MyCollection<T> : IEnumerable<T>
+{
+    IEnumerator<T> IEnumerable<T>.GetEnumerator() => throw null!;
+    IEnumerator IEnumerable.GetEnumerator() => throw null!;
+}
+""";
+        comp = CreateCompilation(src);
+        CompileAndVerify(comp, expectedOutput: "True False").VerifyDiagnostics();
     }
 
     [Fact]