Implement improved overload candidates, aka "Bestest Betternes".

- Refine candidate set based on static/instance receiver, or static context
- Refine candidate methods whose type parameter constraints are violated.
- For a method group conversion, remove methods with the wrong return type or ref mode.

Implements dotnet/csharplang#98

See also dotnet#24675 for a proposal to improve the quality of diagnostics for the method group conversion. This change occasionally exposes this opportunity for diagnostic improvement (search for references to dotnet#24675 in the source to find examples).

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

@@ -259,6 +259,18 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind
  return ToBadExpression(expr, resultKind);
  }
 
+ internal static bool IsTypeOrValueExpression(BoundExpression expression)
+ {
+ switch (expression?.Kind)
+ {
+ case BoundKind.TypeOrValueExpression:
+ case BoundKind.QueryClause when ((BoundQueryClause)expression).Value.Kind == BoundKind.TypeOrValueExpression:
+ return true;
+ default:
+ return false;
+ }
+ }
+
  /// <summary>
  /// The purpose of this method is to determine if the expression satisfies desired capabilities. 
  /// If it is not then this code gives an appropriate error message.

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

@@ -347,7 +347,7 @@ internal bool BindingTopLevelScriptCode
  get
  {
  var containingMember = this.ContainingMemberOrLambda;
- switch (containingMember.Kind)
+ switch (containingMember?.Kind)
  {
  case SymbolKind.Method:
  // global statements

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

@@ -455,8 +455,7 @@ private static bool IsMethodGroupWithTypeOrValueReceiver(BoundNode node)
  return false;
  }
 
- BoundNode receiverOpt = ((BoundMethodGroup)node).ReceiverOpt;
- return receiverOpt != null && receiverOpt.Kind == BoundKind.TypeOrValueExpression;
+ return Binder.IsTypeOrValueExpression(((BoundMethodGroup)node).ReceiverOpt);
  }
 
  private BoundMethodGroup FixMethodGroupWithTypeOrValue(BoundMethodGroup group, Conversion conversion, DiagnosticBag diagnostics)
