Skip to content

Respect ReportSuppressedDiagnostics option for DiagnosticSuppressor#53045

Merged
mavasani merged 9 commits intodotnet:mainfrom
safu9:fix-41584
Mar 16, 2022
Merged

Respect ReportSuppressedDiagnostics option for DiagnosticSuppressor#53045
mavasani merged 9 commits intodotnet:mainfrom
safu9:fix-41584

Conversation

@safu9
Copy link
Copy Markdown
Contributor

@safu9 safu9 commented Apr 30, 2021

This PR fixes #41584.

Diagnostics suppressed programatically by DiagnosticSuppressor were not excluded properly when ReportSuppressedDiagnostics option is set to true.

@safu9 safu9 requested a review from a team as a code owner April 30, 2021 01:42
@ghost ghost added the Area-Analyzers label Apr 30, 2021
@safu9 safu9 mentioned this pull request May 1, 2021
Merged
@safu9
Copy link
Copy Markdown
Contributor Author

safu9 commented May 5, 2021

@sharwell @mavasani Could you review this PR?

mavasani
mavasani suggested changes May 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compilation option is not to determine whether or not diagnostic suppressors should run. The option indicates whether or suppressed diagnostics should be filtered out from final diagnostics output. Basically, this option is unrelated to suppressors.

@safu9
Copy link
Copy Markdown
Contributor Author

safu9 commented May 5, 2021

I mentioned this in #52953.
This option works properly with many ways to suppress diagnostics such as pragma warning disable and SuppressMessageAttribute.
I think the only exception is DiagnosticSuppressor.
This option does not exclude diagnostics suppressed by DiagnosticSuppressor for now.

The current program executes following steps if this option is true.

  1. List up all diagnostics and call this method.

    roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

    Line 1070 in 2f20729

    private ImmutableArray<Diagnostic> FilterDiagnosticsSuppressedInSourceOrByAnalyzers(ImmutableArray<Diagnostic> diagnostics, Compilation compilation)
  2. At line 1072, FilterDiagnosticsSuppressedInSource exclude suppressed diagnostics in source, which is fine.
  3. At line 1073, ApplyProgrammaticSuppressions check whether each diagnostic is suppressed by DiagnosticSuppressor.

In 3rd step, ApplyProgrammaticSuppressions is expected exclude suppressed diagnostics, but they actually only set Diagnostic.IsSuppressed to true and return them.
I fixed this behaviour of ApplyProgrammaticSuppressions in this PR.
This PR doesn't prevent DiagnosticSuppressor from runnning.

@Youssef1313
Copy link
Copy Markdown
Member

Youssef1313 commented May 5, 2021 •
edited
Loading

What I understand from the PR is that currently, ReportSuppressedDiagnostics isn't respected for DiagnosticSuppressors. However, I see DiagnosticSuppressor tests in roslyn-analyzers relying on ReportSuppressedDiagnostics property.

https://github.com/dotnet/roslyn-analyzers/blob/a534bf8c8310f375d163f701ade001e47ef55fb2/src/Roslyn.Diagnostics.Analyzers/UnitTests/RelaxTestNamingSuppressorTests.cs#L24

Not sure what's going on?

Edit: I see tthe tests are skipped.

@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented May 5, 2021

I agree with the analysis in #53045 (comment). This oversight is the main reason we don't have any support for testing diagnostic suppressors in Microsoft.CodeAnalysis.Testing.

@Youssef1313
Copy link
Copy Markdown
Member

Youssef1313 commented May 5, 2021

I agree with the analysis in #53045 (comment). This oversight is the main reason we don't have any support for testing diagnostic suppressors in Microsoft.CodeAnalysis.Testing.

FYI - I managed to write tests for suppressors before. dotnet/efcore#24151 (might not be the best way - but you may be able to use a similar idea for roslyn-sdk until this PR is merged and shipped)

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 5, 2021
@safu9 safu9 requested review from mavasani and removed request for a team May 6, 2021 05:47
@mavasani
Copy link
Copy Markdown
Contributor

mavasani commented May 6, 2021

The implementation in this PR is wrong, the filtering is done at the wrong place. Note that without your change, all programmatically suppressed diagnostics from suppressors will always go through the below code path and info diagnostics will be reported in binary logs for them, regardless of the value of this option:

roslyn/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

Lines 560 to 571 in a6b8bb5

