Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add new analyzer API (DiagnosticSuppressor) to allow programmatic sup… #36067

Merged
merged 15 commits into from
Jun 19, 2019
Merged

Add new analyzer API (DiagnosticSuppressor) to allow programmatic sup… #36067

merged 15 commits into from
Jun 19, 2019

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented May 30, 2019 •
edited by sharwell
Loading

…pression of analyzer and/or compiler non-error diagnostics

Fixes #20242
Fixes #30172

Detailed design proposal here

@mavasani
Add new analyzer API (DiagnosticSuppressor) to allow programmatic sup…
f2f7a06 
…pression of analyzer and/or compiler non-error diagnostics

Fixes dotnet#20242 and dotnet#30172

Detailed design proposal [here](https://gist.github.com/mavasani/fcac17a9581b5c54cef8a689eeec954a).

Added public APIs with documentation comments:
```cs
namespace Microsoft.CodeAnalysis.Diagnostics
{
    /// <summary>
    /// The base type for diagnostic suppressors that can programmatically suppress analyzer and/or compiler non-error diagnostics.
    /// </summary>
    public abstract class DiagnosticSuppressor : DiagnosticAnalyzer
    {
        // Disallow suppressors from reporting diagnostics or registering analysis actions.
        public sealed override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray<DiagnosticDescriptor>.Empty;

        public sealed override void Initialize(AnalysisContext context) { }

        /// <summary>
        /// Returns a set of descriptors for the suppressions that this suppressor is capable of producing.
        /// </summary>
        public abstract ImmutableArray<SuppressionDescriptor> SupportedSuppressions { get; }

        /// <summary>
        /// Suppress analyzer and/or compiler non-error diagnostics reported for the compilation.
        /// This may be a subset of the full set of reported diagnostics, as an optimization for
        /// supporting incremental and partial analysis scenarios.
        /// A diagnostic is considered suppressible by a DiagnosticSuppressor if *all* of the following conditions are met:
        ///     1. Diagnostic is not already suppressed in source via pragma/suppress message attribute.
        ///     2. Diagnostic's <see cref="Diagnostic.DefaultSeverity"/> is not <see cref="DiagnosticSeverity.Error"/>.
        ///     3. Diagnostic is not tagged with <see cref="WellKnownDiagnosticTags.NotConfigurable"/> custom tag.
        /// </summary>
        public abstract void ReportSuppressions(SuppressionAnalysisContext context);
    }

    /// <summary>
    /// Provides a description about a programmatic suppression of a <see cref="Diagnostic"/> by a <see cref="DiagnosticSuppressor"/>.
    /// </summary>
    public sealed class SuppressionDescriptor : IEquatable<SuppressionDescriptor>
    {
        /// <summary>
        /// An unique identifier for the suppression.
        /// </summary>
        public string Id { get; }

        /// <summary>
        /// Identifier of the suppressed diagnostic, i.e. <see cref="Diagnostic.Id"/>.
        /// </summary>
        public string SuppressedDiagnosticId { get; }

        /// <summary>
        /// A localizable description about the suppression.
        /// </summary>
        public LocalizableString Description { get; }
    }

    /// <summary>
    /// Context for suppressing analyzer and/or compiler non-error diagnostics reported for the compilation.
    /// </summary>
    public struct SuppressionAnalysisContext
    {
        /// <summary>
        /// Suppressible analyzer and/or compiler non-error diagnostics reported for the compilation.
        /// This may be a subset of the full set of reported diagnostics, as an optimization for
        /// supporting incremental and partial analysis scenarios.
        /// A diagnostic is considered suppressible by a DiagnosticSuppressor if *all* of the following conditions are met:
        ///     1. Diagnostic is not already suppressed in source via pragma/suppress message attribute.
        ///     2. Diagnostic's <see cref="Diagnostic.DefaultSeverity"/> is not <see cref="DiagnosticSeverity.Error"/>.
        ///     3. Diagnostic is not tagged with <see cref="WellKnownDiagnosticTags.NotConfigurable"/> custom tag.
        /// </summary>
        public ImmutableArray<Diagnostic> ReportedDiagnostics { get; }

        /// <summary>
        /// Report a <see cref="Suppression"/> for a reported diagnostic.
        /// </summary>
        public void ReportSuppression(Suppression suppression);

        /// <summary>
        /// Gets a <see cref="SemanticModel"/> for the given <see cref="SyntaxTree"/>, which is shared across all analyzers.
        /// </summary>
        public SemanticModel GetSemanticModel(SyntaxTree syntaxTree);

        /// <summary>
        /// <see cref="CodeAnalysis.Compilation"/> for the context.
        /// </summary>
        public Compilation Compilation { get; }

        /// <summary>
        /// Options specified for the analysis.
        /// </summary>
        public AnalyzerOptions Options { get; }

        /// <summary>
        /// Token to check for requested cancellation of the analysis.
        /// </summary>
        public CancellationToken CancellationToken { get; }
    }

    /// <summary>
    /// Programmatic suppression of a <see cref="Diagnostic"/> by a <see cref="DiagnosticSuppressor"/>.
    /// </summary>
    public struct Suppression
    {
        /// <summary>
        /// Creates a suppression of a <see cref="Diagnostic"/> with the given <see cref="SuppressionDescriptor"/>.
        /// </summary>
        /// <param name="descriptor">
        /// Descriptor for the suppression, which must be from <see cref="DiagnosticSuppressor.SupportedSuppressions"/>
        /// for the <see cref="DiagnosticSuppressor"/> creating this suppression.
        /// </param>
        /// <param name="suppressedDiagnostic">
        /// <see cref="Diagnostic"/> to be suppressed, which must be from <see cref="SuppressionAnalysisContext.ReportedDiagnostics"/>
        /// for the suppression context in which this suppression is being created.</param>
        public static Suppression Create(SuppressionDescriptor descriptor, Diagnostic suppressedDiagnostic);

        /// <summary>
        /// Descriptor for this suppression.
        /// </summary>
        public SuppressionDescriptor Descriptor { get; }

        /// <summary>
        /// Diagnostic suppressed by this suppression.
        /// </summary>
        public Diagnostic SuppressedDiagnostic { get; }
    }
}
```

For batch compilation, suppressors always run after all the analyzer diagnostics have been computed.
For IDE partial/incremental analysis scenario, we may run the suppressors with partial diagnostics.
Suppressed diagnostics from diagnostic suppressors are equivalent to source suppressed diagnostics: they show up in the error list with "Suppression State" column as "Suppressed" and are also output to errorlog as suppressed diagnostics.
@mavasani mavasani mentioned this pull request May 30, 2019
Closed
@mavasani mavasani marked this pull request as ready for review June 2, 2019 13:55
@mavasani mavasani requested review from a team as code owners June 2, 2019 13:55
@agocke
Copy link
Member

agocke commented Jun 3, 2019

@mavasani What do you plan on doing about http://source.roslyn.io/#Microsoft.CodeAnalysis/DiagnosticAnalyzer/CompilationWithAnalyzers.cs,1153

which is a public static method that only has access to a Compilation?

@mavasani
Copy link
Contributor Author

mavasani commented Jun 3, 2019

What do you plan on doing about http://source.roslyn.io/#Microsoft.CodeAnalysis/DiagnosticAnalyzer/CompilationWithAnalyzers.cs,1153

I did not plan on changing the semantics of that API, which deals with applying configurations/suppressions within the given compilation.

mavasani
mavasani commented Jun 5, 2019
View reviewed changes
src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
additionalFlags: new[] { "/warnAsError", "/features:DiagnosticSuppressor" }, includeCurrentAssemblyAsAnalyzerReference: false);
Assert.Contains("error CS1522", output, StringComparison.Ordinal);