@@ -572,7 +571,7 @@ private bool MemberGroupFinalValidationAccessibilityChecks(BoundExpression recei
  //note that the same assert does not hold for all properties. Some properties and (all indexers) are not referenceable by name, yet
  //their binding brings them through here, perhaps needlessly.
 
- if (receiverOpt != null && receiverOpt.Kind == BoundKind.TypeOrValueExpression)
+ if (IsTypeOrValueExpression(receiverOpt))
  {
  // TypeOrValue expression isn't replaced only if the invocation is late bound, in which case it can't be extension method.
  // None of the checks below apply if the receiver can't be classified as a type or value. 
@@ -678,7 +677,7 @@ private static bool IsMemberAccessedThroughVariableOrValue(BoundExpression recei
  return !IsMemberAccessedThroughType(receiverOpt);
  }
 
- private static bool IsMemberAccessedThroughType(BoundExpression receiverOpt)
+ internal static bool IsMemberAccessedThroughType(BoundExpression receiverOpt)
  {
  if (receiverOpt == null)
  {
@@ -696,7 +695,7 @@ private static bool IsMemberAccessedThroughType(BoundExpression receiverOpt)
  /// <summary>
  /// Was the receiver expression compiler-generated?
  /// </summary>
- private static bool WasImplicitReceiver(BoundExpression receiverOpt)
+ internal static bool WasImplicitReceiver(BoundExpression receiverOpt)
  {
  if (receiverOpt == null) return true;
  if (!receiverOpt.WasCompilerGenerated) return false;

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

@@ -26,12 +26,12 @@ internal partial class Binder
  /// <param name="isExplicit">The reference was explicitly specified in syntax.</param>
  /// <param name="inStaticContext">True if "this" is not available due to the current method/property/field initializer being static.</param>
  /// <returns>True if a reference to "this" is available.</returns>
- private bool HasThis(bool isExplicit, out bool inStaticContext)
+ internal bool HasThis(bool isExplicit, out bool inStaticContext)
  {
- var member = this.ContainingMemberOrLambda.ContainingNonLambdaMember();
- if (member.IsStatic)
+ var memberOpt = this.ContainingMemberOrLambda?.ContainingNonLambdaMember();
+ if (memberOpt?.IsStatic == true)
  {
- inStaticContext = member.Kind == SymbolKind.Field || member.Kind == SymbolKind.Method || member.Kind == SymbolKind.Property;
+ inStaticContext = memberOpt.Kind == SymbolKind.Field || memberOpt.Kind == SymbolKind.Method || memberOpt.Kind == SymbolKind.Property;
  return false;
  }
 
@@ -42,7 +42,7 @@ private bool HasThis(bool isExplicit, out bool inStaticContext)
  return false;
  }
 
- var containingType = member.ContainingType;
+ var containingType = memberOpt?.ContainingType;
  bool inTopLevelScriptMember = (object)containingType != null && containingType.IsScriptClass;
 
  // "this" is not allowed in field initializers (that are not script variable initializers):
@@ -4860,8 +4860,10 @@ private bool TryPerformConstructorOverloadResolution(
  }
  else
  {
- result.ReportDiagnostics(this, errorLocation, diagnostics,
- errorName, null, analyzedArguments, candidateConstructors, typeContainingConstructors, null);
+ result.ReportDiagnostics(
+ binder: this, location: errorLocation, nodeOpt: null, diagnostics: diagnostics,
+ name: errorName, receiver: null, invokedExpression: null, arguments: analyzedArguments,
+ memberGroup: candidateConstructors, typeContainingConstructor: typeContainingConstructors, delegateTypeBeingInvoked: null);
  }
  }
 
@@ -5823,7 +5825,10 @@ private MethodGroupResolution BindExtensionMethod(
  AnalyzedArguments analyzedArguments,
  BoundExpression left,
  ImmutableArray<TypeSymbol> typeArguments,
- bool isMethodGroupConversion)
+ bool isMethodGroupConversion,
+ RefKind returnRefKind = default,
+ TypeSymbol returnType = null
+)
  {
  var firstResult = new MethodGroupResolution();
  AnalyzedArguments actualArguments = null;
@@ -5872,7 +5877,17 @@ private MethodGroupResolution BindExtensionMethod(
  var overloadResolutionResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
  bool allowRefOmittedArguments = methodGroup.Receiver.IsExpressionOfComImportType();
  HashSet<DiagnosticInfo> useSiteDiagnostics = null;
- OverloadResolution.MethodInvocationOverloadResolution(methodGroup.Methods, methodGroup.TypeArguments, actualArguments, overloadResolutionResult, ref useSiteDiagnostics, isMethodGroupConversion, allowRefOmittedArguments);
+ OverloadResolution.MethodInvocationOverloadResolution(
+ methods: methodGroup.Methods,
+ typeArguments: methodGroup.TypeArguments,
+ receiver: methodGroup.Receiver,
+ arguments: actualArguments,
+ result: overloadResolutionResult,
+ useSiteDiagnostics: ref useSiteDiagnostics,
+ isMethodGroupConversion: isMethodGroupConversion,
+ allowRefOmittedArguments: allowRefOmittedArguments,
+ returnRefKind: returnRefKind,
+ returnType: returnType);
  diagnostics.Add(expression, useSiteDiagnostics);
  var sealedDiagnostics = diagnostics.ToReadOnlyAndFree();
  var result = new MethodGroupResolution(methodGroup, null, overloadResolutionResult, actualArguments, methodGroup.ResultKind, sealedDiagnostics);
@@ -6737,7 +6752,7 @@ private BoundExpression BindIndexerOrIndexedPropertyAccess(
  OverloadResolutionResult<PropertySymbol> overloadResolutionResult = OverloadResolutionResult<PropertySymbol>.GetInstance();
  bool allowRefOmittedArguments = receiverOpt.IsExpressionOfComImportType();
  HashSet<DiagnosticInfo> useSiteDiagnostics = null;
- this.OverloadResolution.PropertyOverloadResolution(propertyGroup, analyzedArguments, overloadResolutionResult, allowRefOmittedArguments, ref useSiteDiagnostics);
+ this.OverloadResolution.PropertyOverloadResolution(propertyGroup, receiverOpt, analyzedArguments, overloadResolutionResult, allowRefOmittedArguments, ref useSiteDiagnostics);
  diagnostics.Add(syntax, useSiteDiagnostics);
  BoundExpression propertyAccess;
 
@@ -6768,13 +6783,15 @@ private BoundExpression BindIndexerOrIndexedPropertyAccess(
  var name = candidate.IsIndexer ? SyntaxFacts.GetText(SyntaxKind.ThisKeyword) : candidate.Name;
 
  overloadResolutionResult.ReportDiagnostics(
- this,
- syntax.Location,
- diagnostics,
- name,
- null,
- analyzedArguments,
- candidates,
+ binder: this,
+ location: syntax.Location,
+ nodeOpt: syntax,
+ diagnostics: diagnostics,
+ name: name,
+ receiver: null,
+ invokedExpression: null,
+ arguments: analyzedArguments,
+ memberGroup: candidates,
  typeContainingConstructor: null,
  delegateTypeBeingInvoked: null);
  }
@@ -6866,9 +6883,13 @@ internal MethodGroupResolution ResolveMethodGroup(
  AnalyzedArguments analyzedArguments,
  bool isMethodGroupConversion,
  ref HashSet<DiagnosticInfo> useSiteDiagnostics,
- bool inferWithDynamic = false)
+ bool inferWithDynamic = false,
+ RefKind returnRefKind = default,
+ TypeSymbol returnType = null)
  {
- return ResolveMethodGroup(node, node.Syntax, node.Name, analyzedArguments, isMethodGroupConversion, ref useSiteDiagnostics, inferWithDynamic: inferWithDynamic);
+ return ResolveMethodGroup(
+ node, node.Syntax, node.Name, analyzedArguments, isMethodGroupConversion, ref useSiteDiagnostics,
+ inferWithDynamic: inferWithDynamic, returnRefKind: returnRefKind, returnType: returnType);
  }
 
  internal MethodGroupResolution ResolveMethodGroup(
@@ -6879,11 +6900,14 @@ internal MethodGroupResolution ResolveMethodGroup(
  bool isMethodGroupConversion,
  ref HashSet<DiagnosticInfo> useSiteDiagnostics,
  bool inferWithDynamic = false,
- bool allowUnexpandedForm = true)
+ bool allowUnexpandedForm = true,
+ RefKind returnRefKind = default,
+ TypeSymbol returnType = null)
  {
  var methodResolution = ResolveMethodGroupInternal(
  node, expression, methodName, analyzedArguments, isMethodGroupConversion, ref useSiteDiagnostics,
- inferWithDynamic: inferWithDynamic, allowUnexpandedForm: allowUnexpandedForm);
+ inferWithDynamic: inferWithDynamic, allowUnexpandedForm: allowUnexpandedForm,
+ returnRefKind: returnRefKind, returnType: returnType);
  if (methodResolution.IsEmpty && !methodResolution.HasAnyErrors)
  {
  Debug.Assert(node.LookupError == null);
@@ -6904,11 +6928,14 @@ private MethodGroupResolution ResolveMethodGroupInternal(
  bool isMethodGroupConversion,
  ref HashSet<DiagnosticInfo> useSiteDiagnostics,
  bool inferWithDynamic = false,
- bool allowUnexpandedForm = true)
+ bool allowUnexpandedForm = true,
+ RefKind returnRefKind = default,
+ TypeSymbol returnType = null)
  {
  var methodResolution = ResolveDefaultMethodGroup(
  methodGroup, analyzedArguments, isMethodGroupConversion, ref useSiteDiagnostics,
- inferWithDynamic: inferWithDynamic, allowUnexpandedForm: allowUnexpandedForm);
+ inferWithDynamic: inferWithDynamic, allowUnexpandedForm: allowUnexpandedForm,
+ returnRefKind: returnRefKind, returnType: returnType);
 
  // If the method group's receiver is dynamic then there is no point in looking for extension methods; 
  // it's going to be a dynamic invocation.
@@ -6917,7 +6944,9 @@ private MethodGroupResolution ResolveMethodGroupInternal(
  return methodResolution;
  }
 
- var extensionMethodResolution = BindExtensionMethod(expression, methodName, analyzedArguments, methodGroup.ReceiverOpt, methodGroup.TypeArgumentsOpt, isMethodGroupConversion);
+ var extensionMethodResolution = BindExtensionMethod(
+ expression, methodName, analyzedArguments, methodGroup.ReceiverOpt, methodGroup.TypeArgumentsOpt, isMethodGroupConversion,
+ returnRefKind: returnRefKind, returnType: returnType);
  bool preferExtensionMethodResolution = false;
 
  if (extensionMethodResolution.HasAnyApplicableMethod)
@@ -6969,7 +6998,9 @@ private MethodGroupResolution ResolveDefaultMethodGroup(
  bool isMethodGroupConversion,
  ref HashSet<DiagnosticInfo> useSiteDiagnostics,
  bool inferWithDynamic = false,
- bool allowUnexpandedForm = true)
+ bool allowUnexpandedForm = true,
+ RefKind returnRefKind = default,
+ TypeSymbol returnType = null)
  {
  var methods = node.Methods;
  if (methods.Length == 0)
@@ -7015,9 +7046,18 @@ private MethodGroupResolution ResolveDefaultMethodGroup(
  var result = OverloadResolutionResult<MethodSymbol>.GetInstance();
  bool allowRefOmittedArguments = methodGroup.Receiver.IsExpressionOfComImportType();
  OverloadResolution.MethodInvocationOverloadResolution(
- methodGroup.Methods, methodGroup.TypeArguments, analyzedArguments,
- result, ref useSiteDiagnostics, isMethodGroupConversion, allowRefOmittedArguments,
- inferWithDynamic: inferWithDynamic, allowUnexpandedForm: allowUnexpandedForm);
+ methods: methodGroup.Methods,
+ typeArguments: methodGroup.TypeArguments,
+ receiver: methodGroup.Receiver,
+ arguments: analyzedArguments,
+ result: result,
+ useSiteDiagnostics: ref useSiteDiagnostics,
+ isMethodGroupConversion: isMethodGroupConversion,
+ allowRefOmittedArguments: allowRefOmittedArguments,
+ inferWithDynamic: inferWithDynamic,
+ allowUnexpandedForm: allowUnexpandedForm,
+ returnRefKind: returnRefKind,
+ returnType: returnType);
  return new MethodGroupResolution(methodGroup, null, result, analyzedArguments, methodGroup.ResultKind, sealedDiagnostics);
  }
  }

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

@@ -497,7 +497,13 @@ private BoundExpression BindDelegateInvocation(
  methodGroup.PopulateWithSingleMethod(boundExpression, delegateType.DelegateInvokeMethod);
  var overloadResolutionResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
  HashSet<DiagnosticInfo> useSiteDiagnostics = null;
- OverloadResolution.MethodInvocationOverloadResolution(methodGroup.Methods, methodGroup.TypeArguments, analyzedArguments, overloadResolutionResult, ref useSiteDiagnostics);
+ OverloadResolution.MethodInvocationOverloadResolution(
+ methods: methodGroup.Methods,
+ typeArguments: methodGroup.TypeArguments,
+ receiver: methodGroup.Receiver,
+ arguments: analyzedArguments,
+ result: overloadResolutionResult,
+ useSiteDiagnostics: ref useSiteDiagnostics);
  diagnostics.Add(node, useSiteDiagnostics);
 
  // If overload resolution on the "Invoke" method found an applicable candidate, and one of the arguments
@@ -943,8 +949,9 @@ private BoundCall BindInvocationExpressionContinued(
  {
  // Since there were no argument errors to report, we report an error on the invocation itself.
  string name = (object)delegateTypeOpt == null ? methodName : null;
- result.ReportDiagnostics(this, GetLocationForOverloadResolutionDiagnostic(node, expression), diagnostics, name,
- methodGroup.Receiver, analyzedArguments, methodGroup.Methods.ToImmutable(),
+ result.ReportDiagnostics(
+ binder: this, location: GetLocationForOverloadResolutionDiagnostic(node, expression), nodeOpt: node, diagnostics: diagnostics, name: name,
+ receiver: methodGroup.Receiver, invokedExpression: expression, arguments: analyzedArguments, memberGroup: methodGroup.Methods.ToImmutable(),
  typeContainingConstructor: null, delegateTypeBeingInvoked: delegateTypeOpt,
  queryClause: queryClause);
  }

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

@@ -807,7 +807,15 @@ private MethodSymbol PerformForEachPatternOverloadResolution(TypeSymbol patternT
  OverloadResolutionResult<MethodSymbol> overloadResolutionResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
 
  HashSet<DiagnosticInfo> useSiteDiagnostics = null;
- this.OverloadResolution.MethodInvocationOverloadResolution(candidateMethods, typeArguments, arguments, overloadResolutionResult, ref useSiteDiagnostics);
+ // We create a dummy receiver of the invocation so MethodInvocationOverloadResolution knows it was invoked from an instance, not a type
+ var dummyReceiver = new BoundImplicitReceiver(_syntax.Expression, patternType);
+ this.OverloadResolution.MethodInvocationOverloadResolution(
+ methods: candidateMethods,
+ typeArguments: typeArguments,
+ receiver: dummyReceiver,
+ arguments: arguments,
+ result: overloadResolutionResult,
+ useSiteDiagnostics: ref useSiteDiagnostics);
  diagnostics.Add(_syntax.Expression, useSiteDiagnostics);
 
  MethodSymbol result = null;

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

@@ -238,12 +238,8 @@ protected override void AddLookupSymbolsInfoInSingleBinder(LookupSymbolsInfo res
  internal static bool ReportConflictWithParameter(Symbol parameter, Symbol newSymbol, string name, Location newLocation, DiagnosticBag diagnostics)
  {
  var oldLocation = parameter.Locations[0];
-#if PATTERNS_FIXED
  Debug.Assert(oldLocation != newLocation || oldLocation == Location.None || newLocation.SourceTree?.GetRoot().ContainsDiagnostics == true,
  "same nonempty location refers to different symbols?");
-#else
- if (oldLocation == newLocation) return false;
-#endif
  SymbolKind parameterKind = parameter.Kind;
 
  // Quirk of the way we represent lambda parameters.