Implement improved overload candidates, aka "Bestest Betterness". #24756
Conversation
- 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).
|
@dotnet/roslyn-compiler May I please have a couple of reviews for this "small" language feature implementation? #Resolved |
gafter
added
Area-Compilers
4 - In Review
| result.ReportDiagnostics( | ||
| binder: this, location: errorLocation, nodeOpt: null, diagnostics: diagnostics, | ||
| name: errorName, receiver: null, invokedExpression: null, arguments: analyzedArguments, | ||
| memberGroup: candidateConstructors, typeContainingConstructor: typeContainingConstructors, delegateTypeBeingInvoked: null); |
There was a problem hiding this comment.
Nit: you could skip some of the argument names (diagnostics, arguments and typeContainingConstructor).
That is also applicable elsewhere (like OverloadResolution.cs) where I'm not sure adding the name clarified the code. #Closed
| bool isMethodGroupConversion, | ||
| RefKind returnRefKind = default, | ||
| TypeSymbol returnType = null | ||
| ) |
There was a problem hiding this comment.
Nit: formatting on paren #Closed
| @@ -6866,9 +6883,13 @@ private ErrorPropertySymbol CreateErrorPropertySymbol(ImmutableArray<PropertySym | |||
| AnalyzedArguments analyzedArguments, | |||
| bool isMethodGroupConversion, | |||
| ref HashSet<DiagnosticInfo> useSiteDiagnostics, | |||
| bool inferWithDynamic = false) | |||
| bool inferWithDynamic = false, | |||
| RefKind returnRefKind = default, | |||
There was a problem hiding this comment.
I assume you reviewed all the callers. Optional parameters make me nervous, as the PR will not highlight all the call locations which need reviewing.
If most callers had to be updated anyways, consider making the parameters non-optional. #Resolved
There was a problem hiding this comment.
I will add comments. These defaults are correct for almost all callers. They are only used when isMethodGroupConversion is true and we are not inferring the return type.
In reply to: 167680751 [](ancestors = 167680751)
| @@ -14,6 +17,7 @@ internal struct MemberAnalysisResult | |||
| public readonly ImmutableArray<Conversion> ConversionsOpt; | |||
| public readonly ImmutableArray<int> BadArgumentsOpt; | |||
| public readonly ImmutableArray<int> ArgsToParamsOpt; | |||
| public readonly ArrayBuilder<TypeParameterDiagnosticInfo> ConstraintFailureDiagnosticsOpt; | |||
There was a problem hiding this comment.
I'd be tempted to make ConstraintFailureDiagnosticsOpt immutable like the other members. #Closed
| @@ -25,24 +29,26 @@ internal struct MemberAnalysisResult | |||
| public readonly bool HasAnyRefOmittedArgument; | |||
|
|
|||
| private MemberAnalysisResult(MemberResolutionKind kind) | |||
| : this(kind, default(ImmutableArray<int>), default(ImmutableArray<int>), default(ImmutableArray<Conversion>)) | |||
| : this(kind, constraintFailureDiagnosticsOpt: default) | |||
There was a problem hiding this comment.
Not sure why constraintFailureDiagnosticsOpt is passed explicitly. Seems like this constructor overload can be removed. #Resolved
| @@ -1390,6 +1390,25 @@ public static void M() | |||
| // (58,9): error CS0453: The type 'string' must be a non-nullable value type in order to use it as parameter 'S' in the generic type or method 'C.L<S>' | |||
| // Test6<L<string>>(null); | |||
| Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "Test6<L<string>>").WithArguments("C.L<S>", "S", "string").WithLocation(58, 9)); | |||
| CreateStandardCompilation(source).VerifyDiagnostics( | |||
There was a problem hiding this comment.
Nit: an empty line would help separate the blocks #Closed
| // (23,38): error CS0121: The call is ambiguous between the following methods or properties: 'Program.Bar<T, V>.Create(Func<T, bool>, params int[])' and 'Program.Bar<T, V>.Create(Func<T, V>, params int[])' | ||
| // var x = Bar<Goo, double>.Create(Goo.IsThing); | ||
| Diagnostic(ErrorCode.ERR_AmbigCall, "Create").WithArguments("ConsoleApplication2.Program.Bar<T, V>.Create(System.Func<T, bool>, params int[])", "ConsoleApplication2.Program.Bar<T, V>.Create(System.Func<T, V>, params int[])").WithLocation(23, 38) | ||
| ); | ||
| CreateStandardCompilation(source1, options: TestOptions.DebugExe).VerifyDiagnostics( |
There was a problem hiding this comment.
This is surprising. Is it intended? Update: I understand why this should now work.
We should verify the semantic model (which overload was picked) and execution.
Update2: I see that you covered those in the new test file.
#Closed
| // We are in a context where only instance (or only static) methods are permitted. We reject the others. | ||
| bool keepStatic = isImplicitReceiver && isStaticContext || Binder.IsMemberAccessedThroughType(receiverOpt); | ||
|
|
||
| for (int f = 0; f < results.Count; ++f) |
There was a problem hiding this comment.
Nit: Was i already used? ;-P #Resolved
There was a problem hiding this comment.
This is copy-pasta from most of this source file's code to do this kind of filtering.
In reply to: 167693606 [](ancestors = 167693606)
| // violate the constraints of the method's type parameters. | ||
|
|
||
| // Constraint violations apply to method in a method group, not to properties in a "property group". | ||
| if (typeof(TMember) != typeof(MethodSymbol)) |
There was a problem hiding this comment.
Is there some other trick to avoid this?
For instance, what if we have a RemoveConstraintViolations<TMember> (which does nothing) and a RemoveConstraintViolations<MethodSymbol>. Would the right one get picked in the right cases? #Closed
There was a problem hiding this comment.
No, C# doesn't have source-level "template specialization". This should get compiled away by the runtime compiler just as if we had written what you suggested.
In reply to: 167694965 [](ancestors = 167694965)
| for (int f = 0; f < results.Count; ++f) | ||
| { | ||
| var result = results[f]; | ||
| var member = (MethodSymbol)(Symbol)result.Member; |
There was a problem hiding this comment.
Symbol [](start = 44, length = 6)
One cast can be removed. Same comment applies in a few other locations. #Closed
There was a problem hiding this comment.
Not true. Both casts are needed. There is no explicit conversion from TSymbol to MethodSymbol.
In reply to: 167695275 [](ancestors = 167695275)
| } | ||
|
|
||
| var diagnosticsBuilder = ArrayBuilder<TypeParameterDiagnosticInfo>.GetInstance(); | ||
| ArrayBuilder <TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder = null; |
There was a problem hiding this comment.
Nit: formatting on type #Resolved
|
|
||
| var diagnosticsBuilder = ArrayBuilder<TypeParameterDiagnosticInfo>.GetInstance(); | ||
| ArrayBuilder <TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder = null; | ||
| var constraintsSatisfied = ConstraintsHelper.CheckMethodConstraints( |
| } | ||
|
|
||
| diagnosticsBuilder.Free(); | ||
| useSiteDiagnosticsBuilder?.Free(); |
There was a problem hiding this comment.
Is it possible for useSiteDiagnosticsBuilder to be set here? #Closed
There was a problem hiding this comment.
| var method = (MethodSymbol)(Symbol)result.Member; | ||
| bool returnsMatch = returnType == null || | ||
| method.ReturnType.Equals(returnType, TypeCompareKind.AllIgnoreOptions) || | ||
| returnRefKind == RefKind.None && Conversions.HasIdentityOrImplicitReferenceConversion(method.ReturnType, returnType, ref useSiteDiagnostics); |
There was a problem hiding this comment.
Nit: consider regular indentation, which will help with long line. #Closed
| } | ||
|
|
||
| var method = (MethodSymbol)(Symbol)result.Member; | ||
| bool returnsMatch = returnType == null || |
|
|
||
| // If there is any such method that has a bad conversion or out/ref mismatch | ||
| // Otherwise, f there is any such method that has a bad conversion or out/ref mismatch |
There was a problem hiding this comment.
f [](start = 26, length = 1)
typo: if #Closed
| @@ -85,15 +85,15 @@ public static void AddToMap(BoundNode root, Dictionary<SyntaxNode, ImmutableArra | |||
| Debug.Assert( | |||
| ((BoundTypeExpression)existing[i]).Type == ((BoundTypeOrValueExpression)added[i]).Type, | |||
| string.Format( | |||
| CultureInfo.InvariantCulture, | |||
| System.Globalization.CultureInfo.InvariantCulture, | |||
There was a problem hiding this comment.
What prompted this change? #Resolved
There was a problem hiding this comment.
Uncommenting out this code and finding that it doesn't compile.
In reply to: 167697572 [](ancestors = 167697572)
| @@ -347,7 +347,7 @@ internal bool BindingTopLevelScriptCode | |||
| get | |||
| { | |||
| var containingMember = this.ContainingMemberOrLambda; | |||
| switch (containingMember.Kind) | |||
| switch (containingMember?.Kind) | |||
There was a problem hiding this comment.
I didn't understand what prompted this change. #Resolved
There was a problem hiding this comment.
This is now used in error scenarios where containingMember can be null.
In reply to: 167699262 [](ancestors = 167699262)
jcouv
changed the title
| { | ||
| this.Kind = kind; | ||
| this.BadArgumentsOpt = badArgumentsOpt; | ||
| this.ArgsToParamsOpt = argsToParamsOpt; | ||
| this.ConversionsOpt = conversionsOpt; | ||
| this.BadParameter = missingParameter; | ||
| this.HasAnyRefOmittedArgument = hasAnyRefOmittedArgument; | ||
| this.ConstraintFailureDiagnosticsOpt = constraintFailureDiagnosticsOpt.NullToEmpty(); |
There was a problem hiding this comment.
ConstraintFailureDiagnosticsOpt [](start = 17, length = 31)
ConstraintFailureDiagnosticsOpt shouldn't be "Opt" if we always at least put an empty array there. #Resolved
|
@dotnet/roslyn-compiler May I please have a second review of this small language feature implementation? #Resolved |
2 similar comments
|
@dotnet/roslyn-compiler May I please have a second review of this small language feature implementation? #Resolved |
|
@dotnet/roslyn-compiler May I please have a second review of this small language feature implementation? #Resolved |
| //When a method group contains some generic methods whose type parameters do not satisfy their constraints, these members are removed from the candidate set. | ||
| // Test that this permits overload resolution to use type parameter constraints "as a tie-breaker" to guide overload resolution. | ||
| [Fact] | ||
| public void TestConstraintFailed02() |
There was a problem hiding this comment.
I think we need couple more tests (i think they will pass, but in case of future regressions):
- one candidate is generic, but candidate fails constraints, while another overload requires a conversion. Used to be an error, second should be picked now.
- one candidate is generic without constraints, but we pass a ref-struct to it, which cannot be a generic type arg, another candidate requires a conversion and now works.
#Resolved
There was a problem hiding this comment.
|
@gafter Could you update the feature status page, since this feature is now merged? |
|
@jcouv On what branch? 😄 |
|
We only maintain the feature status page on |
* features/compiler: (118 commits) Fix up test failures Post merge cleanup SubstituteNoPiaLocalType - use comparison of SpecialTypes to match bases across distinct core libraries. (dotnet#24965) Respond to PR feedback PR feedback PR Feedback Fix tests Standardized on the doc mcomments Unified most helpers now Standardize the IL helpers Unify the WinRT overloads Unify the CompileAndVerify Unify overloads Fix a name Implement improved overload candidates, aka "Bestest Betterness". (dotnet#24756) Include PDB Checksum in Debug Directory (dotnet#24711) Disable multicore jitting More renames Fixed up the unit tests Fix a compilation issue ...
|
/cc @cston |
Implements dotnet/csharplang#98
Also fixes #24675