-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Only the compiler analyzer can use CS or BC prefix for diagnostics #23776
Conversation
e1a8d9d
to
bc5462f
Compare
private static bool IsCompilerReservedDiagnostic(string id) | ||
{ | ||
// Only the compiler analyzer should produce diagnostics with CS or BC prefixes (followed by digit) | ||
if (id.Length >= 3 && (id.StartsWith("CS", StringComparison.Ordinal) || id.StartsWith("BC", StringComparison.Ordinal))) |
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.
Would it be better to instead check for presence of custom tag WellKnownDiagnosticTags.Compiler on the diagnostic descriptor?
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.
It feels like this tag would only be set by the compiler, and so would not help catch regular analyzers misbehaving.
I can double-check though.
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.
Ah yes, that is correct. We should probably add to your current check and disallow that tag on any analyzer diagnostic. Only compiler diagnostics should use that tag.
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.
That doesn't need to happen in this PR though..
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.
Confirmed (by stepping through the tests I added for this scenario) that tags are not a good criteria.
test windows_release_unit64_prtest please |
@Pilchie for ask-mode approval for 15.6. Thanks |
This PR reverts that change and a similar diagnostic will be implemented as an analyzer with dotnet/roslyn-analyzers#1727. Fixes dotnet#25748
Revert compat break change made as part of #23776
Customer scenario
Some analyzer (such as the UnboundIdentifier analyzer in the linked issue) produce similar diagnostics (ID and message) to the compiler's.
That is extremely confusing and leads to wasting time in investigation.
In the screenshot below, the red squiggles on
nameof
are actually produced by an analyzer, which makes a "CS0107" diagnostic...Bugs this fixes
Fixes part of #23667
Fixes part of #22615
Workarounds, if any
Not applicable.
Risk
Low. This PR modifies the analyzer infrastructure to catch when non-compiler analyzers produce a diagnostic starting with "CS" or "BC". A couple of such faulty analyzers are updated to produce distinct diagnostics instead (with correct "IDE" prefix).
Performance impact
Low. Just an additional validation on diagnostics.
Is this a regression from a previous update?
No.
How was the bug found?
Some customer reported issues caused me to hit this problem during investigation.