Add a new SuppressMessageAttribute target scope to suppress diagnosti… #31092

mavasani · 2018-11-10T00:18:43Z

…cs in a namespace and all its descendant symbols

Fixes #486

This SuppressMessageAttribute feature has been requested for a very long time and still gets frequent customer requests to date.

The current "Namespace" target scope for SuppressMessageAttribute suppresses diagnostics only in nodes directly contained within the namespace, but not in any of it's descendant symbols. This makes it very tedious to suppress certain analyzer diagnostics in entire namespace as one needs to add suppressions to individual types/namespaces within the namespace. This also creates a maintenance nightmare.

This PR adds a new target scope "NamespaceAndChildren" which suppresses diagnostics on the corresponding namespace and all its descendant symbols/members. Better naming suggestions for this scope name are welcome!

Example: [assembly: SuppressMessage("Test", "Declaration", Scope="NamespaceAndChildren", Target="N.N1")]

…cs in a namespace and all its descendant symbols Fixes dotnet#486 This SuppressMessageAttribute feature has been requested for a very long time and still gets frequent customer requests to date. The current "Namespace" target scope for SuppressMessageAttribute suppresses diagnostics only in nodes directly contained within the namespace, but in none of it's child symbols. This makes it very tedious to suppress certain analyzer diagnostics in entire namespace as one needs to add suppressions to individual types/namespaces within the namespace. This also creates a maintenance nightmare. This PR adds a new target scope "NamespaceAndChildren" which suppresses diagnostics on the corresponding namespace and all its descendant symbols/members. Better naming suggestions for this scope name are welcome!

mavasani · 2018-11-10T05:01:49Z

retest windows_release_vs-integration_prtest please #Resolved

josefpihrt · 2018-11-12T14:32:08Z

Maybe it would be better to use name "NamespaceAndDescendants" to keep established terminology. #Resolved

mavasani · 2018-11-12T23:45:14Z

Maybe it would be better to use name "NamespaceAndDescendants" to keep established terminology.

That seems reasonable. If we don't reach a consensus on the terminology in the PR, then I will follow-up offline on preferred terminology once the PR has been reviewed.

Ping @dotnet/roslyn-analysis @dotnet/roslyn-compiler for reviews. #Resolved

mavasani · 2018-11-13T20:52:53Z

Ping @dotnet/roslyn-analysis @dotnet/roslyn-compiler for reviews - Note that this change only affects suppression for analyzer diagnostics, compiler diagnostics can only be suppressed with pragmas. #Resolved

jcouv · 2018-11-14T22:30:50Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs

            };

