Add compiler support for UnconditionalSuppressMessage #51792

agocke · 2021-03-11T02:11:25Z

Fixes #48885

Fixes dotnet#48885

src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs

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

AlekseyTs · 2021-03-16T16:28:38Z

src/EditorFeatures/Test/Diagnostics/SuppressMessageAttributeWorkspaceTests.cs

+        public string Justification { get; set; }
+    }
+}";
+            var mscorlibRef = new[] { TestBase.MscorlibRef };


TestBase.MscorlibRef [](start = 38, length = 20)

How do we know this is the right version to use? #Closed

I think this should be OK regardless: all 4.x+ corlibs should be unified, and I'm only using AttributeUsage, Attribute, and string. Those should always be present.

I looked at trying to find the references used by the workspaces layer, but the TestWorkspace was confusing and I couldn't figure out a good way to keep them in-sync.

src/EditorFeatures/Test/Diagnostics/SuppressMessageAttributeWorkspaceTests.cs

src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs

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

AlekseyTs · 2021-03-18T16:16:10Z

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

+            {
+                if (!_lazyUnconditionalSuppressMessageAttribute.HasValue)
+                {
+                    _lazyUnconditionalSuppressMessageAttribute = new(_compilation.GetTypeByMetadataName("System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute"));


_lazyUnconditionalSuppressMessageAttribute [](start = 20, length = 42)

Is this assignment thread-safe? #Closed

I think so, in the specific way I was using it -- but rather than guess I just changed it to something I'm certain is safe.

I think so, in the specific way I was using it

It looks like Optional is a struct with several fields, the assignment is not guaranteed to be atomic. What am I missing?

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

You're right, I the previous version wasn't thread safe. The path would be:

Thread 1:
_field.HasValue is false
retrieve Type
_field.HasValue <- true
--- interrupted here

Thread 2 (continuing at interruption):
_field.HasValue is true
return value is null (not correct)

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

AlekseyTs · 2021-03-18T16:22:21Z

Done with review pass (commit 4) #Closed

AlekseyTs · 2021-03-18T18:17:20Z

/azp run

azure-pipelines · 2021-03-18T18:17:40Z

Azure Pipelines successfully started running 3 pipeline(s).

AlekseyTs

LGTM (commit 5), assuming CI is passing

agocke · 2021-03-18T18:34:48Z

Looks like there was an infra break -- I don't think these were caused by my changes.

RikkiGibson

Looks good, just had a few small questions.

Is @dotnet/roslyn-ide sign-off needed for the EditorFeatures test changes?

RikkiGibson · 2021-03-18T20:20:05Z

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

+        private ISymbol? UnconditionalSuppressMessageAttribute
+        {
+            get
+            {


Just checking my understanding: is the purpose of this StrongBox just to allow us to distinguish these cases:

_lazySuppressMessageAttribute is not initialized

_lazySuppressMessageAttribute is initialized, and its value is null

_lazySuppressMessageAttribute is initialized, and its value is non-null

Correct, although we don't really need to care whether the value is null or non null, just whether or not it's initialized.

RikkiGibson · 2021-03-18T20:25:10Z

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

-            var attributes = symbol.GetAttributes().Where(a => a.AttributeClass == this.SuppressMessageAttribute);
+            var attributes = symbol.GetAttributes().Where(a =>
+                a.AttributeClass == this.SuppressMessageAttribute
+                || a.AttributeClass == this.UnconditionalSuppressMessageAttribute);
            DecodeGlobalSuppressMessageAttributes(compilation, symbol, globalSuppressions, attributes);


Could this be called with an argument with a null AttributeClass? I'm wondering if this API could give strange answers in the event that either SuppressMessageAttribute or UnconditionalSuppressMessageAttribute is null, and a.AttributeClass is null.

Heh, yeah I suppose that's possible. I think that's an existing bug. I'd rather not hold up the PR for it if you don't mind, but I can file a bug about it.

(I don't think it semantically matters because the diagnostic is only suppressed if the ID's match, but if the attribute doesn't exist, we won't be able to fetch the ID and they will never match).

Filed #51972

Sounds good.

agocke · 2021-03-19T01:42:33Z

@sharwell Anything else? I'd like to get this in for Preview 3 (snaps tomorrow)

sharwell · 2021-03-19T06:47:09Z

Send me a message tomorrow on teams so I don't miss it

sharwell

Looks fine with the understanding that this will break for cases where a dependency declares a second copy of the attribute as internal (even if there is no IVT access to it).

RikkiGibson · 2021-03-19T19:01:22Z

Are we good to merge this? @agocke do you have a preference for squash merge or merge commit?

agocke · 2021-03-19T19:34:44Z

Squashed, thanks for the quick review everyone!

Add compiler support for UnconditionalSuppressMessage

9cfdf63

Fixes dotnet#48885

dotnet-issue-labeler bot added the Area-Compilers label Mar 11, 2021

Fix workpsace tests

98dea18

agocke marked this pull request as ready for review March 11, 2021 19:22

agocke requested review from a team as code owners March 11, 2021 19:22

AlekseyTs added Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 15, 2021

AlekseyTs reviewed Mar 16, 2021

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 16, 2021

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 16, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 16, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 16, 2021

View reviewed changes

src/EditorFeatures/Test/Diagnostics/SuppressMessageAttributeWorkspaceTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 16, 2021

View reviewed changes

src/EditorFeatures/Test/Diagnostics/SuppressMessageAttributeWorkspaceTests.cs Outdated Show resolved Hide resolved

AlekseyTs requested a review from a team March 16, 2021 16:32

RikkiGibson self-assigned this Mar 16, 2021

agocke added 2 commits March 16, 2021 18:16

Respond to PR comments

5c6818d

Fix formatting

f75f112

AlekseyTs reviewed Mar 18, 2021

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 18, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 18, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Mar 18, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/SuppressMessageAttributeState.cs Outdated Show resolved Hide resolved

Respond to PR comments

2220ba7

AlekseyTs approved these changes Mar 18, 2021

View reviewed changes

RikkiGibson reviewed Mar 18, 2021

View reviewed changes

agocke mentioned this pull request Mar 18, 2021

Diagnostic suppression checking in SuppressMessageAttributeState may do unnecessary work #51972

Open

RikkiGibson approved these changes Mar 18, 2021

View reviewed changes

sharwell approved these changes Mar 19, 2021

View reviewed changes

agocke merged commit 8cdc029 into dotnet:main Mar 19, 2021

ghost added this to the Next milestone Mar 19, 2021

agocke deleted the unconditional-suppress branch March 19, 2021 19:34

Youssef1313 mentioned this pull request Mar 19, 2021

Add support for UnconditionalSuppressMessageAttribute #43639

Closed

tlakollo mentioned this pull request Mar 26, 2021

Suppress warning if UnconditionalSuppressMessageAttribute is used dotnet/linker#1892

Closed

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compiler support for UnconditionalSuppressMessage #51792

Add compiler support for UnconditionalSuppressMessage #51792

agocke commented Mar 11, 2021

AlekseyTs Mar 16, 2021 •

edited

Loading

agocke Mar 17, 2021

AlekseyTs Mar 18, 2021 •

edited

Loading

agocke Mar 18, 2021

AlekseyTs Mar 18, 2021

agocke Mar 18, 2021

AlekseyTs commented Mar 18, 2021 •

edited

Loading

AlekseyTs commented Mar 18, 2021

azure-pipelines bot commented Mar 18, 2021

AlekseyTs left a comment

agocke commented Mar 18, 2021

RikkiGibson left a comment

RikkiGibson Mar 18, 2021

agocke Mar 18, 2021

RikkiGibson Mar 18, 2021

agocke Mar 18, 2021

agocke Mar 18, 2021

agocke Mar 18, 2021

RikkiGibson Mar 18, 2021

agocke commented Mar 19, 2021

sharwell commented Mar 19, 2021

sharwell left a comment

RikkiGibson commented Mar 19, 2021

agocke commented Mar 19, 2021 •

edited

Loading

Add compiler support for UnconditionalSuppressMessage #51792

Add compiler support for UnconditionalSuppressMessage #51792

Conversation

agocke commented Mar 11, 2021

AlekseyTs Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 18, 2021 • edited Loading

AlekseyTs commented Mar 18, 2021

azure-pipelines bot commented Mar 18, 2021

AlekseyTs left a comment

Choose a reason for hiding this comment

agocke commented Mar 18, 2021

RikkiGibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Mar 19, 2021

sharwell commented Mar 19, 2021

sharwell left a comment

Choose a reason for hiding this comment

RikkiGibson commented Mar 19, 2021

agocke commented Mar 19, 2021 • edited Loading

AlekseyTs Mar 16, 2021 •

edited

Loading

AlekseyTs Mar 18, 2021 •

edited

Loading

AlekseyTs commented Mar 18, 2021 •

edited

Loading

agocke commented Mar 19, 2021 •

edited

Loading