// If the diagnostic was suppressed by one or more DiagnosticSuppressor(s), then we report info diagnostics for each suppression
// so that the suppression information is available in the binary logs and verbose build logs.
if (diag.ProgrammaticSuppressionInfo != null)
{
foreach (var (id, justification) in diag.ProgrammaticSuppressionInfo.Suppressions)
{
var suppressionDiag = new SuppressionDiagnostic(diag, id, justification);
if (_reportedDiagnostics.Add(suppressionDiag))
{
PrintError(suppressionDiag, consoleOutput);
}
}

With your change, this will no longer be true when ReportSuppressedDiagnostics option is false. Note that always reporting these info diagnostics corresponding to diagnostics suppressed by suppressors was a hard requirement in the original design approval of suppressors. Without these info diagnostics in binlogs, end users would have no idea what caused their diagnostics to be suppressed.

The correct fix here would be to revert your changes and move out the filtering to be done just after the callsite inside CompilationWithAnalyzers to apply programmatic suppressions:

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

Line 468 in a6b8bb5

return driver.ApplyProgrammaticSuppressions(reportedDiagnostics, compilation);

Also, please add unit tests for this change.

@Youssef1313
Copy link
Copy Markdown
Member

Youssef1313 commented May 6, 2021

@mavasani Wouldn't that make the method FilterDiagnosticsSuppressedInSourceOrByAnalyzers misleading? It won't be filtering anything by DiagnosticSuppressors as the name suggests?

@mavasani
Copy link
Copy Markdown
Contributor

mavasani commented May 6, 2021

@Youssef1313 Naming is a minor thing for a private method. We cannot cause a functionality break for the most important requirement in the DiagnosticSuppressor design.

@Youssef1313
Copy link
Copy Markdown
Member

Youssef1313 commented May 6, 2021

@mavasani Yeah certainly. I meant that we could rename it as a minor cleanup.

@safu9
Copy link
Copy Markdown
Contributor Author

safu9 commented May 7, 2021

@mavasani Thank you for the review. I'll fix the implementation of CompilationWithAnalyzers.

@safu9
Copy link
Copy Markdown
Contributor Author

safu9 commented Jun 9, 2021

@mavasani I have rewrited the implementation and add some tests.
This should not change behaviour of CommonCompiler, and just fix CompilationWithAnalyzers.
CommonCompiler is using AnalyzerDriver.ApplyProgramaticSuppressions, so I revert its changes and added a new method to filter suppressed diagnostics.

@safu9
Copy link
Copy Markdown
Contributor Author

safu9 commented Jun 19, 2021

@mavasani Could you re-review this?

@Youssef1313
Copy link
Copy Markdown
Member

Youssef1313 commented Jan 21, 2022

@mavasani Can you take a look please? Thanks!

mavasani
mavasani reviewed Jan 24, 2022
View reviewed changes
mavasani
mavasani reviewed Jan 24, 2022
View reviewed changes
mavasani
mavasani approved these changes Jan 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I have couple of suggestions to further simplify the code changes.

@mavasani
Copy link
Copy Markdown
Contributor

mavasani commented Jan 24, 2022

@dotnet/roslyn-compiler for reviews

safu9 and others added 2 commits January 26, 2022 11:04
@safu9
Simplify code
aaf8c6d
@safu9 @mavasani
Remove redundant test
7cb680e 
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
@runfoapp runfoapp bot mentioned this pull request Jan 26, 2022
Closed
@jinujoseph jinujoseph requested a review from a team February 7, 2022 08:10
@cston
Copy link
Copy Markdown
Contributor

cston commented Feb 8, 2022

@dotnet/roslyn-compiler for a second compiler review.

@mavasani
Copy link
Copy Markdown
Contributor

mavasani commented Feb 24, 2022

@dotnet/roslyn-compiler for second review.

@mavasani mavasani merged commit cb71476 into dotnet:main Mar 16, 2022
@ghost ghost added this to the Next milestone Mar 16, 2022
@safu9 safu9 deleted the fix-41584 branch March 16, 2022 04:06
@safu9 safu9 restored the fix-41584 branch March 16, 2022 04:06
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
@Andreas-Dorfer Andreas-Dorfer mentioned this pull request May 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@333fred 333fred 333fred approved these changes

+3 more reviewers

@sharwell sharwell sharwell approved these changes

@mavasani mavasani mavasani approved these changes

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mavasani mavasani

Labels

Area-Analyzers Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

17.2.P3

Development

Successfully merging this pull request may close these issues.

CompilationWithAnalyzers.GetAllDiagnosticsAsync() does not respect CompilationWithAnalyzersOptions.ReportSuppressedDiagnostics

8 participants

@safu9 @Youssef1313 @sharwell @mavasani @cston @333fred @jinujoseph @allisonchou