-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for assembly/module-level [Experimental]
and contextual suppression
#69316
Changes from 6 commits
2b800fb
c8a0fa0
6669f0b
d44dae2
6baacd2
b0ed1cd
afba201
fba843c
c9d41c6
7ad505e
9add30d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,12 @@ | |
|
||
#nullable disable | ||
|
||
using System; | ||
using System.Diagnostics; | ||
using System.Reflection.Metadata; | ||
using System.Threading; | ||
using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp.Symbols | ||
{ | ||
|
@@ -54,7 +56,7 @@ internal static ObsoleteAttributeData GetObsoleteDataFromMetadata(EntityHandle t | |
/// symbol's Obsoleteness is Unknown. False, if we are certain that no symbol in the parent | ||
/// hierarchy is Obsolete. | ||
/// </returns> | ||
private static ThreeState GetObsoleteContextState(Symbol symbol, bool forceComplete) | ||
private static ThreeState GetObsoleteContextState(Symbol symbol, bool forceComplete, Func<Symbol, ThreeState> getStateFromSymbol) | ||
{ | ||
while ((object)symbol != null) | ||
{ | ||
|
@@ -73,7 +75,7 @@ private static ThreeState GetObsoleteContextState(Symbol symbol, bool forceCompl | |
symbol.ForceCompleteObsoleteAttribute(); | ||
} | ||
|
||
var state = symbol.ObsoleteState; | ||
var state = getStateFromSymbol(symbol); | ||
if (state != ThreeState.False) | ||
{ | ||
return state; | ||
|
@@ -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)) | ||
{ | ||
// Skip for System.Diagnostics.CodeAnalysis.ExperimentalAttribute to mitigate cycles | ||
return ObsoleteDiagnosticKind.NotObsolete; | ||
} | ||
|
||
if (symbol is MethodSymbol { MethodKind: MethodKind.Constructor } method && isExperimentalSymbol(method.ContainingType)) | ||
{ | ||
// Skip for constructors of System.Diagnostics.CodeAnalysis.ExperimentalAttribute to mitigate cycles | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to skip for non-constructor methods? #Resolved |
||
return ObsoleteDiagnosticKind.NotObsolete; | ||
} | ||
|
||
if (symbol.ContainingModule.ObsoleteKind is ObsoleteAttributeKind.Experimental | ||
|| symbol.ContainingAssembly.ObsoleteKind is ObsoleteAttributeKind.Experimental) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we do this here? Couldn't we check containing member only when this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, it's better to put there. That said, I don't think one should use both attributes (Obsolete and Experimental) :-P |
||
{ | ||
return getDiagnosticKind(containingMember, forceComplete, getStateFromSymbol: static (symbol) => symbol.ExperimentalState); | ||
} | ||
|
||
switch (symbol.ObsoleteKind) | ||
{ | ||
case ObsoleteAttributeKind.None: | ||
if (symbol.ContainingModule.ObsoleteKind is ObsoleteAttributeKind.Uninitialized | ||
|| symbol.ContainingAssembly.ObsoleteKind is ObsoleteAttributeKind.Uninitialized) | ||
{ | ||
return ObsoleteDiagnosticKind.Lazy; | ||
} | ||
|
||
return ObsoleteDiagnosticKind.NotObsolete; | ||
case ObsoleteAttributeKind.WindowsExperimental: | ||
case ObsoleteAttributeKind.Experimental: | ||
return ObsoleteDiagnosticKind.Diagnostic; | ||
case ObsoleteAttributeKind.Experimental: | ||
return getDiagnosticKind(containingMember, forceComplete, getStateFromSymbol: static (symbol) => symbol.ExperimentalState); | ||
case ObsoleteAttributeKind.Uninitialized: | ||
// If we haven't cracked attributes on the symbol at all or we haven't | ||
// cracked attribute arguments enough to be able to report diagnostics for | ||
|
@@ -110,18 +137,30 @@ internal static ObsoleteDiagnosticKind GetObsoleteDiagnosticKind(Symbol symbol, | |
return ObsoleteDiagnosticKind.Lazy; | ||
} | ||
|
||
switch (GetObsoleteContextState(containingMember, forceComplete)) | ||
return getDiagnosticKind(containingMember, forceComplete, getStateFromSymbol: static (symbol) => symbol.ObsoleteState); | ||
|
||
static bool isExperimentalSymbol(NamedTypeSymbol namedTypeSymbol) | ||
{ | ||
case ThreeState.False: | ||
return ObsoleteDiagnosticKind.Diagnostic; | ||
case ThreeState.True: | ||
// If we are in a context that is already obsolete, there is no point reporting | ||
// more obsolete diagnostics. | ||
return ObsoleteDiagnosticKind.Suppressed; | ||
default: | ||
// If the context is unknown, then store the symbol so that we can do this check at a | ||
// later stage | ||
return ObsoleteDiagnosticKind.LazyPotentiallySuppressed; | ||
return namedTypeSymbol.Arity == 0 | ||
&& namedTypeSymbol.Name.Equals("ExperimentalAttribute", StringComparison.Ordinal) | ||
&& namedTypeSymbol.IsWellKnownDiagnosticsCodeAnalysisTopLevelType(); | ||
} | ||
|
||
static ObsoleteDiagnosticKind getDiagnosticKind(Symbol containingMember, bool forceComplete, Func<Symbol, ThreeState> getStateFromSymbol) | ||
{ | ||
switch (GetObsoleteContextState(containingMember, forceComplete, getStateFromSymbol)) | ||
{ | ||
case ThreeState.False: | ||
return ObsoleteDiagnosticKind.Diagnostic; | ||
case ThreeState.True: | ||
// If we are in a context that is already experimental/obsolete, there is no point reporting | ||
// more experimental/obsolete diagnostics. | ||
return ObsoleteDiagnosticKind.Suppressed; | ||
default: | ||
// If the context is unknown, then store the symbol so that we can do this check at a | ||
// later stage | ||
return ObsoleteDiagnosticKind.LazyPotentiallySuppressed; | ||
} | ||
} | ||
} | ||
|
||
|
@@ -137,7 +176,7 @@ internal static DiagnosticInfo CreateObsoleteDiagnostic(Symbol symbol, BinderFla | |
|
||
static DiagnosticInfo createObsoleteDiagnostic(Symbol symbol, BinderFlags location) | ||
{ | ||
var data = symbol.ObsoleteAttributeData; | ||
var data = symbol.ObsoleteAttributeData ?? symbol.ContainingModule.ObsoleteAttributeData ?? symbol.ContainingAssembly.ObsoleteAttributeData; | ||
Debug.Assert(data != null); | ||
|
||
if (data == null) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,10 +11,8 @@ | |||||||||||
using System.Diagnostics; | ||||||||||||
using System.Linq; | ||||||||||||
using System.Reflection; | ||||||||||||
using System.Security.Cryptography; | ||||||||||||
using System.Threading; | ||||||||||||
using Microsoft.CodeAnalysis.CSharp.Emit; | ||||||||||||
using Microsoft.CodeAnalysis.CSharp.Symbols; | ||||||||||||
using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE; | ||||||||||||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||||||||||||
using Microsoft.CodeAnalysis.PooledObjects; | ||||||||||||
|
@@ -1382,7 +1380,7 @@ private void LoadAndValidateNetModuleAttributes(ref CustomAttributesBag<CSharpAt | |||||||||||
// This affects only diagnostics. | ||||||||||||
for (int i = _modules.Length - 1; i > 0; i--) | ||||||||||||
{ | ||||||||||||
var peModuleSymbol = (Metadata.PE.PEModuleSymbol)_modules[i]; | ||||||||||||
var peModuleSymbol = (PEModuleSymbol)_modules[i]; | ||||||||||||
|
||||||||||||
foreach (NamedTypeSymbol forwarded in peModuleSymbol.GetForwardedTypes()) | ||||||||||||
{ | ||||||||||||
|
@@ -2553,6 +2551,11 @@ private void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments<Attr | |||||||||||
|
||||||||||||
arguments.GetOrCreateData<CommonAssemblyWellKnownAttributeData>().AssemblyAlgorithmIdAttributeSetting = algorithmId; | ||||||||||||
} | ||||||||||||
else if (attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute)) | ||||||||||||
{ | ||||||||||||
var obsoleteData = attribute.DecodeExperimentalAttribute(); | ||||||||||||
arguments.GetOrCreateData<CommonAssemblyWellKnownAttributeData>().ExperimentalAttributeData = obsoleteData; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
// Checks that the integral arguments for the given well-known attribute are non-negative. | ||||||||||||
|
@@ -2860,6 +2863,41 @@ private static string DefaultValue(TypeSymbol type) | |||||||||||
return null; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// <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> | ||||||||||||
Comment on lines
+2866
to
+2869
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||||||||
internal sealed override ObsoleteAttributeData? ObsoleteAttributeData | ||||||||||||
{ | ||||||||||||
get | ||||||||||||
{ | ||||||||||||
// [assembly: Experimental] may have been specified in the assembly or one of the modules | ||||||||||||
var lazySourceAttributesBag = _lazySourceAttributesBag; | ||||||||||||
if (lazySourceAttributesBag != null && lazySourceAttributesBag.IsDecodedWellKnownAttributeDataComputed) | ||||||||||||
{ | ||||||||||||
var data = (CommonAssemblyWellKnownAttributeData)lazySourceAttributesBag.DecodedWellKnownAttributeData; | ||||||||||||
if (data?.ExperimentalAttributeData is { } experimentalAttributeData) | ||||||||||||
{ | ||||||||||||
return experimentalAttributeData; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
var lazyNetModuleAttributesBag = _lazyNetModuleAttributesBag; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||
if (lazyNetModuleAttributesBag != null && lazyNetModuleAttributesBag.IsDecodedWellKnownAttributeDataComputed) | ||||||||||||
{ | ||||||||||||
var data = (CommonAssemblyWellKnownAttributeData)lazyNetModuleAttributesBag.DecodedWellKnownAttributeData; | ||||||||||||
return data?.ExperimentalAttributeData; | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (GetAttributeDeclarations().IsEmpty) | ||||||||||||
{ | ||||||||||||
return null; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an optimization. It allows not producing a lazy diagnostic when we know from syntax that there are no attributes. |
||||||||||||
} | ||||||||||||
|
||||||||||||
return ObsoleteAttributeData.Uninitialized; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
#nullable disable | ||||||||||||
|
||||||||||||
internal override IEnumerable<NamedTypeSymbol> GetAllTopLevelForwardedTypes() | ||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this cycle avoidance for
ExperimentalAttribute
but not forObsoleteAttribute
? #ResolvedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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