-
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
Ensure that we respect source suppressions for IDE analyzer diagnosti… #11343
Conversation
@heejaechang @dotnet/roslyn-analysis can you please review? |
Ping |
_updateSource.UpdateDiagnosticsForProject(projectId, Tuple.Create(s_analyzerChangedErrorId, analyzerPath), SpecializedCollections.SingletonEnumerable(data)); | ||
var messageArguments = new string[] { analyzerPath }; | ||
var diagnostic = await DiagnosticData.CreateAsync(_analyzerChangedRule, messageArguments, projectId, _workspace, CancellationToken.None).ConfigureAwait(false); | ||
_updateSource.UpdateDiagnosticsForProject(projectId, Tuple.Create(s_analyzerChangedErrorId, analyzerPath), SpecializedCollections.SingletonEnumerable(diagnostic)); |
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 it okay to be async now? and order of event become changed? for example, it raised event (but not registered to update source yet) but before that happens, solution is closed and then it is registered to update source.
left some comment, I think isSuppressed argument is a bit weird since diagnostic and issuppressed can be 2 different value given by users. |
…cs (analyzer dependency errors, reading ruleset errors, etc.) Fixes dotnet#8877
@heejaechang Thanks. I have addressed your feedback and it should be much cleaner now. |
@heejaechang Does the new changes look good? |
{ | ||
// Get diagnostic with effective severity. | ||
// Additionally, if the diagnostic was suppressed by a source suppression, effectiveDiagnostics will have a diagnostic with IsSuppressed = true. | ||
var compilation = project.GetCompilationAsync(cancellationToken).WaitAndGetResult(cancellationToken); |
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.
will this be okay?
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.
Not sure, but all the current callsites are sync. We can revisit in future.
👍 |
…cs (analyzer dependency errors, reading ruleset errors, etc.)
Fixes #8877