Skip to content
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

Merged
merged 11 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -276,15 +276,6 @@ public sealed override bool IsExtern
}
}

/// <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>
internal sealed override ObsoleteAttributeData ObsoleteAttributeData
{
get { return null; }
}

public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ internal sealed class PEAssemblySymbol : MetadataOrSourceAssemblySymbol

#nullable enable
private DiagnosticInfo? _lazyCachedCompilerFeatureRequiredDiagnosticInfo = CSDiagnosticInfo.EmptyErrorInfo;

private ObsoleteAttributeData? _lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
#nullable disable

internal PEAssemblySymbol(PEAssembly assembly, DocumentationProvider documentationProvider, bool isLinked, MetadataImportOptions importOptions)
Expand Down Expand Up @@ -311,5 +313,31 @@ internal PEModuleSymbol PrimaryModule

public override bool HasUnsupportedMetadata
=> GetCompilerFeatureRequiredDiagnostic()?.Code == (int)ErrorCode.ERR_UnsupportedCompilerFeature || base.HasUnsupportedMetadata;

internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
{
get
{
if (_lazyObsoleteAttributeData == ObsoleteAttributeData.Uninitialized)
{
Interlocked.CompareExchange(ref _lazyObsoleteAttributeData, computeObsoleteAttributeData(), ObsoleteAttributeData.Uninitialized);
}

return _lazyObsoleteAttributeData;

ObsoleteAttributeData? computeObsoleteAttributeData()
{
foreach (var attrData in GetAttributes())
{
if (attrData.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute))
{
return attrData.DecodeExperimentalAttribute();
}
}

return null;
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ internal enum RefSafetyRulesAttributeVersion

#nullable enable
private DiagnosticInfo? _lazyCachedCompilerFeatureRequiredDiagnosticInfo = CSDiagnosticInfo.EmptyErrorInfo;

private ObsoleteAttributeData? _lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
#nullable disable

internal PEModuleSymbol(PEAssemblySymbol assemblySymbol, PEModule module, MetadataImportOptions importOptions, int ordinal)
Expand Down Expand Up @@ -857,5 +859,31 @@ RefSafetyRulesAttributeVersion getAttributeVersion()
}
}
}

internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
{
get
{
if (_lazyObsoleteAttributeData == ObsoleteAttributeData.Uninitialized)
{
Interlocked.CompareExchange(ref _lazyObsoleteAttributeData, computeObsoleteAttributeData(), ObsoleteAttributeData.Uninitialized);
}

return _lazyObsoleteAttributeData;

ObsoleteAttributeData? computeObsoleteAttributeData()
{
foreach (var attrData in GetAttributes())
{
if (attrData.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute))
{
return attrData.DecodeExperimentalAttribute();
}
}

return null;
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -209,5 +209,8 @@ internal sealed override IEnumerable<NamedTypeSymbol> GetAllTopLevelForwardedTyp
{
return SpecializedCollections.EmptyEnumerable<NamedTypeSymbol>();
}

#nullable enable
internal sealed override ObsoleteAttributeData? ObsoleteAttributeData => null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ public sealed override bool AreLocalsZeroed
}

internal sealed override bool UseUpdatedEscapeRules => false;

#nullable enable
internal sealed override ObsoleteAttributeData? ObsoleteAttributeData => null;
#nullable disable
}

internal sealed class MissingModuleSymbolWithName : MissingModuleSymbol
Expand Down
9 changes: 0 additions & 9 deletions src/Compilers/CSharp/Portable/Symbols/ModuleSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,6 @@ public sealed override bool IsExtern
}
}

/// <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>
internal sealed override ObsoleteAttributeData ObsoleteAttributeData
{
get { return null; }
}

public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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)
{
Expand All @@ -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;
Expand All @@ -95,13 +97,35 @@ 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))
Copy link
Member

@jjonescz jjonescz Aug 9, 2023

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 for ObsoleteAttribute? #Resolved

Copy link
Member Author

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

{
// 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
Copy link
Member

@jjonescz jjonescz Aug 9, 2023

Choose a reason for hiding this comment

The 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;
}

Debug.Assert(symbol.ContainingModule.ObsoleteKind is not ObsoleteAttributeKind.Uninitialized);
Debug.Assert(symbol.ContainingAssembly.ObsoleteKind is not ObsoleteAttributeKind.Uninitialized);

if (symbol.ContainingModule.ObsoleteKind is ObsoleteAttributeKind.Experimental
|| symbol.ContainingAssembly.ObsoleteKind is ObsoleteAttributeKind.Experimental)
Copy link
Member

@jjonescz jjonescz Aug 9, 2023

Choose a reason for hiding this comment

The 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 symbol's obsolete kind is None? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The 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 getExperimentalDiagnosticKind(containingMember, forceComplete);
}

