[ILLink analyzer] Add analyzer support for feature checks (#94944)

Adds support for annotating static boolean properties with
`[FeatureCheckAttribute(typeof(RequiresDynamicCodeAttribute))]`,
causing the property to be treated as a guard for analyzer
warnings about the corresponding `Requires` attribute.

Adds two new warnings for:

- Invalid use of `FeatureCheckAttribute` applied to a non-static
  or non-bool property

- Implementation of the property doesn't obviously satisfy
  the "guard property" (it should return false whenever the
  guarded feature is disabled).

See #94625 for notes on the design.
See #96859 for the API proposal.

---------

Co-authored-by: Jackson Schuster <[email protected]>

Author: sbomer

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureChecksValue.cs

@@ -13,11 +13,15 @@ namespace ILLink.RoslynAnalyzer.DataFlow
 	// For now, this is only designed to track the built-in "features"/"capabilities"
 	// like RuntimeFeatures.IsDynamicCodeSupported, where a true return value
 	// indicates that a feature/capability is available.
-	public record struct FeatureChecksValue : INegate<FeatureChecksValue>
+	public record struct FeatureChecksValue : INegate<FeatureChecksValue>, IDeepCopyValue<FeatureChecksValue>
 	{
 		public ValueSet<string> EnabledFeatures;
 		public ValueSet<string> DisabledFeatures;
 
+		public static readonly FeatureChecksValue All = new FeatureChecksValue (ValueSet<string>.Unknown, ValueSet<string>.Empty);
+
+		public static readonly FeatureChecksValue None = new FeatureChecksValue (ValueSet<string>.Empty, ValueSet<string>.Empty);
+
 		public FeatureChecksValue (string enabledFeature)
 		{
 			EnabledFeatures = new ValueSet<string> (enabledFeature);
@@ -48,5 +52,10 @@ public FeatureChecksValue Negate ()
 		{
 			return new FeatureChecksValue (DisabledFeatures.DeepCopy (), EnabledFeatures.DeepCopy ());
 		}
+
+		public FeatureChecksValue DeepCopy ()
+		{
+			return new FeatureChecksValue (EnabledFeatures.DeepCopy (), DisabledFeatures.DeepCopy ());
+		}
 	}
 }

…Analyzer/DataFlow/FeatureCheckVisitor.cs …nalyzer/DataFlow/FeatureChecksVisitor.cssrc/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureCheckVisitor.cs renamed to src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureChecksVisitor.cs src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureCheckVisitor.cs renamed to src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureChecksVisitor.cs

@@ -24,7 +24,7 @@ namespace ILLink.RoslynAnalyzer.DataFlow
 	// (a set features that are checked to be enabled or disabled).
 	// The visitor takes a LocalDataFlowState as an argument, allowing for checks that
 	// depend on the current dataflow state.
-	public class FeatureChecksVisitor : OperationVisitor<StateValue, FeatureChecksValue?>
+	public class FeatureChecksVisitor : OperationVisitor<StateValue, FeatureChecksValue>
 	{
 		DataFlowAnalyzerContext _dataFlowAnalyzerContext;
 
@@ -33,32 +33,48 @@ public FeatureChecksVisitor (DataFlowAnalyzerContext dataFlowAnalyzerContext)
 			_dataFlowAnalyzerContext = dataFlowAnalyzerContext;
 		}
 
-		public override FeatureChecksValue? VisitArgument (IArgumentOperation operation, StateValue state)
+		public override FeatureChecksValue DefaultVisit (IOperation operation, StateValue state)
+		{
+			// Visiting a non-understood pattern should return the empty set of features, which will
+			// prevent this check from acting as a guard for any feature.
+			return FeatureChecksValue.None;
+		}
+
+		public override FeatureChecksValue VisitArgument (IArgumentOperation operation, StateValue state)
 		{
 			return Visit (operation.Value, state);
 		}
 
-		public override FeatureChecksValue? VisitPropertyReference (IPropertyReferenceOperation operation, StateValue state)
+		public override FeatureChecksValue VisitPropertyReference (IPropertyReferenceOperation operation, StateValue state)
 		{
+			// A single property may serve as a feature check for multiple features.
+			FeatureChecksValue featureChecks = FeatureChecksValue.None;
 			foreach (var analyzer in _dataFlowAnalyzerContext.EnabledRequiresAnalyzers) {
-				if (analyzer.IsRequiresCheck (_dataFlowAnalyzerContext.Compilation, operation.Property)) {
-					return new FeatureChecksValue (analyzer.FeatureName);
+				if (analyzer.IsFeatureCheck (operation.Property, _dataFlowAnalyzerContext.Compilation)) {
+					var featureCheck = new FeatureChecksValue (analyzer.RequiresAttributeFullyQualifiedName);
+					featureChecks = featureChecks.And (featureCheck);
 				}
 			}
 
-			return null;
+			return featureChecks;
 		}
 
-		public override FeatureChecksValue? VisitUnaryOperator (IUnaryOperation operation, StateValue state)
+		public override FeatureChecksValue VisitUnaryOperator (IUnaryOperation operation, StateValue state)
 		{
 			if (operation.OperatorKind is not UnaryOperatorKind.Not)
-				return null;
+				return FeatureChecksValue.None;
 
-			FeatureChecksValue? context = Visit (operation.Operand, state);
-			if (context == null)
-				return null;
+			FeatureChecksValue context = Visit (operation.Operand, state);
+			return context.Negate ();
+		}
 
-			return context.Value.Negate ();
+		public override FeatureChecksValue VisitLiteral (ILiteralOperation operation, StateValue state)
+		{
+			// 'false' can guard any feature
+			if (GetConstantBool (operation.ConstantValue) is false)
+				return FeatureChecksValue.All;
+
+			return FeatureChecksValue.None;
 		}
 
 		public bool? GetLiteralBool (IOperation operation)
@@ -77,7 +93,7 @@ public FeatureChecksVisitor (DataFlowAnalyzerContext dataFlowAnalyzerContext)
 			return value;
 		}
 
-		public override FeatureChecksValue? VisitBinaryOperator (IBinaryOperation operation, StateValue state)
+		public override FeatureChecksValue VisitBinaryOperator (IBinaryOperation operation, StateValue state)
 		{
 			bool expectEqual;
 			switch (operation.OperatorKind) {
@@ -88,36 +104,32 @@ public FeatureChecksVisitor (DataFlowAnalyzerContext dataFlowAnalyzerContext)
 					expectEqual = false;
 					break;
 				default:
-					return null;
+					return FeatureChecksValue.None;
 			}
 
 			if (GetLiteralBool (operation.LeftOperand) is bool leftBool) {
-				if (Visit (operation.RightOperand, state) is not FeatureChecksValue rightValue)
-					return null;
+				FeatureChecksValue rightValue = Visit (operation.RightOperand, state);
 				return leftBool == expectEqual
 					? rightValue
 					: rightValue.Negate ();
 			}
 
 			if (GetLiteralBool (operation.RightOperand) is bool rightBool) {
-				if (Visit (operation.LeftOperand, state) is not FeatureChecksValue leftValue)
-					return null;
+				FeatureChecksValue leftValue = Visit (operation.LeftOperand, state);
 				return rightBool == expectEqual
 					? leftValue
 					: leftValue.Negate ();
 			}
 
-			return null;
+			return FeatureChecksValue.None;
 		}
 
-		public override FeatureChecksValue? VisitIsPattern (IIsPatternOperation operation, StateValue state)
+		public override FeatureChecksValue VisitIsPattern (IIsPatternOperation operation, StateValue state)
 		{
 			if (GetExpectedValueFromPattern (operation.Pattern) is not bool patternValue)
-				return null;
-
-			if (Visit (operation.Value, state) is not FeatureChecksValue value)
-				return null;
+				return FeatureChecksValue.None;
 
+			FeatureChecksValue value = Visit (operation.Value, state);
 			return patternValue
 				? value
 				: value.Negate ();

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

@@ -88,12 +88,12 @@ public LocalDataFlowVisitor (
 				return null;
 
 			var branchValue = Visit (branchValueOperation, state);
-
+			TConditionValue conditionValue = GetConditionValue (branchValueOperation, state);
 			if (block.Block.ConditionKind != ControlFlowConditionKind.None) {
 				// BranchValue may represent a value used in a conditional branch to the ConditionalSuccessor.
 				// If so, give the analysis an opportunity to model the checked condition, and return the model
 				// of the condition back to the generic analysis. It will be applied to the state of each outgoing branch.
-				return GetConditionValue (branchValueOperation, state);
+				return conditionValue;
 			}
 
 			// If not, the BranchValue represents a return or throw value associated with the FallThroughSuccessor of this block.
@@ -118,10 +118,13 @@ public LocalDataFlowVisitor (
 			// We don't want the return operation because this might have multiple possible return values in general.
 			var current = state.Current;
 			HandleReturnValue (branchValue, branchValueOperation, in current.Context);
+			// Must be called for every return value even if it did not return an understood condition,
+			// because the non-understood conditions will produce warnings for FeatureCheck properties.
+			HandleReturnConditionValue (conditionValue, branchValueOperation);
 			return null;
 		}
 
-		public abstract TConditionValue? GetConditionValue (
+		public abstract TConditionValue GetConditionValue (
 			IOperation branchValueOperation,
 			LocalDataFlowState<TValue, TContext, TValueLattice, TContextLattice> state);
 
@@ -146,6 +149,10 @@ public abstract void HandleReturnValue (
 			IOperation operation,
 			in TContext context);
 
+		public abstract void HandleReturnConditionValue (
+			TConditionValue returnConditionValue,
+			IOperation branchValueOperation);
+
 		// This is called for any method call, which includes:
 		// - Normal invocation operation
 		// - Accessing property value - which is treated as a call to the getter
@@ -776,9 +783,7 @@ TValue HandleMethodCallHelper (
 
 				// Get the condition value that is being asserted. If the attribute is DoesNotReturnIf(true),
 				// the condition value needs to be negated so that we can assert the false condition.
-				if (GetConditionValue (argumentOperation, state) is not TConditionValue conditionValue)
-					continue;
-
+				TConditionValue conditionValue = GetConditionValue (argumentOperation, state);
 				var current = state.Current;
 				ApplyCondition (
 					doesNotReturnIfConditionValue == false

…oslynAnalyzer/DataflowAnalyzerContext.cs …oslynAnalyzer/DataFlowAnalyzerContext.cssrc/tools/illink/src/ILLink.RoslynAnalyzer/DataflowAnalyzerContext.cs renamed to src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlowAnalyzerContext.cs src/tools/illink/src/ILLink.RoslynAnalyzer/DataflowAnalyzerContext.cs renamed to src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlowAnalyzerContext.cs

@@ -21,6 +21,8 @@ public class DynamicallyAccessedMembersAnalyzer : DiagnosticAnalyzer
 		internal const string DynamicallyAccessedMembersAttribute = nameof (DynamicallyAccessedMembersAttribute);
 		public const string attributeArgument = "attributeArgument";
 		public const string FullyQualifiedDynamicallyAccessedMembersAttribute = "System.Diagnostics.CodeAnalysis." + DynamicallyAccessedMembersAttribute;
+		public const string FullyQualifiedFeatureCheckAttribute  = "System.Diagnostics.CodeAnalysis.FeatureCheckAttribute";
+		public const string FullyQualifiedFeatureDependsOnAttribute  = "System.Diagnostics.CodeAnalysis.FeatureDependsOnAttribute";
 		public static Lazy<ImmutableArray<RequiresAnalyzerBase>> RequiresAnalyzers { get; } = new Lazy<ImmutableArray<RequiresAnalyzerBase>> (GetRequiresAnalyzers);
 		static ImmutableArray<RequiresAnalyzerBase> GetRequiresAnalyzers () =>
 			ImmutableArray.Create<RequiresAnalyzerBase> (
@@ -51,6 +53,8 @@ public static ImmutableArray<DiagnosticDescriptor> GetSupportedDiagnostics ()
 			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnrecognizedTypeNameInTypeGetType));
 			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnrecognizedParameterInMethodCreateInstance));
 			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.ParametersOfAssemblyCreateInstanceCannotBeAnalyzed));
+			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.ReturnValueDoesNotMatchFeatureChecks));
+			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.InvalidFeatureCheck));
 
 			foreach (var requiresAnalyzer in RequiresAnalyzers.Value) {
 				foreach (var diagnosticDescriptor in requiresAnalyzer.SupportedDiagnostics)

src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs

@@ -1,9 +1,14 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System.Collections.Immutable;
+using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
+using System.Linq;
 using System.Text;
 using Microsoft.CodeAnalysis;
+using ILLink.RoslynAnalyzer.DataFlow;
+using ILLink.Shared.DataFlow;
 
 namespace ILLink.RoslynAnalyzer
 {
@@ -34,6 +39,14 @@ internal static bool TryGetAttribute (this ISymbol member, string attributeName,
 			return false;
 		}
 
+		internal static IEnumerable<AttributeData> GetAttributes (this ISymbol member, string attributeName)
+		{
+			foreach (var attr in member.GetAttributes ()) {
+				if (attr.AttributeClass is { } attrClass && attrClass.HasName (attributeName))
+					yield return attr;
+			}
+		}
+
 		internal static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypes (this ISymbol symbol)
 		{
 			if (!TryGetAttribute (symbol, DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var dynamicallyAccessedMembers))
@@ -58,6 +71,28 @@ internal static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypes
 			return (DynamicallyAccessedMemberTypes) dynamicallyAccessedMembers.ConstructorArguments[0].Value!;
 		}
 
+		internal static ValueSet<string> GetFeatureCheckAnnotations (this IPropertySymbol propertySymbol)
+		{
+			HashSet<string> featureSet = new ();
+			foreach (var attributeData in propertySymbol.GetAttributes (DynamicallyAccessedMembersAnalyzer.FullyQualifiedFeatureCheckAttribute)) {
+				if (attributeData.ConstructorArguments is [TypedConstant { Value: INamedTypeSymbol featureType }])
+					AddFeatures (featureType);
+			}
+			return featureSet.Count == 0 ? ValueSet<string>.Empty : new ValueSet<string> (featureSet);
+
+			void AddFeatures (INamedTypeSymbol featureType) {
+				var featureName = featureType.GetDisplayName ();
+				if (!featureSet.Add (featureName))
+					return;
+
+				// Look at FeatureDependsOn attributes on the feature type.
+				foreach (var featureTypeAttributeData in featureType.GetAttributes (DynamicallyAccessedMembersAnalyzer.FullyQualifiedFeatureDependsOnAttribute)) {
+					if (featureTypeAttributeData.ConstructorArguments is [TypedConstant { Value: INamedTypeSymbol featureTypeSymbol }])
+						AddFeatures (featureTypeSymbol);
+				}
+			}
+		}
+
 		internal static bool TryGetReturnAttribute (this IMethodSymbol member, string attributeName, [NotNullWhen (returnValue: true)] out AttributeData? attribute)
 		{
 			attribute = null;

src/tools/illink/src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs

@@ -5,8 +5,9 @@
 using System.Collections.Immutable;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
-using ILLink.Shared;
 using ILLink.RoslynAnalyzer.DataFlow;
+using ILLink.Shared;
+using ILLink.Shared.DataFlow;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -19,9 +20,7 @@ public abstract class RequiresAnalyzerBase : DiagnosticAnalyzer
 	{
 		private protected abstract string RequiresAttributeName { get; }
 
-		internal abstract string FeatureName { get; }
-
-		private protected abstract string RequiresAttributeFullyQualifiedName { get; }
+		internal abstract string RequiresAttributeFullyQualifiedName { get; }
 
 		private protected abstract DiagnosticTargets AnalyzerDiagnosticTargets { get; }
 
@@ -301,7 +300,23 @@ protected virtual bool CreateSpecialIncompatibleMembersDiagnostic (
 		// - false return value indicating that a feature is supported
 		// - feature settings supplied by the project
 		// - custom feature checks defined in library code
-		internal virtual bool IsRequiresCheck (Compilation compilation, IPropertySymbol propertySymbol) => false;
+		private protected virtual bool IsRequiresCheck (IPropertySymbol propertySymbol, Compilation compilation) => false;
+
+		internal static bool IsAnnotatedFeatureCheck (IPropertySymbol propertySymbol, string featureName)
+		{
+			// Only respect FeatureCheckAttribute on static boolean properties.
+			if (!propertySymbol.IsStatic || propertySymbol.Type.SpecialType != SpecialType.System_Boolean)
+				return false;
+
+			ValueSet<string> featureCheckAnnotations = propertySymbol.GetFeatureCheckAnnotations ();
+			return featureCheckAnnotations.Contains (featureName);
+		}
+
+		internal bool IsFeatureCheck (IPropertySymbol propertySymbol, Compilation compilation)
+		{
+			return IsAnnotatedFeatureCheck (propertySymbol, RequiresAttributeFullyQualifiedName)
+				|| IsRequiresCheck (propertySymbol, compilation);
+		}
 
 		internal bool CheckAndCreateRequiresDiagnostic (
 			IOperation operation,
@@ -312,7 +327,7 @@ internal bool CheckAndCreateRequiresDiagnostic (
 			[NotNullWhen (true)] out Diagnostic? diagnostic)
 		{
 			// Warnings are not emitted if the featureContext says the feature is available.
-			if (featureContext.IsEnabled (FeatureName)) {
+			if (featureContext.IsEnabled (RequiresAttributeFullyQualifiedName)) {
 				diagnostic = null;
 				return false;
 			}

src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

@@ -36,9 +36,7 @@ public sealed class RequiresAssemblyFilesAnalyzer : RequiresAnalyzerBase
 
 		private protected override string RequiresAttributeName => RequiresAssemblyFilesAttribute;
 
-		internal override string FeatureName => "AssemblyFiles";
-
-		private protected override string RequiresAttributeFullyQualifiedName => RequiresAssemblyFilesAttributeFullyQualifiedName;
+		internal override string RequiresAttributeFullyQualifiedName => RequiresAssemblyFilesAttributeFullyQualifiedName;
 
 		private protected override DiagnosticTargets AnalyzerDiagnosticTargets => DiagnosticTargets.MethodOrConstructor | DiagnosticTargets.Property | DiagnosticTargets.Event;
 
@@ -61,7 +59,7 @@ internal override bool IsAnalyzerEnabled (AnalyzerOptions options)
 			return true;
 		}
 
-		internal override bool IsRequiresCheck (Compilation compilation, IPropertySymbol propertySymbol)
+		private protected override bool IsRequiresCheck (IPropertySymbol propertySymbol, Compilation compilation)
 		{
 			// "IsAssemblyFilesSupported" is treated as a requires check for testing purposes only, and
 			// is not officially-supported product behavior.

src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

@@ -25,9 +25,7 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase
 
 		private protected override string RequiresAttributeName => RequiresDynamicCodeAttribute;
 
-		internal override string FeatureName => "DynamicCode";
-
-		private protected override string RequiresAttributeFullyQualifiedName => FullyQualifiedRequiresDynamicCodeAttribute;
+		internal override string RequiresAttributeFullyQualifiedName => FullyQualifiedRequiresDynamicCodeAttribute;
 
 		private protected override DiagnosticTargets AnalyzerDiagnosticTargets => DiagnosticTargets.MethodOrConstructor | DiagnosticTargets.Class;
 
@@ -40,7 +38,7 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase
 		internal override bool IsAnalyzerEnabled (AnalyzerOptions options) =>
 			options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableAotAnalyzer);
 
-		internal override bool IsRequiresCheck (Compilation compilation, IPropertySymbol propertySymbol) {
+		private protected override bool IsRequiresCheck (IPropertySymbol propertySymbol, Compilation compilation) {
 			var runtimeFeaturesType = compilation.GetTypeByMetadataName ("System.Runtime.CompilerServices.RuntimeFeature");
 			if (runtimeFeaturesType == null)
 				return false;

src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresDynamicCodeAnalyzer.cs

@@ -49,11 +49,7 @@ private Action<SymbolAnalysisContext> typeDerivesFromRucBase {
 
 		private protected override string RequiresAttributeName => RequiresUnreferencedCodeAttribute;
 
-		public const string UnreferencedCode = nameof (UnreferencedCode);
-
-		internal override string FeatureName => UnreferencedCode;
-
-		private protected override string RequiresAttributeFullyQualifiedName => FullyQualifiedRequiresUnreferencedCodeAttribute;
+		internal override string RequiresAttributeFullyQualifiedName => FullyQualifiedRequiresUnreferencedCodeAttribute;
 
 		private protected override DiagnosticTargets AnalyzerDiagnosticTargets => DiagnosticTargets.MethodOrConstructor | DiagnosticTargets.Class;
 
@@ -66,7 +62,7 @@ private Action<SymbolAnalysisContext> typeDerivesFromRucBase {
 		internal override bool IsAnalyzerEnabled (AnalyzerOptions options) =>
 			options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer);
 
-		internal override bool IsRequiresCheck (Compilation compilation, IPropertySymbol propertySymbol)
+		private protected override bool IsRequiresCheck (IPropertySymbol propertySymbol, Compilation compilation)
 		{
 			// "IsUnreferencedCodeSupported" is treated as a requires check for testing purposes only, and
 			// is not officially-supported product behavior.