Extensions: address some diagnostic quality issues

Author: Julien Couvreur

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

@@ -8688,9 +8688,9 @@ static bool tryResolveExtensionInScope(
                     {
                         if (propertyResult != null)
                         {
+                            firstResult = makeErrorResult(methodResult, propertyResult, expression, left, memberName, arity, lookupResult, analyzedArguments, ref actualMethodArguments, binder, diagnostics);
                             methodResult.Free(keepArguments: true);
                             propertyResult.Free();
-                            firstResult = makeErrorResult(left.Type, memberName, arity, lookupResult, expression, diagnostics);
                         }
                         else
                         {
@@ -8714,28 +8714,28 @@ static bool tryResolveExtensionInScope(
                     }
 
                     // ambiguous between methods and properties
+                    result = makeErrorResult(methodResult, propertyResult, expression, left, memberName, arity, lookupResult, analyzedArguments, ref actualMethodArguments, binder, diagnostics);
                     methodResult.Free(keepArguments: true);
                     propertyResult?.Free();
-                    result = makeErrorResult(left.Type, memberName, arity, lookupResult, expression, diagnostics);
                     return true;
                 }
 
                 // If the search in the current scope resulted in any applicable property (regardless of whether a best
                 // applicable property could be determined) then our search is complete.
                 Debug.Assert(propertyResult?.HasAnyApplicableMember == true);
-                methodResult.Free(keepArguments: true);
                 if (propertyResult.Succeeded && propertyResult.BestResult.Member is { } bestProperty)
                 {
                     // property wins
+                    methodResult.Free(keepArguments: true);
                     propertyResult.Free();
                     result = new MethodGroupResolution(bestProperty, LookupResultKind.Viable, diagnostics.ToReadOnly());
                     return true;
                 }
 
                 // ambiguous between multiple applicable properties
+                result = makeErrorResult(methodResult, propertyResult, expression, left, memberName, arity, lookupResult, analyzedArguments, ref actualMethodArguments, binder, diagnostics);
+                methodResult.Free(keepArguments: true);
                 propertyResult.Free();
-                // Tracked by https://github.com/dotnet/roslyn/issues/78830 : diagnostic quality, consider using the property overload resolution result in the result to improve reported diagnostics
-                result = makeErrorResult(left.Type, memberName, arity, lookupResult, expression, diagnostics);
                 return true;
             }
 
@@ -8796,12 +8796,7 @@ static MethodGroupResolution resolveMethods(
                     return default;
                 }
 
-                if (actualMethodArguments == null)
-                {
-                    // Create a set of arguments for overload resolution including the receiver.
-                    actualMethodArguments = AnalyzedArguments.GetInstance();
-                    CombineExtensionMethodArguments(left, analyzedArguments, actualMethodArguments);
-                }
+                initActualArguments(left, analyzedArguments, ref actualMethodArguments);
 
                 var overloadResolutionResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
                 CompoundUseSiteInfo<AssemblySymbol> overloadResolutionUseSiteInfo = binder.GetNewCompoundUseSiteInfo(diagnostics);
@@ -8862,14 +8857,54 @@ static MethodGroupResolution resolveMethods(
                 return overloadResolutionResult;
             }
 
-            static MethodGroupResolution makeErrorResult(TypeSymbol receiverType, string memberName, int arity, LookupResult lookupResult, SyntaxNode expression, BindingDiagnosticBag diagnostics)
+            static MethodGroupResolution makeErrorResult(
+                MethodGroupResolution methodResult,
+                OverloadResolutionResult<PropertySymbol> propertyResult,
+                SyntaxNode expression,
+                BoundExpression left,
+                string memberName,
+                int arity,
+                LookupResult lookupResult,
+                AnalyzedArguments? analyzedArguments,
+                ref AnalyzedArguments? actualMethodArguments,
+                Binder binder,
+                BindingDiagnosticBag diagnostics)
             {
-                // Tracked by https://github.com/dotnet/roslyn/issues/78830 : diagnostic quality, we'll want to describe what went wrong in a useful way (see OverloadResolutionResult.ReportDiagnostics)
-                var errorInfo = new CSDiagnosticInfo(ErrorCode.ERR_ExtensionResolutionFailed, receiverType, memberName);
-                diagnostics.Add(errorInfo, expression.Location);
-                var resultSymbol = new ExtendedErrorTypeSymbol(containingSymbol: null, lookupResult.Symbols.ToImmutable(), LookupResultKind.OverloadResolutionFailure, errorInfo, arity);
+                Debug.Assert(propertyResult is not null);
+                ImmutableArray<Symbol> symbols = lookupResult.Symbols.ToImmutable();
+
+                DiagnosticInfo errorInfo;
+                if (methodResult.HasAnyApplicableMethod && propertyResult.HasAnyApplicableMember)
+                {
+                    var firstMethod = methodResult.OverloadResolutionResult?.BestResult.Member ?? methodResult.MethodGroup.Methods[0];
+                    var firstProperty = propertyResult.BestResult.Member;
+                    errorInfo = OverloadResolutionResult<Symbol>.CreateAmbiguousCallDiagnosticInfo(binder.Compilation, firstMethod, firstProperty, symbols, isExtension: true);
+
+                    diagnostics.Add(errorInfo, expression.Location);
+                }
+                else
+                {
+                    initActualArguments(left, analyzedArguments, ref actualMethodArguments);
+
+                    propertyResult.ReportDiagnostics(binder, expression.Location, expression, diagnostics, memberName, left, left.Syntax, actualMethodArguments, symbols,
+                        typeContainingConstructor: null, delegateTypeBeingInvoked: null, isMethodGroupConversion: false, isExtension: true);
+
+                    errorInfo = new CSDiagnosticInfo(ErrorCode.ERR_ExtensionResolutionFailed, left.Type, memberName);
+                }
+
+                ExtendedErrorTypeSymbol resultSymbol = new ExtendedErrorTypeSymbol(containingSymbol: null, symbols, LookupResultKind.OverloadResolutionFailure, errorInfo, arity);
                 return new MethodGroupResolution(resultSymbol, LookupResultKind.Viable, diagnostics.ToReadOnly());
             }
+
+            static void initActualArguments(BoundExpression left, AnalyzedArguments? analyzedArguments, [NotNull] ref AnalyzedArguments? actualMethodArguments)
+            {
+                if (actualMethodArguments == null)
+                {
+                    // Create a set of arguments for overload resolution including the receiver.
+                    actualMethodArguments = AnalyzedArguments.GetInstance();
+                    CombineExtensionMethodArguments(left, analyzedArguments, actualMethodArguments);
+                }
+            }
         }
 
         private bool AllowRefOmittedArguments(BoundExpression receiver)

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

