Add support for `System.Diagnostics.CodeAnalysis.ExperimentalAttribute` by jcouv · Pull Request #68702 · dotnet/roslyn

jcouv · 2023-06-20T22:49:00Z

This can be reviewed commit-by-commit. The second commit has renames.

Relates to https://github.com/dotnet/designs/blob/main/accepted/2023/preview-apis/preview-apis.md#compiler-behavior
Relates to dotnet/runtime#85444

The PR does not include support for assembly/module targets, but there may still be some discussion on that (dotnet/runtime#85444 (comment)).
Update: we'll keep assembly/module out-of-scope for now, we can revisit later. (comment)

jjonescz · 2023-06-22T07:55:18Z

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

+
+                    // Provide an explicit format for fully-qualified type names.
+                    return new CustomObsoleteDiagnosticInfo(MessageProvider.Instance, (int)ErrorCode.WRN_Experimental,
+                        data, new FormattedSymbol(symbol, SymbolDisplayFormat.CSharpErrorMessageFormat));


Is FormattedSymbol necessary? Isn't CSharpErrorMessageFormat the default? #Resolved

Simplified. Thanks

jjonescz · 2023-06-22T08:04:58Z

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

+            : CreateCompilation(src, references: new[] { CreateCompilation(new[] { libSrc, experimentalAttributeSrc }).EmitToImageReference() });
+
+        comp.VerifyDiagnostics(
+            // (1,1): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.


https://github.com/dotnet/designs/blob/main/accepted/2023/preview-apis/preview-apis.md#compiler-behavior says "The severity is always error." but here it's a warning. #Resolved

That looks like a spec bug. I'll let them know.
An error wouldn't work, as it would completely disallow use of the experimental API, which defeats the purpose. Errors can't be suppressed.
That doc also says any use will generate a diagnostic and the caller is expected to suppress it, with the usual means (i.e. #pragma or project-wide NoWarn).

Pinged to fix the design doc at dotnet/designs@183cee7#r119278540

jjonescz · 2023-06-22T08:05:47Z

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_Experimental.cs

 namespace Microsoft.CodeAnalysis.CSharp.UnitTests
 {
-    public class AttributeTests_Experimental : CSharpTestBase
+    public class AttributeTests_WindowsExperimental : CSharpTestBase


Also rename the file?

I was intentionally not planning to rename the file. That unfortunately degrades the history and doesn't seem critical for this case.

History is only degraded when the changes to the file exceed the threshold of similarity. This one-line change would be detected by Git as a rename and features like blame would not be impacted.

Note that the rename can be implemented in a separate commit to ensure Git tracks the rename correctly. The squash-merge anti-feature works hard to undermine core Git functionality, but again this file would not exceed the standard similarity threshold.

Do you have an example where blame on GitHub shows the proper history after a file rename?
I've seen git show a rename locally, but never saw blame work.

jjonescz · 2023-06-22T08:22:15Z

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

+            // (1,1): warning DiagID: 'C' is for evaluation purposes only and is subject to change or removal in future updates.
+            // C.M();
+            Diagnostic("DiagID", "C").WithArguments("C").WithLocation(1, 1)
+            );


Consider verifying the Help URL is the default one here:

Assert.Equal(DefaultHelpLinkUri, diag.Descriptor.HelpLinkUri); ``` #Resolved

jjonescz · 2023-06-22T08:23:13Z

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

+""";
+
+        var src = """
+#pragma warning disable DiagID1


Consider testing suppression using SuppressMessageAttribute. #Resolved

Good idea. But it looks like SuppressMessageAttribute doesn't work for compiler warnings, only analyzer warnings.

jjonescz · 2023-06-22T08:23:55Z

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

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


Did you mean an empty string here? Also consider testing a space only string, e.g., " ", "\n".

Suggested change

[System.Diagnostics.CodeAnalysis.Experimental(null)]

[System.Diagnostics.CodeAnalysis.Experimental("")]

``` #Resolved

jjonescz · 2023-06-22T08:24:54Z

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

+    }
+
+    [Fact]
+    public void EmptyDiagnosticId()


Also consider testing a diagnostic ID with a space, e.g., "DIAG 01". Not sure if that's invalid here, but it is invalid in source generators and diagnostic analyzers. #Resolved

Good catch. There's a check in DiagnosticDescriptor that disallows whitespaces.

jcouv · 2023-06-22T21:15:44Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-06-23T18:26:26Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-06-26T14:37:00Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-06-27T20:31:07Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-06-30T04:42:18Z

@dotnet/roslyn-compiler for second review (~~candidate for 17.7~~, never mind, moved out to 17.8). Thanks

cston · 2023-07-03T18:16:04Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs

                    throw ExceptionUtilities.UnexpectedValue(kind);
            }
+
+            ObsoleteAttributeData decodeExperimentalAttribute()


decodeExperimentalAttribute

Consider using a regular method, rather than a local function, for consistency with the other cases in the switch above. #Closed

cston · 2023-07-03T21:41:53Z

src/Compilers/VisualBasic/Portable/Symbols/Source/QuickAttributeChecker.vb

            ElseIf Matches(name, inAttribute, AttributeDescription.DeprecatedAttribute) Then
                result = result Or QuickAttributes.Obsolete
+            ElseIf Matches(name, inAttribute, AttributeDescription.WindowsExperimentalAttribute) Then
+                result = result Or QuickAttributes.Obsolete


It looks like the WindowsExperimentalAttribute case and the ExperimentalAttribute case are identical. Consider dropping one. #Resolved

Many of the cases in here are identical. I'd rather stick to the existing structure.

cston · 2023-07-03T21:43:27Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceModuleSymbol.vb

            checker.AddName(AttributeDescription.CaseInsensitiveExtensionAttribute.Name, QuickAttributes.Extension)
            checker.AddName(AttributeDescription.ObsoleteAttribute.Name, QuickAttributes.Obsolete)
            checker.AddName(AttributeDescription.DeprecatedAttribute.Name, QuickAttributes.Obsolete)
+            checker.AddName(AttributeDescription.WindowsExperimentalAttribute.Name, QuickAttributes.Obsolete)


checker.AddName(AttributeDescription.WindowsExperimentalAttribute.Name, QuickAttributes.Obsolete)

This looks identical to the case below. Consider dropping this. #Resolved

cston · 2023-07-03T21:45:11Z

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceNamedTypeSymbol.vb

                                Case AttributeDescription.CaseInsensitiveExtensionAttribute.Name,
                                     AttributeDescription.ObsoleteAttribute.Name,
                                     AttributeDescription.DeprecatedAttribute.Name,
+                                     AttributeDescription.WindowsExperimentalAttribute.Name,


AttributeDescription.WindowsExperimentalAttribute.Name,

Identical to the case below. Consider dropping. #Resolved

cston · 2023-07-03T21:46:12Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests.vb

 ]]></expected>)
        End Sub