switch (symbol.ObsoleteKind)
{
case ObsoleteAttributeKind.None:
return ObsoleteDiagnosticKind.NotObsolete;
case ObsoleteAttributeKind.WindowsExperimental:
case ObsoleteAttributeKind.Experimental:
return ObsoleteDiagnosticKind.Diagnostic;
case ObsoleteAttributeKind.Experimental:
return getExperimentalDiagnosticKind(containingMember, forceComplete);
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
Expand All @@ -110,7 +134,7 @@ internal static ObsoleteDiagnosticKind GetObsoleteDiagnosticKind(Symbol symbol,
return ObsoleteDiagnosticKind.Lazy;
}

switch (GetObsoleteContextState(containingMember, forceComplete))
switch (GetObsoleteContextState(containingMember, forceComplete, getStateFromSymbol: static (symbol) => symbol.ObsoleteState))
{
case ThreeState.False:
return ObsoleteDiagnosticKind.Diagnostic;
Expand All @@ -123,6 +147,30 @@ internal static ObsoleteDiagnosticKind GetObsoleteDiagnosticKind(Symbol symbol,
// later stage
return ObsoleteDiagnosticKind.LazyPotentiallySuppressed;
}

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

@cston cston Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

&& namedTypeSymbol.Name.Equals("ExperimentalAttribute", StringComparison.Ordinal);
Copy link
Member Author

@jcouv jcouv Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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

Copy link
Member

@cston cston Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Copy link
Member

@cston cston Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
case ThreeState.False:
return ObsoleteDiagnosticKind.Diagnostic;
case ThreeState.True:
// If we are in a context that is already experimental, 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;
}
}
}

/// <summary>
Expand All @@ -137,7 +185,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,8 @@ internal override bool GetGuidString(out string guidString)
}

#nullable enable
internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
=> _underlyingAssembly.ObsoleteAttributeData;

internal override NamedTypeSymbol? TryLookupForwardedMetadataTypeWithCycleDetection(ref MetadataTypeName emittedName, ConsList<AssemblySymbol>? visitedAssemblies)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,5 +318,9 @@ public sealed override bool AreLocalsZeroed
}

internal override bool UseUpdatedEscapeRules => _underlyingModule.UseUpdatedEscapeRules;

#nullable enable
internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
=> _underlyingModule.ObsoleteAttributeData;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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())
{
Expand Down Expand Up @@ -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>().ObsoleteAttributeData = obsoleteData;
}
}

// Checks that the integral arguments for the given well-known attribute are non-negative.
Expand Down Expand Up @@ -2860,6 +2863,19 @@ private static string DefaultValue(TypeSymbol type)
return null;
}

internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
{
get
{
// [assembly: Experimental] may have been specified in the assembly or one of the modules
var result = GetSourceDecodedWellKnownAttributeData()?.ObsoleteAttributeData
?? GetNetModuleDecodedWellKnownAttributeData()?.ObsoleteAttributeData;

Debug.Assert(result is null || result is { Kind: ObsoleteAttributeKind.Experimental });
return result;
}
}

#nullable disable

internal override IEnumerable<NamedTypeSymbol> GetAllTopLevelForwardedTypes()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,10 @@ protected override void DecodeWellKnownAttributeImpl(ref DecodeWellKnownAttribut
{
CSharpAttributeData.DecodeSkipLocalsInitAttribute<ModuleWellKnownAttributeData>(DeclaringCompilation, ref arguments);
}
else if (attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute))
{
arguments.GetOrCreateData<ModuleWellKnownAttributeData>().ObsoleteAttributeData = attribute.DecodeExperimentalAttribute();
}
}

#nullable enable
Expand Down Expand Up @@ -652,5 +656,8 @@ internal override bool UseUpdatedEscapeRules
return _lazyUseUpdatedEscapeRules == ThreeState.True;
}
}

internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
=> GetDecodedWellKnownAttributeData()?.ObsoleteAttributeData;
}
}
24 changes: 23 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Symbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1309,6 +1309,7 @@ internal static bool GetUnificationUseSiteDiagnosticRecursive(ref DiagnosticInfo

#endregion

#nullable enable
/// <summary>
/// True if this symbol has been marked with the <see cref="ObsoleteAttribute"/> attribute.
/// This property returns <see cref="ThreeState.Unknown"/> if the <see cref="ObsoleteAttribute"/> attribute hasn't been cracked yet.
Expand All @@ -1331,6 +1332,26 @@ internal ThreeState ObsoleteState
}
}

/// <summary>
/// True if this symbol has been marked with the System.Diagnostics.CodeAnalysis.ExperimentalAttribute attribute.
/// This property returns <see cref="ThreeState.Unknown"/> if the attribute hasn't been cracked yet.
/// </summary>
internal ThreeState ExperimentalState
{
get
{
switch (ObsoleteKind)
{
case ObsoleteAttributeKind.Experimental:
return ThreeState.True;
case ObsoleteAttributeKind.Uninitialized:
return ThreeState.Unknown;
default:
return ThreeState.False;
}
}
}

internal ObsoleteAttributeKind ObsoleteKind
{
get
Expand All @@ -1344,7 +1365,8 @@ internal ObsoleteAttributeKind ObsoleteKind
/// 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; }
internal abstract ObsoleteAttributeData? ObsoleteAttributeData { get; }
#nullable disable

/// <summary>
/// Returns true and a <see cref="string"/> from the first <see cref="GuidAttribute"/> on the symbol,
Expand Down
Loading