Extensions: improve diagnostics for operators#80928
Conversation
ebe80f9 to
928f3b3
Compare
928f3b3 to
7f4d084
Compare
| <value>The extension resolution is ambiguous between the following members: '{0}' and '{1}'</value> | ||
| </data> | ||
| <data name="ERR_SingleInapplicableBinaryOperator" xml:space="preserve"> | ||
| <value>Operator cannot be applied on operands of type '{0}' and '{1}'. The closest inapplicable candidate is '{2}'</value> |
| <value>Operator cannot be applied on operands of type '{0}' and '{1}'. The closest inapplicable candidate is '{2}'</value> | ||
| </data> | ||
| <data name="ERR_SingleInapplicableUnaryOperator" xml:space="preserve"> | ||
| <value>Operator cannot be applied on operand of type '{0}'. The closest inapplicable candidate is '{1}'</value> |
Consider declaring this parameter before all Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:587 in 7f4d084. [](commit_id = 7f4d084, deletion_comment = False) |
| /// </summary> | ||
| private struct OperatorResolutionForReporting | ||
| { | ||
| private object? _nonExtensionResult; |
| /// <summary> | ||
| /// Follows a very simplified version of OverloadResolutionResult.ReportDiagnostics which can be expanded in the future if needed. | ||
| /// </summary> | ||
| internal bool TryReportDiagnostics(SyntaxNode node, BindingDiagnosticBag diagnostics, Binder binder, object leftDisplay, object? rightDisplay) |
| { | ||
| if (res.Signature.Method is null) | ||
| { | ||
| // Skip build-in operators |
| { | ||
| if (res.Signature.Method is null) | ||
| { | ||
| // Skip build-in operators |
| return false; | ||
| } | ||
|
|
||
| static void populateResults(ArrayBuilder<(MethodSymbol?, OperatorAnalysisResultKind)> results, object? extensionResult) |
| { | ||
| return kind switch | ||
| { | ||
| MemberResolutionKind.ApplicableInExpandedForm => OperatorAnalysisResultKind.Applicable, |
There was a problem hiding this comment.
Is this code path reachable?
I guess this shouldn't be a concern for this component.
| }; | ||
| } | ||
|
|
||
| static void assertNone(ArrayBuilder<(MethodSymbol? member, OperatorAnalysisResultKind resultKind)> results, OperatorAnalysisResultKind kind) |
|
|
||
| if (results.Any(m => m.resultKind == OperatorAnalysisResultKind.Applicable)) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
Yes, for instance ReportDiagnostics_Binary_03 where we get here with only built-in operators being applicable. tryGetTwoBest discards built-in operators since we don't have a member to report them.
| } | ||
|
|
||
| assertNone(results, OperatorAnalysisResultKind.Applicable); | ||
| assertNone(results, OperatorAnalysisResultKind.Worse); |
There was a problem hiding this comment.
I went back and forth on this. I'll put fallback handling here instead, even though I don't think it's currently reachable due to how we produce operator results. It'll be more robust
It feels counterintuitive to do that when Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:556 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
It feels counterintuitive to do that when result of Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:632 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
This looks suspicious and unintuitive. It doesn't look like we tried to report anything from the object. Also it looks like the caller will try to Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:1241 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
Similar comment for a situation when result of Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2157 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
Similar comment for a situation when result of Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2199 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
Same response provided elsewhere applies to remaining comments similar to this one In reply to: 3464425102 Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2157 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
We usually keep this parameter last #Resolved Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:3279 in 99a248c. [](commit_id = 99a248c, deletion_comment = False) |
Diagnostics usually the last parameter, that is when there are no Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:1439 in 99a248c. [](commit_id = 99a248c, deletion_comment = False) |
Consider placing this parameter before Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:1140 in 99a248c. [](commit_id = 99a248c, deletion_comment = False) |
Diagnostics usually the last parameter #Resolved Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:31 in 99a248c. [](commit_id = 99a248c, deletion_comment = False) |
|
@jjonescz @dotnet/roslyn-compiler for second review. Thanks |
| private object? _extensionResult; | ||
|
|
||
| [Conditional("DEBUG")] | ||
| private void AssertInvariant() |
There was a problem hiding this comment.
| private void AssertInvariant() | |
| private readonly void AssertInvariant() | |
| ``` #Resolved |
| /// <summary> | ||
| /// Follows a very simplified version of OverloadResolutionResult.ReportDiagnostics which can be expanded in the future if needed. | ||
| /// </summary> | ||
| internal bool TryReportDiagnostics(SyntaxNode node, Binder binder, object leftDisplay, object? rightDisplay, BindingDiagnosticBag diagnostics) |
There was a problem hiding this comment.
| internal bool TryReportDiagnostics(SyntaxNode node, Binder binder, object leftDisplay, object? rightDisplay, BindingDiagnosticBag diagnostics) | |
| internal readonly bool TryReportDiagnostics(SyntaxNode node, Binder binder, object leftDisplay, object? rightDisplay, BindingDiagnosticBag diagnostics) | |
| ``` #Resolved |
| Debug.Assert(results.All(r => r.resultKind == OperatorAnalysisResultKind.Inapplicable)); | ||
|
|
||
| // There is much room to improve diagnostics on inapplicable candidates, but for now we just report the candidate if there is a single one. | ||
| if (results.Count == 1 && results[0].member is { } inapplicableMember) |
There was a problem hiding this comment.
| if (results.Count == 1 && results[0].member is { } inapplicableMember) | |
| if (results is [{ member: { } inapplicableMember }]) | |
| ``` #Resolved |
| var comp = CreateCompilation(src, targetFramework: TargetFramework.Net90); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (32,17): error CS0035: Operator '-' is ambiguous on an operand of type 'I2' | ||
| // (32,17): error CS9339: Operator resolution is ambiguous between the following members:'I1.operator -(I1)' and 'I3.operator -(I3)' |
There was a problem hiding this comment.
| // (32,17): error CS9339: Operator resolution is ambiguous between the following members:'I1.operator -(I1)' and 'I3.operator -(I3)' | |
| // (32,17): error CS9339: Operator resolution is ambiguous between the following members: 'I1.operator -(I1)' and 'I3.operator -(I3)' | |
| ``` #Resolved |
| """; | ||
|
|
||
| var comp = CreateCompilation(src, options: TestOptions.DebugExe); | ||
| #if DEBUG |
There was a problem hiding this comment.
Why are the diagnostics subtly different between Debug and Release? #ByDesign
There was a problem hiding this comment.
We have GetMembers() and GetMembersUnordered().
We use GetMembersUnordered() in NamedTypeSymbol.GetExtensionMembers() as we do in NamedTypeSymbol.DoGetExtensionMethods().
I explored making the diagnostics deterministic too (in #80552 ) but we ended up deciding against it.
For some reason, GetMembersUnordered() only swaps the first and last elements, instead of a more aggressive de-ordering, so our usage of GetMembersUnordered() isn't apparent is as many tests as you'd think.
| #if DEBUG | ||
| comp.VerifyEmitDiagnostics( | ||
| // (35,13): error CS0035: Operator '-' is ambiguous on an operand of type 'C1' | ||
| // (35,13): error CS9339: Operator resolution is ambiguous between the following members: 'Extensions2.extension(C1).operator -(C1)' and 'Extensions1.extension(C1).operator -(C1)' |
There was a problem hiding this comment.
| // (35,13): error CS9339: Operator resolution is ambiguous between the following members: 'Extensions2.extension(C1).operator -(C1)' and 'Extensions1.extension(C1).operator -(C1)' | |
| // (35,13): error CS9342: Operator resolution is ambiguous between the following members: 'Extensions2.extension(C1).operator -(C1)' and 'Extensions1.extension(C1).operator -(C1)' | |
| ``` #Resolved |
| [Fact] | ||
| public void ReportDiagnostics_CompoundAssignment_01() | ||
| { | ||
| // inner scope has inapplicable operator, outter scope too |
There was a problem hiding this comment.
| // inner scope has inapplicable operator, outter scope too | |
| // inner scope has inapplicable operator, outer scope too | |
| ``` #Resolved |
Addresses parts of #78830
Relates to test plan #76130