Validate diagnostic ID in Experimental attribute by jcouv · Pull Request #70397 · dotnet/roslyn

jcouv · 2023-10-16T18:08:02Z

Corresponding spec update: dotnet/csharplang#7597

Relates to #68702
Relates to dotnet/runtime#85444

jcouv · 2023-10-17T17:17:37Z

Moved out to 17.9

cston · 2023-10-17T21:49:11Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs


        comp.VerifyDiagnostics(
-            // 0.cs(1,1): error CS9204: 'C' is for evaluation purposes only and is subject to change or removal in future updates.
+            // 0.cs(1,1): error CS9204: 'C' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.


error CS9204

Is WRN_Experimental ever reported as a warning? If so, are we testing that case? #Resolved

I've added some command-line tests to configure the severity on various diagnostics

cston · 2023-10-17T21:50:40Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

+    extends [mscorlib]System.Object
+{
+    // Experimental("Diag 01")
+    .custom instance void System.Diagnostics.CodeAnalysis.ExperimentalAttribute::.ctor(string) = ( 01 00 07 44 69 61 67 20 30 31 00 00 )


( 01 00 07 44 69 61 67 20 30 31 00 00 )

Consider using = { string('Diag 01') } instead. #Resolved

cston · 2023-10-17T21:53:13Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

+    {
+        .get instance string System.Diagnostics.CodeAnalysis.ExperimentalAttribute::get_UrlFormat()
+        .set instance void System.Diagnostics.CodeAnalysis.ExperimentalAttribute::set_UrlFormat(string)
+    }


Consider removing if not needed, including backing field declaration. #Resolved

AlekseyTs · 2023-10-18T16:20:30Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>Expected interpolated string</value>
  </data>
+  <data name="ERR_InvalidExperimentalDiagID" xml:space="preserve">
+    <value>The argument to the 'Experimental' attribute must be a valid identifier</value>


The argument

"The first argument"? The spec implies that there could be more than one argument and the restriction is for the first one. #Closed

AlekseyTs · 2023-10-18T16:28:00Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

+    public void NullDiagnosticId()
    {
        var libSrc = """
 [System.Diagnostics.CodeAnalysis.Experimental(null)]


null

I think it would be good to test a scenario:

without an argument

the argument isn't a string. An integer literal 1, for example
#Closed

AlekseyTs · 2023-10-18T16:30:02Z

src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs

+            if (arguments.Attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute)
+                && !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))
+            {
+                arguments.Diagnostics.DiagnosticBag.Add(ErrorCode.ERR_InvalidExperimentalDiagID, arguments.AttributeSyntaxOpt.Location);


arguments.AttributeSyntaxOpt.Location

I think we have a helper that finds syntax for specific argument when there is one #Closed

AlekseyTs · 2023-10-18T16:35:37Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

+    public void DiagnosticIdWithTrailingNewline()
    {
        var libSrc = """
 [System.Diagnostics.CodeAnalysis.Experimental("Diag\n")]


("Diag\n")

Are we testing with more than one argument? #Closed

AlekseyTs · 2023-10-18T16:36:21Z

Done with review pass (commit 2)

cston · 2023-10-18T21:22:47Z

src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs

+            if (arguments.Attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute)
+                && !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))
+            {
+                Debug.Assert(arguments.AttributeSyntaxOpt is not null);


Debug.Assert(arguments.AttributeSyntaxOpt is not null);

This is already asserted earlier. #Resolved

AlekseyTs · 2023-10-19T13:46:36Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+""");
+            var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
+[*.cs]
+dotnet_diagnostic.CS9208.severity = warning


Does this have any effect on the scenario? If yes, then consider including observations without this line present. If no, consider removing. #Closed

Or did you mean to attempt to suppress CS9211 instead?

Yes, should have been CS9211. Thanks

AlekseyTs · 2023-10-19T13:56:44Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+""");
+            var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
+[*.cs]
+dotnet_diagnostic.CS9204.severity = warning


CS9204

Consider asserting that the number corresponds to WRN_Experimental #Closed

AlekseyTs · 2023-10-19T14:02:36Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

        var src = """
 C.M();
+
+[System.Diagnostics.CodeAnalysis.Experimental("DiagID", "other")]


DiagID

Consider testing what happens if this value is not a valid identifier #Closed

AlekseyTs · 2023-10-19T14:08:34Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs


        comp.VerifyDiagnostics(
-            // 0.cs(1,1): error CS9204: 'C' is for evaluation purposes only and is subject to change or removal in future updates.
+            // 0.cs(1,1): error CS9204: 'C' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.


error CS9204

What happens if we try to suppress this diagnostic with pragma? #Closed

AlekseyTs · 2023-10-19T14:11:18Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

+        var comp = CreateCompilation(new[] { src, libSrc, experimentalAttributeSrc });
+
+        comp.VerifyDiagnostics(
+            // 0.cs(1,1): error Diag\n: 'C' is for evaluation purposes only and is subject to change or removal in future updates.Suppress this diagnostic to proceed.


// 0.cs(1,1): error Diag\n: 'C' is for evaluation purposes only and is subject to change or removal in future updates.Suppress this diagnostic to proceed.

What happens if we try to suppress CS9204 with a pragma? #Closed

AlekseyTs · 2023-10-19T17:25:31Z

src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs

+    public void WhitespaceDiagnosticId_WithSuppression()
+    {
+        var src = """
+#pragma warning disable CS9204


The effect of the pragma feels unexpected to me. However, this isn't primarily related to the goal of the PR. Therefore, not blocking #Resolved

What's unexpected? We have a diagnostic that we're going to report as CS9204 and we suppress CS9204, therefore the diagnostic is suppressed.

What's unexpected?

Errors are usually not suppressable

The spec this is not an error, it is merely reported as an error

AlekseyTs · 2023-10-19T17:28:51Z

        // 0.cs(1,1): error DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.

It would be good to test if we can suppress this error with pragma. Not blocking. #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs:58 in 5d1c56d. [](commit_id = 5d1c56d, deletion_comment = False)

AlekseyTs

LGTM (commit 8)

AlekseyTs · 2023-10-19T17:31:04Z

Is this check already implemented for VB? #Resolved

jcouv · 2023-10-19T21:15:17Z

Added VB implementation

Added VB

jcouv · 2023-10-19T21:18:00Z

        // 0.cs(1,1): error DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.

I think that's already covered by test Suppressed at line 2031

In reply to: 1771422464

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/ExperimentalAttributeTests.cs:58 in 5d1c56d. [](commit_id = 5d1c56d, deletion_comment = False)

cston · 2023-10-19T23:04:33Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

+        End Sub
+
+        <Fact>
+        Public Sub ExperimentalWithValidDiagnosticID_WarnForDiagID()


WarnForDiagID

"WarnForExperimental"? #Resolved

This test has dotnet_diagnostic.DiagID.severity = warning

AlekseyTs · 2023-10-20T17:13:02Z

src/Compilers/VisualBasic/Portable/Symbols/Symbol_Attributes.vb

            ElseIf arguments.Attribute.IsTargetAttribute(Me, AttributeDescription.RequiredMemberAttribute) Then
                DirectCast(arguments.Diagnostics, BindingDiagnosticBag).Add(ERRID.ERR_DoNotUseRequiredMember, arguments.AttributeSyntaxOpt.Location)
+            ElseIf arguments.Attribute.IsTargetAttribute(Me, AttributeDescription.ExperimentalAttribute) AndAlso
+                Not SyntaxFacts.IsValidIdentifier(DirectCast(arguments.Attribute.CommonConstructorArguments(0).ValueInternal, String)) Then


Consider using a nested if instead. So that we wouldn't keep checking for other well-known attributes once we run into the Experimental attribute. We might add checks for other attributes below in the future. #Closed

AlekseyTs · 2023-10-20T17:20:07Z

src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs

            }