@@ -201,7 +201,8 @@ internal void ReportDiagnostics<T>(
             bool isMethodGroupConversion = false,
             RefKind? returnRefKind = null,
             TypeSymbol delegateOrFunctionPointerType = null,
-            bool isParamsModifierValidation = false) where T : Symbol
+            bool isParamsModifierValidation = false,
+            bool isExtension = false) where T : Symbol
         {
             Debug.Assert(!this.Succeeded, "Don't ask for diagnostic info on a successful overload resolution result.");
 
@@ -220,7 +221,7 @@ internal void ReportDiagnostics<T>(
             // however, be more than one.  We'll check for that first, since applicable candidates are
             // always better than inapplicable candidates.
 
-            if (HadAmbiguousBestMethods(binder.Compilation, diagnostics, symbols, location))
+            if (HadAmbiguousBestMethods(binder.Compilation, diagnostics, symbols, location, isExtension))
             {
                 return;
             }
@@ -243,7 +244,7 @@ internal void ReportDiagnostics<T>(
             // (Obviously, there can't be a LessDerived cycle, since we break type hierarchy cycles during
             // symbol table construction.)
 
-            if (HadAmbiguousWorseMethods(binder.Compilation, diagnostics, symbols, location, queryClause != null, receiver, name))
+            if (HadAmbiguousWorseMethods(binder.Compilation, diagnostics, symbols, location, queryClause != null, receiver, name, isExtension))
             {
                 return;
             }
@@ -774,10 +775,22 @@ private bool TypeInferenceFailed(
                 }
                 else
                 {
-                    diagnostics.Add(new DiagnosticInfoWithSymbols(
-                        ErrorCode.ERR_NoSuchMemberOrExtension,
-                        new object[] { instanceArgument.Type, inferenceFailed.Member.Name },
-                        symbols), location);
+                    if (inferenceFailed.Member.Kind == SymbolKind.Method)
+                    {
+                        // error CS0411: The type arguments for method 'M<T>(T)' cannot be inferred
+                        // from the usage. Try specifying the type arguments explicitly.
+                        diagnostics.Add(new DiagnosticInfoWithSymbols(
+                            ErrorCode.ERR_CantInferMethTypeArgs,
+                            new object[] { inferenceFailed.Member },
+                            symbols), location);
+                    }
+                    else
+                    {
+                        diagnostics.Add(new DiagnosticInfoWithSymbols(
+                            ErrorCode.ERR_NoSuchMemberOrExtension,
+                            new object[] { instanceArgument.Type, inferenceFailed.Member.Name },
+                            symbols), location);
+                    }
                 }
 
                 return true;
@@ -1388,7 +1401,7 @@ static Symbol unwrapIfParamsCollection(MemberResolutionResult<TMember> badArg, P
             }
         }
 
-        private bool HadAmbiguousWorseMethods(CSharpCompilation compilation, BindingDiagnosticBag diagnostics, ImmutableArray<Symbol> symbols, Location location, bool isQuery, BoundExpression receiver, string name)
+        private bool HadAmbiguousWorseMethods(CSharpCompilation compilation, BindingDiagnosticBag diagnostics, ImmutableArray<Symbol> symbols, Location location, bool isQuery, BoundExpression receiver, string name, bool isExtension)
         {
             MemberResolutionResult<TMember> worseResult1;
             MemberResolutionResult<TMember> worseResult2;
@@ -1417,7 +1430,8 @@ private bool HadAmbiguousWorseMethods(CSharpCompilation compilation, BindingDiag
                         compilation,
                         worseResult1.LeastOverriddenMember.ConstructedFrom(),
                         worseResult2.LeastOverriddenMember.ConstructedFrom(),
-                        symbols),
+                        symbols,
+                        isExtension),
                     location);
             }
 
