Don't report diagnostics if code-style option is disabled #62497
Conversation
ghost
added
Community
| /// <param name="messageArgs">Arguments to the message of the diagnostic.</param> | ||
| /// <returns>The <see cref="Diagnostic"/> instance.</returns> | ||
| public static Diagnostic Create( | ||
| public static DiagnosticWrapper Create( |
There was a problem hiding this comment.
Do we really need a new wrapper type? Can't we nullable enable all the calling code and have this method return Diagnostic??
There was a problem hiding this comment.
@mavasani All analyzers will have to do the null check. I wanted to minimize changes to analyzers.
And an extension method named ReportDiagnostic will not picked up unless the parameter type is different (the compiler will prefer the existing ReportDiagnostic over the extension)
And Adding an extension with a new name will lead to updates in all analyzers.
There was a problem hiding this comment.
Thanks, so we are basically trading off the cleanest API to avoid making tons of changes in the code base. That seems reasonable to me, but I'll let others in @dotnet/roslyn-analysis share their views.
There was a problem hiding this comment.
Meant to tag @dotnet/roslyn-ide
| /// </param> | ||
| /// <param name="message">Localizable message for the diagnostic.</param> | ||
| /// <returns>The <see cref="Diagnostic"/> instance.</returns> | ||
| public static Diagnostic CreateWithMessage( |
There was a problem hiding this comment.
I am still unsure if removing this overload and LocalizableStringWithArguments is required for this PR. Can you clarify why it is being done here?
There was a problem hiding this comment.
LocalizableStringWithArguments was originally introduced to support option = value:none. Since this PR introduces a new way to support none, the type is no longer needed.
This has also caught extra period in analyzer messages.
| var diagnostic = DiagnosticHelper.CreateWithMessage(s_unusedParameterRule, location, | ||
| option.Notification.Severity, additionalLocations: null, properties: null, message); | ||
| reportDiagnostic(diagnostic); | ||
| var diagnostic = DiagnosticHelper.Create(rule, location, |
There was a problem hiding this comment.
It would be much cleaner to rename this method to TryCreate and have it return a Diagnostic?. Also you can easily avoid the null check at each callsite by having a helper TryCreateAndReport(..., reportDiagnostic).
There was a problem hiding this comment.
@mavasani My intent was to avoid too many changes to analyzers.
|
Converting to a draft, I'm thinking of a larger refactoring which (if accepted) might make this change easier. Note to self (just in case I forgot what I have in mind 😄):
The new If we have now our own analysis contexts that delegate to the compiler, we could simply allow a null diagnostic. The design above has a flaw for analyzers with multiple options, but hopefully it won't be too hard to figure out a good way for these analyzers. |
|
Marking as ready for review again. The refactoring I described in previous comment isn't going well.. :( |
2 participants
Replaces #62150
Previously, we report diagnostics with
IsSuppressedif the code-style option is disabled. The use ofIsSuppressedin this case is suspicious, this property is related to source (attribute/pragma) suppressions. If code-style option is disabled, no diagnostic should be reported.@mavasani for review
Core changes are in
DiagnosticHelper.cs