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

jcouv · 2023-08-01T16:51:42Z

Follow-up on #68702 based on feedback from runtime team.
This PR makes two changes:

the [Experimental] attribute can be applied to assembly or module targets, and it will then apply to all contained types and members
we don't warn when using an experimental type/member and we are within an experimental context

jcouv · 2023-08-02T18:05:29Z

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

+
+            static ObsoleteDiagnosticKind getExperimentalDiagnosticKind(Symbol containingMember, bool forceComplete)
+            {
+                switch (GetObsoleteContextState(containingMember, forceComplete, getStateFromSymbol: static (symbol) => symbol.ExperimentalState))


📝 although I did not run into that situation, I'm re-using the existing mechanism that we use for delaying Obsolete diagnostics when some information isn't yet resolved.

jcouv · 2023-08-02T18:06:59Z

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

+            {
+                return namedTypeSymbol.Arity == 0
+                    && namedTypeSymbol.HasNameQualifier("System.Diagnostics.CodeAnalysis")
+                    && namedTypeSymbol.Name.Equals("ExperimentalAttribute", StringComparison.Ordinal);


📝 This is a poor man's cycle avoidance solution. This would treat any type of the right shape as an ExperimentalAttribute, not merely the well-known one... #Closed

jcouv · 2023-08-02T18:08:29Z

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

+            Return namedType IsNot Nothing AndAlso
+                namedType.Arity = 0 AndAlso
+                namedType.HasNameQualifier("System.Diagnostics.CodeAnalysis", StringComparison.Ordinal) AndAlso
+                namedType.Name.Equals("ExperimentalAttribute", StringComparison.Ordinal)


📝 I debated what kind of string comparison to use. On one hand, VB is more relaxed; on the other hand, this type is not defined by the VB language but by the BCL. I ended up opting for the stricter comparison, but feedback is welcome. #Closed

cston · 2023-08-03T19:32:54Z

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

+
+        #region ExperimentalAttribute
+        private ObsoleteAttributeData _obsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
+        public ObsoleteAttributeData ObsoleteAttributeData


ObsoleteAttributeData

Should the property be named ExperimentalAttributeData if this is intended specifically for that attribute? #Closed

cston · 2023-08-03T19:33:25Z

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

+
+        #region ExperimentalAttribute
+        private ObsoleteAttributeData _obsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
+        public ObsoleteAttributeData ObsoleteAttributeData


ObsoleteAttributeData

ExperimentalAttributeData? #Closed

cston · 2023-08-03T19:51:13Z

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

+            {
+                return namedTypeSymbol.Arity == 0
+                    && namedTypeSymbol.HasNameQualifier("System.Diagnostics.CodeAnalysis")
+                    && namedTypeSymbol.Name.Equals("ExperimentalAttribute", StringComparison.Ordinal);


&& namedTypeSymbol.Name.Equals("ExperimentalAttribute", StringComparison.Ordinal)

Consider checking Name before HasNameQualifier" since checking Name` may be cheaper and rule out mismatches more often. #Closed

cston · 2023-08-03T19:55:35Z

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

+            static bool isExperimentalSymbol(NamedTypeSymbol namedTypeSymbol)
+            {
+                return namedTypeSymbol.Arity == 0
+                    && namedTypeSymbol.HasNameQualifier("System.Diagnostics.CodeAnalysis")


namedTypeSymbol.HasNameQualifier("System.Diagnostics.CodeAnalysis")

Perhaps namedTypeSymbol.IsWellKnownDiagnosticsCodeAnalysisTopLevelType() since that method doesn't require splitting strings, and also checks the type is a top-level type in that namespace. #Closed

cston · 2023-08-03T20:05:09Z

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

+
+            static ObsoleteDiagnosticKind getExperimentalDiagnosticKind(Symbol containingMember, bool forceComplete)
+            {
+                switch (GetObsoleteContextState(containingMember, forceComplete, getStateFromSymbol: static (symbol) => symbol.ExperimentalState))


switch

It looks like the only difference between this switch and the one above is the lambda. Consider refactoring to remove the local function and share the switch (GetObsoleteContextState(...)) { } statement.

Func<Symbol, ThreeState> getStateFromSymbol; switch (symbol.ObsoleteKind) { ... } switch (GetObsoleteContextState(containingMember, forceComplete, getStateFromSymbol) { ... } ``` #Closed

cston · 2023-08-04T15:52:20Z

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

@@ -77,18 +80,779 @@ public class C
        var src = """
 C.M();
 """;
+        var comp = CreateCompilation(new[] { src, libSrc, experimentalAttributeSrc });
+        comp.VerifyDiagnostics();


VerifyDiagnostics()

Should there be an error reported for C in C.M()? Are we not reporting errors with experimental APIs within an assembly with an assembly-level [Experimental] attribute? #Resolved

Are we not reporting errors with experimental APIs within an assembly with an assembly-level [Experimental] attribute?

Correct. That's the second part of the change (described in OP)

cston · 2023-08-04T16:15:35Z

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

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


MissingAssemblyAndModule

Is this testing [Experimental] support? #Resolved

Same answer as for VB

cston · 2023-08-04T16:17:27Z

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

+        var derivedComp = CreateCompilation(derivedSrc, new[] { originalC.ToMetadataReference(), attrRef }, targetFramework: TargetFramework.Standard);
+        derivedComp.VerifyDiagnostics();
+
+        var comp = CreateCompilation("", new[] { derivedComp.ToMetadataReference(), retargetedC.ToMetadataReference() }, targetFramework: TargetFramework.Standard);


""

Consider including a reference to C, such as _ = new C();. #Resolved

Sorry, I guess the reference should have been to Derived to test we're recognizing the retargeting symbol as experimental.

cston · 2023-08-04T16:18:42Z

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

+        var derivedComp = CreateCompilation(@base, new[] { originalC.ToMetadataReference(), attrRef }, targetFramework: TargetFramework.Standard);
+        derivedComp.VerifyDiagnostics();
+
+        var comp = CreateCompilation("", new[] { derivedComp.ToMetadataReference(), retargetedC.ToMetadataReference() }, targetFramework: TargetFramework.Standard);


""

Consider including a reference to C. #Resolved

cston · 2023-08-04T16:26:42Z

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

@@ -85,7 +113,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
                    Return ObsoleteDiagnosticKind.Lazy
            End Select

-            Select Case GetObsoleteContextState(context, forceComplete)
+            Select Case GetObsoleteContextState(context, forceComplete, getStateFromSymbol:=Function(s) s.ObsoleteState)


GetObsoleteContextState

Please consider sharing the code with GetExperimentalDiagnosticKind() below. #Resolved

cston · 2023-08-04T16:30:54Z

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

+        Friend Overrides ReadOnly Property ObsoleteAttributeData As ObsoleteAttributeData
+            Get
+                Dim data = GetDecodedWellKnownAttributeData()
+                Return If(data IsNot Nothing, data.ExperimentalAttributeData, Nothing)


If(data IsNot Nothing, data.ExperimentalAttributeData, Nothing)

data?.ExperimentalAttributeData #Resolved

cston · 2023-08-04T16:33:33Z

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

+            Dim derivedComp = CreateCompilation(derivedSrc, references:={originalC.ToMetadataReference(), attrRef})
+            derivedComp.AssertNoDiagnostics()
+
+            Dim comp = CreateCompilation("", references:={derivedComp.ToMetadataReference(), retargetedC.ToMetadataReference(), attrRef})


""

Consider referencing C. #Resolved

cston · 2023-08-04T20:00:16Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs

+                    }
+                }
+
+                var lazyNetModuleAttributesBag = _lazyNetModuleAttributesBag;


_lazyNetModuleAttributesBag

Hmm, I'm surprised there is a _lazyNetModuleAttributesBag here rather than just delegating to the ModuleSymbol. That said, _lazyNetModuleAttributesBag wasn't added in this PR. #Resolved

cston · 2023-08-04T20:06:01Z

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

            Select Case symbol.ObsoleteKind
                Case ObsoleteAttributeKind.None
+                    If symbol.ContainingModule?.ObsoleteKind = ObsoleteAttributeKind.Uninitialized OrElse


symbol.ContainingModule?

We're allowing for ContainingModule and ContainingAssembly to be null here but not in C#. Is there a reason for the difference?

That's an existing difference. Here's the relevant comment on the ContainingModule and ContainingAssembly APIs (although they weren't super helpful for me to understand...):

''' Returns the module [assembly] containing this symbol. If this symbol is shared ''' across multiple modules [assemblies], or doesn't belong to a module [assembly], returns Nothing.

I don't know about the sharing scenario. Maybe that's a problem for [Experimental] (seems like we'd still want to enforce). I'll dig a bit more.

jcouv · 2023-08-08T00:09:33Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-08-08T18:04:51Z

@dotnet/roslyn-compiler for second review. Thanks

jjonescz · 2023-08-09T07:48:27Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs

+        /// <summary>
+        /// Returns data decoded from Obsolete attribute or null if there is no Obsolete attribute.
+        /// This property returns ObsoleteAttributeData.Uninitialized if attribute arguments haven't been decoded yet.
+        /// </summary>


Looks like this doc comment is duplicate of the base one, hence not needed.

Btw, should the base comment be updated to mention ExperimentalAttribute?

(Same for VB in multiple places.)

Suggested change

/// <summary>

/// Returns data decoded from Obsolete attribute or null if there is no Obsolete attribute.

/// This property returns ObsoleteAttributeData.Uninitialized if attribute arguments haven't been decoded yet.

/// </summary>

``` #Resolved

jjonescz · 2023-08-09T07:51:17Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs

+
+                if (GetAttributeDeclarations().IsEmpty)
+                {
+                    return null;


Why is this needed? #Resolved

This is an optimization. It allows not producing a lazy diagnostic when we know from syntax that there are no attributes.

jjonescz · 2023-08-09T07:57:02Z

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

+
+            if (symbol is MethodSymbol { MethodKind: MethodKind.Constructor } method && isExperimentalSymbol(method.ContainingType))
+            {
+                // Skip for constructors of System.Diagnostics.CodeAnalysis.ExperimentalAttribute to mitigate cycles


We don't need to skip for non-constructor methods? #Resolved

jjonescz · 2023-08-09T08:16:31Z

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

+            }
+
+            if (symbol.ContainingModule.ObsoleteKind is ObsoleteAttributeKind.Experimental
+                || symbol.ContainingAssembly.ObsoleteKind is ObsoleteAttributeKind.Experimental)


Why do we do this here? Couldn't we check containing member only when this symbol's obsolete kind is None? #Resolved

You're right, it's better to put there. That said, I don't think one should use both attributes (Obsolete and Experimental) :-P

jjonescz · 2023-08-09T08:19:35Z

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

+        Assert.Equal(ObsoleteAttributeKind.Experimental, comp.GetTypeByMetadataName("C").ContainingAssembly.ObsoleteKind);
+
+        // Note: the assembly-level [Experimental] is equivalent to marking every type and member as experimental,
+        // whereas a type-level [Experimental] is not equivalent to marking every nested type and member as experimental.


Is there a reason for this? Why not mark only types experimental when assembly is marked experimental? Otherwise members "inherit" obsolete/experimental state from assembly but not from containing type which feels slightly weird. #Resolved

I hope to confirm this in LDM next week, although I don't think we should block the PR on that. Some people have voiced the same preference as you.

For me, the meaning of [Experimental] on assembly is a shortcut that everything inside is marked as experimental.
The benefit of considering methods experimental too is that if we only mark types, then it's possible to use the methods without getting any warning:

library1 is marked with [assembly: Experimental] and has C.M()

library2 returns an instance of C from an API (C CreateC()) and suppresses the experimental warning

library3 does var c = CreateC(); c.M(); // we'd like to warn here, I think

The whole purpose of [assembly: Experimental] is that some contained symbols will inherit it from the assembly (otherwise the attribute has no effect). That is already a departure from how [Experimental] normally works. I'm not sure it's less weird to only inherit to types :-P

jjonescz · 2023-08-09T08:36:54Z

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

@@ -95,13 +97,38 @@ private static ThreeState GetObsoleteContextState(Symbol symbol, bool forceCompl

        internal static ObsoleteDiagnosticKind GetObsoleteDiagnosticKind(Symbol symbol, Symbol containingMember, bool forceComplete = false)
        {
+            if (symbol is NamedTypeSymbol namedTypeSymbol && isExperimentalSymbol(namedTypeSymbol))


Why do we need this cycle avoidance for ExperimentalAttribute but not for ObsoleteAttribute? #Resolved

Thanks for raising this. Turns out that now that ObsoleteAttributeData is lazy on assembly/module symbols, we no longer need this extra layer of cycle protection. I'll remove

jjonescz · 2023-08-09T15:18:39Z

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

@@ -481,7 +481,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
        End Property

        ''' <summary>
-        ''' Returns data decoded from Obsolete attribute or null if there is no Obsolete attribute.
+        ''' Returns data decoded from Obsolete/Experimental attribute or null if there is no Obsolete/Experimental attribute.


Can you update Symbol.cs similarly?

roslyn/src/Compilers/CSharp/Portable/Symbols/Symbol.cs

Lines 1343 to 1347 in 294cbb6

/// <summary>

/// Returns data decoded from <see cref="ObsoleteAttribute"/> attribute or null if there is no <see cref="ObsoleteAttribute"/> attribute.

/// This property returns <see cref="Microsoft.CodeAnalysis.ObsoleteAttributeData.Uninitialized"/> if attribute arguments haven't been decoded yet.

/// </summary>

internal abstract ObsoleteAttributeData ObsoleteAttributeData { get; }

#Resolved

cston · 2023-08-09T21:27:19Z

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

+                kind <> SymbolKind.Method AndAlso
+                kind <> SymbolKind.Event AndAlso
+                kind <> SymbolKind.Field AndAlso
+                kind <> SymbolKind.Property Then


Why check SymbolKind here and not in C#?

cston · 2023-08-09T21:29:41Z

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

+                    Dim moduleObsoleteKind As ObsoleteAttributeKind? = symbol.ContainingModule?.ObsoleteKind
+                    Dim assemblyObsoleteKind As ObsoleteAttributeKind? = symbol.ContainingAssembly?.ObsoleteKind
+
+                    If moduleObsoleteKind = Global.Microsoft.CodeAnalysis.ObsoleteAttributeKind.Experimental OrElse


Global.Microsoft.CodeAnalysis.

Are the namespace qualifiers needed?

jcouv · 2023-08-10T18:32:12Z

FYI @tarekgh, this was merged (for VS 17.8p2)

jcouv added the Area-Compilers label Aug 1, 2023

jcouv self-assigned this Aug 1, 2023

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Aug 1, 2023

build-analysis bot mentioned this pull request Aug 1, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jcouv force-pushed the exp-attr branch 2 times, most recently from 0812f9c to 8a1bc25 Compare August 2, 2023 17:51

jcouv added this to the 17.8 milestone Aug 2, 2023

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

Add support for assembly/module-level [Experimental]

2b800fb

jcouv force-pushed the exp-attr branch from 8a1bc25 to c0c1a09 Compare August 2, 2023 18:03

VB implementation

c8a0fa0

jcouv force-pushed the exp-attr branch from c0c1a09 to c8a0fa0 Compare August 2, 2023 18:03

jcouv commented Aug 2, 2023

View reviewed changes

jcouv marked this pull request as ready for review August 2, 2023 18:08

jcouv requested a review from a team as a code owner August 2, 2023 18:08

cston reviewed Aug 3, 2023

View reviewed changes

Address feedback

6669f0b

cston reviewed Aug 4, 2023

View reviewed changes

cston approved these changes Aug 4, 2023

View reviewed changes

Address feedback

b0ed1cd

RikkiGibson self-assigned this Aug 8, 2023

jjonescz reviewed Aug 9, 2023

View reviewed changes

Address feedback

afba201

jjonescz approved these changes Aug 9, 2023

View reviewed changes

jcouv added 3 commits August 9, 2023 08:44

Update xml doc

fba843c

Clarify null situation

c9d41c6

Only process known symbol kinds

7ad505e

jcouv requested a review from cston August 9, 2023 20:43

cston reviewed Aug 9, 2023

View reviewed changes

cston approved these changes Aug 9, 2023

View reviewed changes

simplify

9add30d

cston approved these changes Aug 9, 2023

View reviewed changes

jcouv enabled auto-merge (squash) August 9, 2023 22:54

jcouv merged commit 73689cc into dotnet:main Aug 10, 2023

ghost modified the milestones: 17.8, Next Aug 10, 2023

jcouv deleted the exp-attr branch August 10, 2023 05:40

allisonchou mentioned this pull request Aug 16, 2023

[Automated] PRs inserted in VS build main-34016.170 #69571

Closed

RikkiGibson mentioned this pull request Aug 17, 2023

Update compiler toolset to latest #69544

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

AlekseyTs mentioned this pull request Sep 28, 2023

StackoverflowException when compiling VB.NET project #70007

Closed

	/// <summary>
	/// Returns data decoded from <see cref="ObsoleteAttribute"/> attribute or null if there is no <see cref="ObsoleteAttribute"/> attribute.
	/// This property returns <see cref="Microsoft.CodeAnalysis.ObsoleteAttributeData.Uninitialized"/> if attribute arguments haven't been decoded yet.
	/// </summary>
	internal abstract ObsoleteAttributeData ObsoleteAttributeData { get; }

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

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

Conversation

jcouv commented Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

jcouv Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

cston Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

cston Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

cston Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

cston Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

cston Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Aug 4, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Aug 8, 2023

jcouv commented Aug 8, 2023

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Aug 9, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Aug 10, 2023

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

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

jcouv commented Aug 1, 2023 •

edited

Loading

jcouv Aug 2, 2023 •

edited

Loading

jcouv Aug 2, 2023 •

edited

Loading

cston Aug 3, 2023 •

edited

Loading

cston Aug 3, 2023 •

edited

Loading

cston Aug 3, 2023 •

edited

Loading

cston Aug 3, 2023 •

edited

Loading

cston Aug 3, 2023 •

edited

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

cston Aug 4, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading

jjonescz Aug 9, 2023 •

edited by jcouv

Loading