// Verify that compiler warning CS1522 is suppressed with diagnostic suppressor even with /warnaserror
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agocke I presume this unit test covers the scenario you mentioned in the email thread: Compiler warning with /warnaserror can be suppressed by a diagnostic suppressor, and it doesn't fail the build.

@mavasani
Copy link
Contributor Author

mavasani commented Jun 6, 2019

Ping @dotnet/roslyn-compiler @dotnet/roslyn-ide for reviews.

agocke
agocke previously requested changes Jun 6, 2019
View reviewed changes
src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs Outdated Show resolved Hide resolved
@mavasani mavasani mentioned this pull request Jun 6, 2019
Closed
@mavasani mavasani requested a review from agocke June 10, 2019 18:00
@mavasani mavasani changed the base branch from master to release/dev16.3-preview1 June 11, 2019 23:58
@mavasani
Merge remote-tracking branch 'upstream/release/dev16.3-preview1' into…
7345b67 
… DiagnosticSuppressorForCompilerAndAnalyzerDiagnostics
@mavasani
Copy link
Contributor Author

FYI: I have rebased the PR to target release/dev16.3-preview1. The additional commits showing up are the ones that have made into master, but pending to be merged into release/dev16.3-preview (i.e. #36336) and should go away once that PR is merged.

@mavasani
Merge remote-tracking branch 'upstream/release/dev16.3-preview1' into…
3ce4484 
… DiagnosticSuppressorForCompilerAndAnalyzerDiagnostics
@mavasani
Copy link
Contributor Author

@heejaechang @roslyn-ide for reviewing IDE changes.
@agocke - any further concerns for compiler changes?

@mavasani mavasani dismissed agocke’s stale review June 12, 2019 16:41

Feedback addressed

@sharwell
Copy link
Member

@mavasani The changes in AnalyzerRunner LGTM

@mavasani
Copy link
Contributor Author

Ping @dotnet/roslyn-compiler for second approval

@agocke
Copy link
Member

agocke commented Jun 13, 2019

@mavasani Is this high priority for 16.3-preview1? We're currently working on a number of remaining nullable bugs for preview 7 that we want to get done ASAP.

@agocke
Copy link
Member

agocke commented Jun 13, 2019

Talking offline, sounds like preview1 is the target. I'll try to get to this today or tomorrow

agocke
agocke reviewed Jun 15, 2019
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Diagnostics/DiagnosticSuppressorTests.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Semantic/Diagnostics/DiagnosticSuppressorTests.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs Show resolved Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerTelemetry.cs Show resolved Hide resolved
@mavasani
Copy link
Contributor Author

@agocke Addressed feedback.

agocke
agocke approved these changes Jun 18, 2019
View reviewed changes
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

Aside from the struct/class question, LGTM

@mavasani mavasani mentioned this pull request Jun 19, 2019
Closed
@mavasani mavasani merged commit f7ce215 into dotnet:release/dev16.3-preview1 Jun 19, 2019
@mavasani mavasani deleted the DiagnosticSuppressorForCompilerAndAnalyzerDiagnostics branch June 19, 2019 05:27
This was referenced Jun 19, 2019
Closed
Closed
mavasani added a commit to mavasani/roslyn that referenced this pull request Jun 19, 2019
@mavasani
Fix diagnostic suppressor feature for compiler warnings in presence o…
d3a2da9 
…f /warnaserror

Follow up to dotnet#36067
Fixes dotnet#36215
This was referenced Jun 19, 2019
Merged
Closed
Closed
@mavasani mavasani mentioned this pull request Jun 26, 2019
Closed
@sailro sailro mentioned this pull request Feb 12, 2020
Closed
@adamralph adamralph mentioned this pull request Mar 21, 2021
Merged
@sharwell sharwell added this to the 16.3.P1 milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gafter gafter gafter left review comments

@333fred 333fred 333fred left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@agocke agocke agocke approved these changes

@sharwell sharwell Awaiting requested review from sharwell

@tmat tmat Awaiting requested review from tmat

@heejaechang heejaechang Awaiting requested review from heejaechang

@jcouv jcouv Awaiting requested review from jcouv

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
16.3.P1
Development

Successfully merging this pull request may close these issues.
7 participants
@mavasani @agocke @AlekseyTs @gafter @sharwell @333fred @jinujoseph