Skip to content

Add IDE analyzer for unnecessary pragma suppressions#44848

Merged
13 commits merged intodotnet:masterfrom
mavasani:RemoveUnncessaryCompilerPragmas
Jul 1, 2020
Merged

Add IDE analyzer for unnecessary pragma suppressions#44848
13 commits merged intodotnet:masterfrom
mavasani:RemoveUnncessaryCompilerPragmas

Conversation

@mavasani
Copy link
Contributor

@mavasani mavasani commented Jun 4, 2020 •
edited
Loading

Fixes #44177
Addresses part of #44178

NOTE: This analyzer can only be executed from IDE, not in command line builds due to the following requirements/restrictions:

  1. Detecting unnecessary suppressions requires knowing all suppressed diagnostics in a file, so we can identify the pragma suppressions that are not used.
  2. Detecting unnecessary analyzer diagnostic suppressions requires all reported (suppressed) diagnostics for relevant analyzers. As regular analyzers cannot have access to other analyzers or their diagnostics, this is a special IDE-only analyzer. This analyzer requires special CompilationWithAnalyzers context information to compute its diagnostics, hence is specially handled and invoked in the IDE diagnostic service.

image

@mavasani
Add IDE analyzer for unnecessary pragma suppressions
053d5ac 
Fixes dotnet#44177
Addresses part of dotnet#44178

NOTE: This analyzer can only be executed from IDE, not in command line builds due to the following requirements/restrictions:

1. Detecting unnecessary suppressions requires knowing all suppressed diagnostics in a file, so we can identify the pragma suppressions that are not used.
2. Detecting unnecessary analyzer diagnostic suppressions requires all reported (suppressed) diagnostics for relevant analyzers. As regular analyzers cannot have access to other analyzers or their diagnostics, this is a special IDE-only analyzer. This analyzer requires special CompilationWithAnalyzers context information to compute its diagnostics, hence is specially handled and invoked in the IDE diagnostic service.
@mavasani mavasani force-pushed the RemoveUnncessaryCompilerPragmas branch from e230dcb to 053d5ac Compare June 4, 2020 13:51
@mavasani mavasani changed the title [WIP] Add IDE analyzer for unnecessary pragma suppressions Add IDE analyzer for unnecessary pragma suppressions Jun 4, 2020
@mavasani mavasani marked this pull request as ready for review June 4, 2020 19:54
@mavasani mavasani requested a review from a team as a code owner June 4, 2020 19:54
@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Jun 4, 2020

Can you show a picture of this in actino?

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 4, 2020
View reviewed changes
...RemoveUnnecessarySuppressions/CSharpRemoveUnnecessaryPragmaSuppressionsDiagnosticAnalyzer.cs
protected override int CompilerErrorCodeDigitCount => 4;
protected override ISyntaxFacts SyntaxFacts => CSharpSyntaxFacts.Instance;
protected override (Assembly assembly, string typeName) GetCompilerDiagnosticAnalyzerInfo()
=> (typeof(SyntaxKind).Assembly, CompilerDiagnosticAnalyzerNames.CSharpCompilerAnalyzerTypeName);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Jun 4, 2020

Choose a reason for hiding this comment

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

slightly worried to see Assembly here... will hold off judgemnt to see how this is used.

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 4, 2020
View reviewed changes
@mavasani
Copy link
Contributor Author

mavasani commented Jun 4, 2020

Can you show a picture of this in actino?

Sure, updated the PR description.

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 4, 2020
View reviewed changes
@mavasani
Copy link
Contributor Author

mavasani commented Jun 26, 2020

@tmat @sharwell Can one of you please take a look at IDE engine changes? @CyrusNajmabadi gave his approval offline for the analyzer/fixer changes, but would prefer one of you to review the remainder of changes. Thanks!

mavasani
mavasani commented Jun 26, 2020
View reviewed changes
src/EditorFeatures/Test/Diagnostics/DiagnosticAnalyzerServiceTests.cs
}

[Theory, CombinatorialData]
internal async Task TestPragmaSuppressionsAnalyzer(BackgroundAnalysisScope analysisScope)
Copy link
Contributor Author

@mavasani mavasani Jun 26, 2020

Choose a reason for hiding this comment

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

Test for IDE diagnostic incremental analyzer changes.

mavasani
mavasani commented Jun 26, 2020
View reviewed changes
src/Features/Core/Portable/Diagnostics/DocumentAnalysisExecutor.cs
return await _compilationWithAnalyzers.GetAnalyzerSemanticDiagnosticsAsync(model, adjustedSpan, ImmutableArray.Create(analyzer), cancellationToken).ConfigureAwait(false);
}

