Remove unneeded LocalizableStringWithArguments#62150
Remove unneeded LocalizableStringWithArguments#62150Youssef1313 wants to merge 3 commits intodotnet:mainfrom
Conversation
ghost
added
Community
|
@mavasani Can you take a look please? |
| if (messageArgs == null || messageArgs.Length == 0) | ||
| { | ||
| message = descriptor.MessageFormat; | ||
| } | ||
| else | ||
| if (effectiveSeverity == ReportDiagnostic.Suppress) | ||
| { | ||
| message = new LocalizableStringWithArguments(descriptor.MessageFormat, messageArgs); | ||
| return s_suppressedDiagnostic; | ||
| } | ||
|
|
||
| return CreateWithMessage(descriptor, location, effectiveSeverity, additionalLocations, properties, message); | ||
| return Diagnostic.Create(descriptor, location, effectiveSeverity.ToDiagnosticSeverity() ?? descriptor.DefaultSeverity, additionalLocations, properties, messageArgs); |
There was a problem hiding this comment.
The public compiler API doesn't have any API that accepts message format and message arguments along with isSuppressed flag.
The only API that takes isSuppressed takes a whole message. I'm using a static diagnostic that should be discarded by the compiler instead of IsSuppressed.
| if (effectiveSeverity == ReportDiagnostic.Suppress) | ||
| { | ||
| message = new LocalizableStringWithArguments(descriptor.MessageFormat, messageArgs); | ||
| return s_suppressedDiagnostic; |
There was a problem hiding this comment.
This is not correct. Even suppressed diagnostics need to be created with correct fields, such as ID, message, etc. Note that source suppressed diagnostics show up in the error list (not by default, but can be enabled by changing the filter on Suppression State column) and can also be unsuppressed by right clicking the entry in the error list and executing the Remove Suppression command.
There was a problem hiding this comment.
@mavasani I thought the (DiagnosticSeverity)(-2) will make the compiler discard the diagnostic completely in
There was a problem hiding this comment.
I don't think we want that - suppressed diagnostics need to be present in the IDE diagnostic system for bunch of features around them to keep working.
There was a problem hiding this comment.
Can you point out what feature(s) need them?
|
@Youssef1313 Can you please clarify the motivation behind this change? I don't see any trivial simplification/code cleanup/bug fix happening in this PR, so am really keen on knowing why you feel this change is better than the current implementation. |
|
@mavasani Most important thing is Also as you mentioned in #62150 (comment), such diagnostics can show in error list. The correct behavior in my opinion is to avoid showing them at all, which (hopefully) is achieved by using -2 for the severity (I haven't yet confirmed if it works that way) |
Why would we want to avoid showing them in the error list? Suppressed diagnostics should should up in the error list when the
|
|
@mavasani The thing is that they're not source suppressed. If I understand correctly, the IDE should show suppressions that are done in source (attribute or pragma). The current use of roslyn/src/Compilers/Core/Portable/Diagnostic/Diagnostic.cs Lines 338 to 341 in f7634f7 |
Ah, that is indeed correct. However, I don't think the approach chosen here will work. An analyzer wouldn't be allowed to report a diagnostic with an ID outside the set of IDs in its |
|
I think the correct approach is either to change the |
|
I wanted to make that with minimal changes as possible. But I'll switch to the I partially did that in the last commit. Remaining is some callsites. Can you review the general approach before I fix all callsites? Thanks! |
|
@mavasani Can you check if the approach taken now is good enough? It requires modifying all analyzers (unfortunately). |
|
Another idea that may avoid too many changes is something similar to: struct DiagnosticWrapper
{
Diagnostic diagnostic;
}
class DiagnosticHelper
{
// all create methods return a DiagnosticWrapper that potentially has a null diagnostic.
}
static class DiagnosticExtensions
{
// this will be repeated for all available analysis context types.
public static void ReportDiagnostic(this Context context, DiagnosticWrapper wrapper)
{
if (wrapper.diagnostic is not null) context.ReportDiagnostic(wrapper.diagnostic);
}
} |
|
I'll open a new PR with the approach I mentioned in the previous comment. If it's not the preferred approach, I'll re-open this PR and work on it. |
2 participants
No description provided.