+
+        Private ReadOnly experimentalAttributeCSharpSrc As String = "


ReadOnly

Const or Shared ReadOnly #Closed

cston · 2023-07-03T21:50:16Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests.vb

+            Dim diag = comp.GetDiagnostics().Single()
+            Assert.Equal("DiagID1", diag.Id)
+            Assert.Equal(ERRID.WRN_Experimental, diag.Code)
+            Assert.Equal("https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(BC42380)", diag.Descriptor.HelpLinkUri)


https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(BC42380)

Similar to C#, the link reports this error is unrecognized currently. Is that expected?

Tagging @BillWagner here as well (this is an existing diagnostic code)

cston · 2023-07-03T21:52:08Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests.vb

+
+            comp.AssertTheseDiagnostics(
+<expected><![CDATA[
+DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.


in future updates.

From C#, the URI was included in the message. Should the URI be included in the VB message too? Same question for test below. #Resolved

I didn't follow. Here's what a C# diagnostic looks like:

// (1,1): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates. // C.M(); Diagnostic("DiagID1", "C").WithArguments("C").WithLocation(1, 1)

Both C# and VB are re-using the existing error codes and messages from the Windows Experimental attribute. If you think we should include the URI in there, consider filing an issue.

The error comment in the C# test UrlFormat() includes the URI. Is that comment correct?

Oh, I see what you mean. In the case where there is a UrlFormat, the C# message does include the URI (copied below).
This is existing behavior from the Windows.Experimental attribute. I'll leave as-is.

// 0.cs(1,1): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates. ([https://example.org/DiagID1](https://example.org/DiagID1)) // C.M(); Diagnostic("DiagID1", "C").WithArguments("C").WithLocation(1, 1)

Is that comment correct?

Yes. The logic comes from FormatHelpLinkUri

cston · 2023-07-03T21:53:56Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests.vb

+            Assert.Equal("DiagID1", diag.Id)
+            Assert.Equal(ERRID.WRN_Experimental, diag.Code)
+            Assert.Equal("https://example.org/DiagID1", diag.Descriptor.HelpLinkUri)
+        End Sub


End Sub

Consider testing:

experimental symbols are reported with fully-qualifed names

reference to symbol marked <Experimental><Obsolete(True)> #Resolved

cston · 2023-07-06T01:00:00Z

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

+            comp.VerifyDiagnostics(
+                // 0.cs(3,12): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.
+                //     void M(C c)
+                Diagnostic("DiagID1", "C").WithArguments("C").WithLocation(3, 12)


Diagnostic("DiagID1", "C").WithArguments("C").WithLocation(3, 12)

Should we report an obsolete error from source as well? #WontFix

This is existing behavior from the Windows.Experimental attribute.
I'll leave as-is.

cston · 2023-07-06T01:01:09Z

src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests.vb

+
+            comp.AssertTheseDiagnostics(
+<expected><![CDATA[
+DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.


DiagID1: 'C'

Should we report an obsolete error instead? #WontFix

Answered elsewhere

cston · 2023-07-06T01:48:25Z

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

+""";
+        var comp = CreateCompilation(new[] { src, experimentalAttributeSrc });
+        comp.VerifyDiagnostics(
+            // 0.cs(1,1): warning DiagID1: 'C' is for evaluation purposes only and is subject to change or removal in future updates.


'C'

The name may not be fully-qualified. I believe the Diagnostic(...) item below may be using a different display format than the comment, and the comment appears to match what is reported in command-line errors. #Resolved

Indeed. Thanks for catching that

jcouv added the Area-Compilers label Jun 20, 2023

jcouv added this to the 17.7 milestone Jun 20, 2023

jcouv self-assigned this Jun 20, 2023

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

jcouv force-pushed the new-attr branch from 840f10e to 58b3d43 Compare June 21, 2023 00:17

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 21, 2023

Julien Couvreur added 2 commits June 20, 2023 22:35

Add support for ExperimentalAttribute

58f7312

Rename

eb8b887

jcouv force-pushed the new-attr branch from 58b3d43 to eb8b887 Compare June 21, 2023 05:36

Add test

4b36e8b

jcouv force-pushed the new-attr branch from 0ee85b2 to 4b36e8b Compare June 21, 2023 07:27

jcouv marked this pull request as ready for review June 21, 2023 07:28

jcouv requested a review from a team as a code owner June 21, 2023 07:28

formatting

2608051

jjonescz reviewed Jun 22, 2023

View reviewed changes

jcouv referenced this pull request in dotnet/designs Jun 22, 2023

Updating based on review

183cee7

Address feedback

1b61429

jcouv marked this pull request as draft June 22, 2023 20:47

jcouv marked this pull request as ready for review June 22, 2023 21:10

jcouv requested a review from jjonescz June 23, 2023 18:26

jjonescz approved these changes Jun 26, 2023

View reviewed changes

RussKie mentioned this pull request Jun 29, 2023

Setup default experimental URL dotnet/extensions#4130

Merged

jcouv modified the milestones: 17.7, 17.8 Jun 30, 2023

cston reviewed Jul 3, 2023

View reviewed changes

jcouv mentioned this pull request Jul 5, 2023

[Experimental] attribute should yield warnings, not errors dotnet/designs#297

Closed

Address feedback

1226862

jcouv requested a review from cston July 6, 2023 00:27

Address feedback

38dbb3f

cston reviewed Jul 6, 2023

View reviewed changes

Error format

bddcfb3

jcouv requested a review from cston July 6, 2023 02:18

cston approved these changes Jul 6, 2023

View reviewed changes

jcouv merged commit cd51ca0 into dotnet:main Jul 6, 2023

jcouv deleted the new-attr branch July 6, 2023 03:47

ghost modified the milestones: 17.8, Next Jul 6, 2023

jcouv mentioned this pull request Jul 20, 2023

Add test for Experimental usage in Experimental method #69131

Merged

allisonchou modified the milestones: Next, 17.8 P1 Jul 24, 2023

jcouv mentioned this pull request Aug 1, 2023

Add support for assembly/module-level [Experimental] and contextual suppression #69316

Merged

sharwell mentioned this pull request Aug 7, 2023

[build] Use compiler constant to toggle experimental public apis open-telemetry/opentelemetry-dotnet#4735

Merged

jcouv mentioned this pull request Oct 16, 2023

Validate diagnostic ID in Experimental attribute #70397

Merged

	[System.Diagnostics.CodeAnalysis.Experimental(null)]
	[System.Diagnostics.CodeAnalysis.Experimental("")]
	``` #Resolved

Conversation

jcouv commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjonescz Jun 22, 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!

jjonescz Jun 22, 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!

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!

jjonescz Jun 22, 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!

jjonescz Jun 22, 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!

jjonescz Jun 22, 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!

jjonescz Jun 22, 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!

jcouv commented Jun 22, 2023

Uh oh!

jcouv commented Jun 23, 2023

Uh oh!

jcouv commented Jun 26, 2023

Uh oh!

jcouv commented Jun 27, 2023

Uh oh!

jcouv commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jul 3, 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!

cston Jul 3, 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 Jul 3, 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 Jul 3, 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 Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jcouv commented Jun 20, 2023 •

edited

Loading

jjonescz Jun 22, 2023 •

edited by jcouv

Loading

jjonescz Jun 22, 2023 •

edited by jcouv

Loading

jjonescz Jun 22, 2023 •

edited by jcouv

Loading

jjonescz Jun 22, 2023 •

edited by jcouv

Loading

jjonescz Jun 22, 2023 •

edited by jcouv

Loading

jjonescz Jun 22, 2023 •

edited by jcouv

Loading

jcouv commented Jun 30, 2023 •

edited

Loading

cston Jul 3, 2023 •

edited

Loading

cston Jul 3, 2023 •

edited by jcouv

Loading

cston Jul 3, 2023 •

edited by jcouv

Loading

cston Jul 3, 2023 •

edited by jcouv

Loading

cston Jul 3, 2023 •

edited

Loading

cston Jul 3, 2023 •

edited by jcouv

Loading

cston Jul 3, 2023 •

edited by jcouv

Loading

cston Jul 6, 2023 •

edited by jcouv

Loading

cston Jul 6, 2023 •

edited by jcouv

Loading

cston Jul 6, 2023 •

edited by jcouv

Loading