// We specially handle IPragmaSuppressionsAnalyzer by passing in the 'CompilationWithAnalyzers'
Copy link
Contributor Author

@mavasani mavasani Jun 26, 2020

Choose a reason for hiding this comment

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

IDE diagnostic incremental analyzer change for open file analysis.

mavasani
mavasani commented Jun 26, 2020
View reviewed changes
src/Workspaces/Core/Portable/Diagnostics/Extensions.cs

public static ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResultBuilder> ToResultBuilderMap(
this AnalysisResult analysisResult,
ImmutableArray<Diagnostic> additionalPragmaSuppressionDiagnostics,
Copy link
Contributor Author

@mavasani mavasani Jun 26, 2020

Choose a reason for hiding this comment

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

This file has changes to IDE diagnostic analyzer execution for OOP execution.

jmarolf
jmarolf reviewed Jun 29, 2020
View reviewed changes
src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs
bool includeSuppressedDiagnostics;
if (diagnosticIds.Contains(IDEDiagnosticIds.RemoveUnnecessarySuppressionDiagnosticId))
{
diagnosticIdsForDiagnosticProvider = null;
Copy link
Contributor

@jmarolf jmarolf Jun 29, 2020

Choose a reason for hiding this comment

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

It feels weird to have setting this to null create this implicit behavior. Can we refactor this code to make this a less implicit side-effect in the codebase?

Copy link
Contributor Author

@mavasani mavasani Jun 30, 2020

Choose a reason for hiding this comment

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

Yeah, but using null diagnosticId is used at multiple places in the IDE diagnostic service in features layer, so I kept it consistent.

roslyn/src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

Line 65 in 91e67a8

Task<ImmutableArray<DiagnosticData>> GetProjectDiagnosticsForIdsAsync(Solution solution, ProjectId? projectId = null, ImmutableHashSet<string>? diagnosticIds = null, bool includeSuppressedDiagnostics = false, CancellationToken cancellationToken = default);

roslyn/src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

Lines 75 to 81 in 91e67a8

/// <summary>
/// Return up to date diagnostics for the given span for the document
///
/// This can be expensive since it is force analyzing diagnostics if it doesn't have up-to-date one yet.
/// If diagnosticIdOpt is not null, it gets diagnostics only for this given diagnosticIdOpt value
/// </summary>
Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForSpanAsync(Document document, TextSpan range, string? diagnosticIdOpt = null, bool includeSuppressedDiagnostics = false, Func<string, IDisposable?>? addOperationScope = null, CancellationToken cancellationToken = default);

roslyn/src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnostics.cs

Lines 252 to 253 in 91e67a8

protected override bool ShouldIncludeDiagnostic(DiagnosticData diagnostic)
=> _diagnosticIds == null || _diagnosticIds.Contains(diagnostic.Id);

I think we need a cleanup across the services to replace null diagnostic IDs with some better representation.

@sharwell sharwell mentioned this pull request Jun 30, 2020
Merged
@mavasani mavasani mentioned this pull request Jun 30, 2020
Merged
@mavasani
Copy link
Contributor Author

mavasani commented Jul 1, 2020

Thank you @jmarolf!

ghost
ghost approved these changes Jul 1, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit a432910 into dotnet:master Jul 1, 2020
@mavasani mavasani deleted the RemoveUnncessaryCompilerPragmas branch July 1, 2020 08:32
@jinujoseph jinujoseph added this to the 16.8.P1 milestone Jul 1, 2020
mavasani added a commit to mavasani/roslyn that referenced this pull request Jul 7, 2020
@mavasani
Add support to detect unnecessary local SuppressMessageAttribute supp…
73fd51e 
…ressions

Fixes dotnet#44178

Builds on top of dotnet#44848, which added support to detect unnecessary pragma suppressions.
This was referenced Jul 7, 2020
Merged
Closed
@MaStr11 MaStr11 mentioned this pull request Aug 14, 2020
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

5 more reviewers

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@jmarolf jmarolf jmarolf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE auto-merge

Projects

None yet

Milestone

16.8.P1

Development

Successfully merging this pull request may close these issues.

Analyzer/fixer for unnecessary pragma suppressions for compiler diagnostics

6 participants

@mavasani @CyrusNajmabadi @sharwell @jmarolf @Youssef1313 @jinujoseph