+        private static bool TryGetTargetScope(SuppressMessageInfo info, out TargetScope scope)
+        {
+            string scopeString = info.Scope != null ? info.Scope.ToLowerInvariant() : null;


info.Scope != null ? info.Scope.ToLowerInvariant() : null [](start = 33, length = 57)

nit: could be simplified to info.Scope?.ToLowerInvariant() #Resolved

jcouv · 2018-11-14T22:52:13Z

                    inImmediatelyContainingSymbol = false;

Not caused by your change, but I'm a little surprised by this.
I would have expected that we would set inImmediatelyContainingSymbol to false after the foreach, when declaredSymbols was not empty.
I think we set inImmediatelyContainingSymbol to false too early, for scenarios where a node declares multiple symbols. For example, with a multi-declaration field, all the declared symbols should be treated with the same inImmediatelyContainingSymbol (we shouldn't change it between declared symbols).

Consider adding a test and filing an issue if confirmed. #Resolved

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs:174 in 677d520. [](commit_id = 677d520, deletion_comment = False)

jcouv · 2018-11-14T22:54:41Z

src/Test/Utilities/Portable/Diagnostics/SuppressMessageAttributeTests.cs

+            await VerifyCSharpAsync(@"
+using System.Diagnostics.CodeAnalysis;
+
+[assembly: SuppressMessage(""Test"", ""Declaration"", Scope=""NamespaceAndChildren"", Target=""N.N1"")]


NamespaceAndChildren [](start = 62, length = 20)

Consider using varied casings for "NamespaceAndChildren" #Resolved

mavasani · 2018-11-14T22:56:18Z

                    inImmediatelyContainingSymbol = false;

Your observation is correct. However, the value of this flag is only used for namespace symbols, hence there is no observable effect with your proposed change. I will still make the suggested change as that is a more correct implementation.

In reply to: 438849151 [](ancestors = 438849151)

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs:174 in 677d520. [](commit_id = 677d520, deletion_comment = False)

jcouv

LGTM Thanks (iteration 1)

jaredpar · 2018-11-14T23:12:32Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs

            };

+        private static bool TryGetTargetScope(SuppressMessageInfo info, out TargetScope scope)
+        {
+            string scopeString = info.Scope != null ? info.Scope.ToLowerInvariant() : null;


Nit: could use info.Scope?.ToLowerInvariant() #Resolved

jaredpar · 2018-11-14T23:13:04Z

    private static readonly SmallDictionary<string, TargetScope> s_suppressMessageScopeTypes = new SmallDictionary<string, TargetScope>()

Why not use an invariant comparer here to avoid the ToLowerInvariant allocations below? #Resolved

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs:15 in 677d520. [](commit_id = 677d520, deletion_comment = False)

jaredpar

mavasani · 2018-11-14T23:50:41Z

I am going to retain the current terminology and file a documentation bug for the implications of the new scope

In reply to: 437902179 [](ancestors = 437902179)

mavasani · 2018-11-15T00:20:08Z

Filed documentation bug: https://github.com/dotnet/docs/issues/9046

mavasani · 2018-11-15T01:55:43Z

retest windows_release_vs-integration_prtest please

mavasani · 2018-11-15T15:03:32Z

Filed #31196 to finalize the naming for the new suppression scope.

ericbl · 2018-11-17T10:28:50Z

I arrive too late in the battle, but brainstorming on the topic yesterday (for Roslyn FxCop on .NET Standard library), is the new scope very necessary? What about NOT specifying the scope and extending the 'inclusive' strategy of project wide validity to the given target?

In the GlobalSuppresion file, if you don't specify neither scope not target, the SuppressionMessage is valid for the whole project.
We could imagine specifying only the target, but not the scope, and whatever the target is (e.g. namespace or type), the suppression message would apply for everything within this target and all descendants items, following the 'include all' strategy of the project wide SuppressionMessage.

Where does the scope 'type' exactly apply?
Think nested classes: are the sub classes considered in the scope 'type' or not?
If not, having a target without a scope could make the difference.
Or we need yet another scope 'typeAndChildren' here...

nelsonprsousa · 2018-12-13T14:59:44Z

This is just awesome, I am looking forward for it. When will it be available and how can I update to get it? Thanks!

Edit: I am not sure where this will be applied. Some Visual Studio version? .NET Framework version?

mavasani · 2018-12-16T15:08:58Z

We could imagine specifying only the target, but not the scope, and whatever the target is (e.g. namespace or type), the suppression message would apply for everything within this target and all descendants items, following the 'include all' strategy of the project wide SuppressionMessage.

@ericbl That seems like a very non-explicit approach and is not quite intuitive compared to an explicitly specified scope. Also it will be confusing when target is a type as the default behavior for types is to suppress it within everything under a type.

mavasani · 2018-12-16T15:15:13Z

This is just awesome, I am looking forward for it. When will it be available and how can I update to get it? Thanks!

@nelsonprsousa This feature should be available with Dev16.0 preview 2. Given that the primary use case here is suppression during CI, you need to ensure that compiler toolset is at least the one that ships with this preview or later. You can follow this page for preview releases: https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes-preview

nelsonprsousa · 2018-12-16T15:33:20Z

This is just awesome, I am looking forward for it. When will it be available and how can I update to get it? Thanks!

@nelsonprsousa This feature should be available with Dev16.0 preview 2. Given that the primary use case here is suppression during CI, you need to ensure that compiler toolset is at least the one that ships with this preview or later. You can follow this page for preview releases: https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes-preview

You guys rock!

mavasani · 2018-12-20T12:59:13Z

FYI: The new scope has been renamed to NamespaceAndDescendants (#31940)

mavasani added Area-Analyzers Area-Compilers labels Nov 10, 2018

mavasani added this to the 16.0.P2 milestone Nov 10, 2018

mavasani requested review from jinujoseph and a team November 10, 2018 00:18

mavasani mentioned this pull request Nov 10, 2018

Provide ability to suppress issues for all types within a namespace #486

Closed

jcouv reviewed Nov 14, 2018

View reviewed changes

jcouv approved these changes Nov 14, 2018

View reviewed changes

jcouv self-assigned this Nov 14, 2018

jaredpar reviewed Nov 14, 2018

View reviewed changes

jaredpar approved these changes Nov 14, 2018

View reviewed changes

Address feedback

83b90db

jinujoseph added the Approved to merge label Nov 15, 2018

mavasani merged commit 8740f8b into dotnet:dev16.0-preview2 Nov 15, 2018

mavasani mentioned this pull request Nov 15, 2018

Naming for the new suppression scope #31196

Closed

sharwell mentioned this pull request Nov 16, 2018

CS1591 / SA1600 - disable for properties? DotNetAnalyzers/StyleCopAnalyzers#2765

Closed

mavasani mentioned this pull request Nov 20, 2018

Add documentation for new SuppressMessageAttribute scope dotnet/dotnet-api-docs#1101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new SuppressMessageAttribute target scope to suppress diagnosti… #31092

Add a new SuppressMessageAttribute target scope to suppress diagnosti… #31092

mavasani commented Nov 10, 2018 •

edited by jcouv

Loading

mavasani commented Nov 10, 2018 •

edited

Loading

josefpihrt commented Nov 12, 2018 •

edited by mavasani

Loading

mavasani commented Nov 12, 2018 •

edited

Loading

mavasani commented Nov 13, 2018 •

edited

Loading

jcouv Nov 14, 2018 •

edited by mavasani

Loading

jcouv commented Nov 14, 2018 •

edited by mavasani

Loading

jcouv Nov 14, 2018 •

edited by mavasani

Loading

mavasani commented Nov 14, 2018

jcouv left a comment

jaredpar Nov 14, 2018 •

edited by mavasani

Loading

jaredpar commented Nov 14, 2018 •

edited by mavasani

Loading

jaredpar left a comment

mavasani commented Nov 14, 2018

mavasani commented Nov 15, 2018

mavasani commented Nov 15, 2018

mavasani commented Nov 15, 2018

ericbl commented Nov 17, 2018 •

edited

Loading

nelsonprsousa commented Dec 13, 2018 •

edited

Loading

mavasani commented Dec 16, 2018

mavasani commented Dec 16, 2018

nelsonprsousa commented Dec 16, 2018

mavasani commented Dec 20, 2018

Add a new SuppressMessageAttribute target scope to suppress diagnosti… #31092

Add a new SuppressMessageAttribute target scope to suppress diagnosti… #31092

Conversation

mavasani commented Nov 10, 2018 • edited by jcouv Loading

mavasani commented Nov 10, 2018 • edited Loading

josefpihrt commented Nov 12, 2018 • edited by mavasani Loading

mavasani commented Nov 12, 2018 • edited Loading

mavasani commented Nov 13, 2018 • edited Loading

jcouv Nov 14, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

jcouv commented Nov 14, 2018 • edited by mavasani Loading

jcouv Nov 14, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani commented Nov 14, 2018

jcouv left a comment

Choose a reason for hiding this comment

jaredpar Nov 14, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

jaredpar commented Nov 14, 2018 • edited by mavasani Loading

jaredpar left a comment

Choose a reason for hiding this comment

mavasani commented Nov 14, 2018

mavasani commented Nov 15, 2018

mavasani commented Nov 15, 2018

mavasani commented Nov 15, 2018

ericbl commented Nov 17, 2018 • edited Loading

nelsonprsousa commented Dec 13, 2018 • edited Loading

mavasani commented Dec 16, 2018

mavasani commented Dec 16, 2018

nelsonprsousa commented Dec 16, 2018

mavasani commented Dec 20, 2018

mavasani commented Nov 10, 2018 •

edited by jcouv

Loading

mavasani commented Nov 10, 2018 •

edited

Loading

josefpihrt commented Nov 12, 2018 •

edited by mavasani

Loading

mavasani commented Nov 12, 2018 •

edited

Loading

mavasani commented Nov 13, 2018 •

edited

Loading

jcouv Nov 14, 2018 •

edited by mavasani

Loading

jcouv commented Nov 14, 2018 •

edited by mavasani

Loading

jcouv Nov 14, 2018 •

edited by mavasani

Loading

jaredpar Nov 14, 2018 •

edited by mavasani

Loading

jaredpar commented Nov 14, 2018 •

edited by mavasani

Loading

ericbl commented Nov 17, 2018 •

edited

Loading

nelsonprsousa commented Dec 13, 2018 •

edited

Loading