+            if (arguments.Attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute)
+                && !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))


&& !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))

Similar refactoring suggestion as for VB #Closed

On the second thought, never mind. It looks like we do want to call DecodeWellKnownAttributeImpl below.

AlekseyTs · 2023-10-20T17:26:35Z

src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs

+            {
+                var attrArgumentLocation = arguments.Attribute.GetAttributeArgumentSyntaxLocation(parameterIndex: 0, arguments.AttributeSyntaxOpt);
+                arguments.Diagnostics.DiagnosticBag.Add(ErrorCode.ERR_InvalidExperimentalDiagID, attrArgumentLocation);
+                return;


return

Shouldn't we still call DecodeWellKnownAttributeImpl below? #Closed

I don't think so. The decoding of experimental attribute is done in EarlyDecodeWellKnownAttributes, before we get to DecodeWellKnownAttribute

I don't think so. The decoding of experimental attribute is done in EarlyDecodeWellKnownAttributes, before we get to DecodeWellKnownAttribute

First, the code should be robust to future changes and it feels reasonable to me that the fact you are referring to, if it is a fact, won't hold in the future.

Second, I actually see decoding done in DecodeWellKnownAttributeImpl for a SourceModuleSymbol today, for example.

Yes, you're right. The return broke things.

AlekseyTs · 2023-10-20T17:28:34Z

Done with review pass (commit 9)

Updated code

AlekseyTs

LGTM (commit 11), assuming CI is passing.

jcouv added the Area-Compilers label Oct 16, 2023

jcouv self-assigned this Oct 16, 2023

ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 16, 2023

Validate diagnostic ID in Experimental attribute

a7ff4a2

jcouv force-pushed the exp-validation branch from 8488e97 to a7ff4a2 Compare October 16, 2023 18:25

fix test

c4534ef

jcouv marked this pull request as ready for review October 17, 2023 15:57

jcouv requested a review from a team as a code owner October 17, 2023 15:57

jcouv added this to the 17.9 milestone Oct 17, 2023

jcouv changed the base branch from release/dev17.8 to main October 17, 2023 17:17

cston reviewed Oct 17, 2023

View reviewed changes

AlekseyTs reviewed Oct 18, 2023

View reviewed changes

Julien Couvreur added 2 commits October 18, 2023 11:53

Address feedback

75e7b28

Merge remote-tracking branch 'dotnet/main' into exp-validation

3bdd89b

cston reviewed Oct 18, 2023

View reviewed changes

cston previously approved these changes Oct 18, 2023

View reviewed changes

Julien Couvreur added 2 commits October 18, 2023 16:14

Remove duplicate assertion

6ad3134

update test

7a9753e

AlekseyTs reviewed Oct 19, 2023

View reviewed changes

AlekseyTs previously approved these changes Oct 19, 2023

View reviewed changes

VB

c4f5f8f

jcouv requested a review from cston October 19, 2023 21:45

cston reviewed Oct 19, 2023

View reviewed changes

cston previously approved these changes Oct 19, 2023

View reviewed changes

AlekseyTs reviewed Oct 20, 2023

View reviewed changes

Julien Couvreur added 2 commits October 20, 2023 10:42

tweaks

caaf149

Avoid using AttributeData in PE symbols

43ea3e5

jcouv requested review from AlekseyTs and cston October 20, 2023 19:44

AlekseyTs approved these changes Oct 20, 2023

View reviewed changes

cston approved these changes Oct 20, 2023

View reviewed changes

jcouv merged commit e027092 into dotnet:main Oct 20, 2023

jcouv deleted the exp-validation branch October 20, 2023 21:05

ghost modified the milestones: 17.9, Next Oct 20, 2023

allisonchou mentioned this pull request Oct 24, 2023

[Automated] PRs inserted in VS build main-34224.107 #70544

Closed

jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023

Conversation

jcouv commented Oct 16, 2023

Uh oh!

jcouv commented Oct 17, 2023

Uh oh!

cston Oct 17, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Oct 17, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Oct 17, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 18, 2023

Uh oh!

cston Oct 18, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 19, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

cston Oct 17, 2023 •

edited by jcouv

Loading

cston Oct 17, 2023 •

edited by jcouv

Loading

cston Oct 17, 2023 •

edited by jcouv

Loading

AlekseyTs Oct 18, 2023 •

edited

Loading

AlekseyTs Oct 18, 2023 •

edited

Loading

AlekseyTs Oct 18, 2023 •

edited

Loading

AlekseyTs Oct 18, 2023 •

edited

Loading

cston Oct 18, 2023 •

edited by jcouv

Loading

AlekseyTs Oct 19, 2023 •

edited

Loading

AlekseyTs Oct 19, 2023 •

edited

Loading

AlekseyTs Oct 19, 2023 •

edited

Loading

AlekseyTs Oct 19, 2023 •

edited

Loading

AlekseyTs Oct 19, 2023 •

edited

Loading

AlekseyTs Oct 19, 2023 •

edited by jcouv

Loading

AlekseyTs commented Oct 19, 2023 •

edited

Loading

AlekseyTs commented Oct 19, 2023 •

edited by jcouv

Loading

cston Oct 19, 2023 •

edited by jcouv

Loading

AlekseyTs Oct 20, 2023 •

edited

Loading

AlekseyTs Oct 20, 2023 •

edited

Loading

AlekseyTs Oct 20, 2023 •

edited

Loading