Extensions: Address follow-ups related to collection expressions

Author: Julien Couvreur

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

@@ -1467,11 +1467,23 @@ static ImmutableArray<MethodSymbol> filterOutBadGenericMethods(
                 {
                     // If the method is generic, skip it if the type arguments cannot be inferred.
                     var member = candidate.Member;
+                    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 +1534,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 +1611,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 +1648,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 +1697,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(parameterContainer: container))
+            {
+                return true;
+            }
+
+            return container.GetIsNewExtensionMember() && parameterType.ContainsTypeParameter(parameterContainer: container.ContainingType);
+        }
+
         private static TMember GetConstructedFrom<TMember>(TMember member) where TMember : Symbol
         {
             switch (member.Kind)

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.Any(static tp => tp is SubstitutedTypeParameterSymbol && tp.ContainingSymbol is not SourceExtensionImplementationMethodSymbol));
         }
 
         // 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,7 +1205,7 @@ 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 parameterContainer)
         {
             RoslynDebug.Assert((object)parameterContainer != null);
 

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

@@ -1722,6 +1722,7 @@ public void LanguageVersionAdded_Canary()
             // - [ ] replace all references to C# "Next" (such as `TestOptions.RegularNext` or `LanguageVersionFacts.CSharpNext`) with the new version and fix failing tests
             // - [ ] update _MaxAvailableLangVersion cap (a relevant test should break when new version is introduced)
             // - [ ] update the "UpgradeProject" codefixer
+            // - [ ] test VS insertion
             //
             // Other repos also need updates:
             // - [ ] email release management to add to the release notes. See csharp-version in release.json in previous example: https://github.com/dotnet/core/pull/9493

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));
@@ -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));
+        comp.VerifyEmitDiagnostics();
+
+        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) { }
+}
+
+public class MyCollection<T> : IEnumerable<T>
+{
+    IEnumerator<T> IEnumerable<T>.GetEnumerator() => throw null!;
+    IEnumerator IEnumerable.GetEnumerator() => throw null!;
+}
+""";
+        comp = CreateCompilation(src);
+        comp.VerifyEmitDiagnostics();
     }
 
     [Fact]