@@ -1453,7 +1467,7 @@ private int TryGetFirstTwoWorseResults(out MemberResolutionResult<TMember> first
             return count;
         }
 
-        private bool HadAmbiguousBestMethods(CSharpCompilation compilation, BindingDiagnosticBag diagnostics, ImmutableArray<Symbol> symbols, Location location)
+        private bool HadAmbiguousBestMethods(CSharpCompilation compilation, BindingDiagnosticBag diagnostics, ImmutableArray<Symbol> symbols, Location location, bool isExtension)
         {
             MemberResolutionResult<TMember> validResult1;
             MemberResolutionResult<TMember> validResult2;
@@ -1473,7 +1487,8 @@ private bool HadAmbiguousBestMethods(CSharpCompilation compilation, BindingDiagn
                     compilation,
                     validResult1.LeastOverriddenMember.ConstructedFrom(),
                     validResult2.LeastOverriddenMember.ConstructedFrom(),
-                    symbols),
+                    symbols,
+                    isExtension),
                 location);
 
             return true;
@@ -1508,10 +1523,13 @@ private int TryGetFirstTwoValidResults(out MemberResolutionResult<TMember> first
             return count;
         }
 
-        private static DiagnosticInfoWithSymbols CreateAmbiguousCallDiagnosticInfo(CSharpCompilation compilation, Symbol first, Symbol second, ImmutableArray<Symbol> symbols)
+        internal static DiagnosticInfoWithSymbols CreateAmbiguousCallDiagnosticInfo(CSharpCompilation compilation, Symbol first, Symbol second, ImmutableArray<Symbol> symbols, bool isExtension)
         {
+            // error: The extension resolution is ambiguous between the following members: 'first' and 'second'
+            // OR
+            // error: The call is ambiguous between the following methods or properties: 'first' and 'second'
             var distinguisher = new SymbolDistinguisher(compilation, first, second);
-            return new DiagnosticInfoWithSymbols(ErrorCode.ERR_AmbigCall, [distinguisher.First, distinguisher.Second], symbols);
+            return new DiagnosticInfoWithSymbols(isExtension ? ErrorCode.ERR_AmbigExtension : ErrorCode.ERR_AmbigCall, [distinguisher.First, distinguisher.Second], symbols);
         }
 
         [Conditional("DEBUG")]

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -8168,4 +8168,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_BadVisBaseType" xml:space="preserve">
     <value>Inconsistent accessibility: type '{1}' is less accessible than class '{0}' </value>
   </data>
+  <data name="ERR_AmbigExtension" xml:space="preserve">
+    <value>The extension resolution is ambiguous between the following members: '{0}' and '{1}'</value>
+  </data>
 </root>

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -2435,6 +2435,7 @@ internal enum ErrorCode
         HDN_RedundantPatternStackGuard = 9337,
 
         ERR_BadVisBaseType = 9338,
+        ERR_AmbigExtension = 9339,
 
         // Note: you will need to do the following after adding errors:
         //  1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

@@ -2543,6 +2543,7 @@ or ErrorCode.HDN_RedundantPattern
                 or ErrorCode.WRN_RedundantPattern
                 or ErrorCode.HDN_RedundantPatternStackGuard
                 or ErrorCode.ERR_BadVisBaseType
+                or ErrorCode.ERR_AmbigExtension
                     => false,
             };
 #pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -4697,7 +4697,6 @@ private SeparatedSyntaxList<ParameterSyntax> ParseParameterList(
                 allowTrailingSeparator: false,
                 requireOneElement: forExtension, // For extension declarations, we require at least one receiver parameter
                 allowSemicolonAsSeparator: false);
-            // Tracked by https://github.com/dotnet/roslyn/issues/78830 : diagnostic quality, consider suppressing parsing diagnostics on extension parameters beyond the first one
 
             _termState = saveTerm;
             close = this.EatToken(closeKind);

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

@@ -294,7 +294,8 @@ private void VisitNamedTypeWithoutNullability(INamedTypeSymbol symbol)
 
             //visit the enclosing type if the style requires it
             if (Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameAndContainingTypes ||
-                Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces)
+                Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces ||
+                (Format.TypeQualificationStyle == SymbolDisplayTypeQualificationStyle.NameOnly && symbol.IsExtension))
             {
                 if (IncludeNamedType(symbol.ContainingType))
                 {