-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Extensions: improve diagnostics for operators #80928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// <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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | ||
| if (res.Signature.Method is null) | ||
| { | ||
| // Skip build-in operators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | ||
| if (res.Signature.Method is null) | ||
| { | ||
| // Skip build-in operators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return false; | ||
| } | ||
|
|
||
| static void populateResults(ArrayBuilder<(MethodSymbol?, OperatorAnalysisResultKind)> results, object? extensionResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | ||
| return kind switch | ||
| { | ||
| MemberResolutionKind.ApplicableInExpandedForm => OperatorAnalysisResultKind.Applicable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| if (results.Any(m => m.resultKind == OperatorAnalysisResultKind.Applicable)) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
|
Done with review pass (commit 2) |
We collect information for reporting systematically. The information we collect may or may not end up being needed to report more diagnostics. In reply to: 3464049874 Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:556 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
Thanks, that was wrong In reply to: 3464343239 Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:1241 in 6a9164b. [](commit_id = 6a9164b, deletion_comment = False) |
|
Done with review pass (commit 3) |
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 5), assuming CI is passing
|
@jjonescz @dotnet/roslyn-compiler for second review. Thanks |
| private object? _extensionResult; | ||
|
|
||
| [Conditional("DEBUG")] | ||
| private void AssertInvariant() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the diagnostics subtly different between Debug and Release? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 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