Report Requires diagnostics from dataflow analyzer (#92724)

This moves the logic for generating diagnostics about
RequiresUnreferencedCode, RequiresAssemblyFiles, and
RequiresDynamicCode attributes from the separate Requires* analyzers
into the DynamicallyAccessedMembersAnalyzer.

This includes logic for warning on access to attributed members, not
the warnings about mismatching attributes on base/override
methods. The override validation logic is still handled by the
respective analyzers.

The DynamicallyAccessedMembersAnalyzer is now turned on by any of the
settings EnableTrimAnalyzer, EnableAotAnalyzer, or
EnableSingleFileAnalyzer.

A new type DataFlowAnalyzerContext is used to cache per-compilation
state of the analyzers, to avoid recomputing it for each trim analysis
pattern that might potentially produce a warning. The context only
stores state for enabled analyzers, based on the MSBuild property
values.

There are a few minor differences in the warning behavior with this
change:

- Implicit calls to annotated base ctors warn because they are visible
  in the CFG

- Unused local functions aren't discovered by the analysis, so don't
  warn (matching linker/AOT behavior)

Also fixes a few asserts that were being hit with the live analyzer
(changes from de011df):

- Ref property passed as out parameter

- Delegate creation of an invocation operation

* Add test for annotated value assigned to event

* Use DiagnosticContext for consistency

* Move field access handling into dataflow analyzer

* Remove unnecessary merge logic for reflection access pattern

* PR feedback

- Clean up a few unused usings
- Remove unused Instance field

* Fix ExpectedWarnings for ILLink

* Fix assert for ref property as out param

And related cases involving implicit indexer references.

* Add test for deconstruction assignment to ref property

* Fix delegate creation over invocation

* Fix ExpectedWarnings for illink

* Silence dataflow warnings if EnableTrimAnalyer isn't set

Extra warnings were being produced on some projects where
the single-file analyzer was enabled, causing the DAM analyzer
to light up. The dataflow warnings were missing a check of
EnableTrimAnalzer to prevent these warnings.

This also lifts the checking of MSBuild property values out into
the compilation start action, so it only needs to be done per
compilation. The context type has been renamed to reflect that
it holds state relevant to all of the analyses handled by the
dataflow logic now.

* Add test coverage for EnableTrimAnalyzer checks

And add missing checks in DAM analyzer

Author: sbomer

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

@@ -11,16 +11,8 @@ internal static class AnalyzerOptionsExtensions
 	{
 		public static string? GetMSBuildPropertyValue (
 			this AnalyzerOptions options,
-			string optionName,
-			Compilation compilation)
+			string optionName)
 		{
-			// MSBuild property values should be set at compilation level, and cannot have different values per-tree.
-			// So, we default to first syntax tree.
-			var tree = compilation.SyntaxTrees.FirstOrDefault ();
-			if (tree is null) {
-				return null;
-			}
-
 			return options.AnalyzerConfigOptionsProvider.GlobalOptions.TryGetValue (
 					$"build_property.{optionName}", out var value)
 				? value
@@ -29,10 +21,9 @@ internal static class AnalyzerOptionsExtensions
 
 		public static bool IsMSBuildPropertyValueTrue (
 			this AnalyzerOptions options,
-			string propertyName,
-			Compilation compilation)
+			string propertyName)
 		{
-			var propertyValue = GetMSBuildPropertyValue (options, propertyName, compilation);
+			var propertyValue = GetMSBuildPropertyValue (options, propertyName);
 			if (!string.Equals (propertyValue?.Trim (), "true", System.StringComparison.OrdinalIgnoreCase))
 				return false;
 

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

@@ -31,7 +31,7 @@ public override void Initialize (AnalysisContext context)
 			context.ConfigureGeneratedCodeAnalysis (GeneratedCodeAnalysisFlags.ReportDiagnostics);
 			context.RegisterCompilationStartAction (context => {
 				var compilation = context.Compilation;
-				if (!context.Options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer, compilation))
+				if (!context.Options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer))
 					return;
 
 				context.RegisterOperationAction (operationContext => {

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

@@ -16,6 +16,7 @@ public CapturedReferenceValue (IOperation operation)
 		{
 			switch (operation.Kind) {
 			case OperationKind.PropertyReference:
+			case OperationKind.EventReference:
 			case OperationKind.LocalReference:
 			case OperationKind.FieldReference:
 			case OperationKind.ParameterReference:
@@ -26,7 +27,6 @@ public CapturedReferenceValue (IOperation operation)
 			case OperationKind.None:
 			case OperationKind.InstanceReference:
 			case OperationKind.Invocation:
-			case OperationKind.EventReference:
 			case OperationKind.Invalid:
 				// These will just be ignored when referenced later.
 				break;

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

@@ -58,7 +58,6 @@ public void InterproceduralAnalyze ()
 				return;
 			}
 
-
 			Debug.Assert (Context.OwningSymbol is not IMethodSymbol methodSymbol ||
 				methodSymbol.MethodKind is not (MethodKind.LambdaMethod or MethodKind.LocalFunction));
 			var startMethod = new MethodBodyValue (Context.OwningSymbol, Context.GetControlFlowGraph (OperationBlock));
@@ -68,9 +67,6 @@ public void InterproceduralAnalyze ()
 				oldInterproceduralState = interproceduralState.Clone ();
 
 				foreach (var method in oldInterproceduralState.Methods) {
-					if (method.OwningSymbol.IsInRequiresUnreferencedCodeAttributeScope (out _))
-						continue;
-
 					AnalyzeMethod (method, ref interproceduralState);
 				}
 			}

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

@@ -97,7 +97,7 @@ public void Transfer (BlockProxy block, LocalDataFlowState<TValue, TValueLattice
 			HandleReturnValue (branchValue, branchValueOperation);
 		}
 
-		public abstract TValue GetFieldTargetValue (IFieldSymbol field);
+		public abstract TValue GetFieldTargetValue (IFieldSymbol field, IFieldReferenceOperation fieldReferenceOperation);
 
 		public abstract TValue GetParameterTargetValue (IParameterSymbol parameter);
 
@@ -170,7 +170,7 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, IAssignmentOpe
 			case IFieldReferenceOperation:
 			case IParameterReferenceOperation: {
 					TValue targetValue = targetOperation switch {
-						IFieldReferenceOperation fieldRef => GetFieldTargetValue (fieldRef.Field),
+						IFieldReferenceOperation fieldRef => GetFieldTargetValue (fieldRef.Field, fieldRef),
 						IParameterReferenceOperation parameterRef => GetParameterTargetValue (parameterRef.Parameter),
 						_ => throw new InvalidOperationException ()
 					};
@@ -212,6 +212,13 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, IAssignmentOpe
 					// even though a property setter has no return value.
 					return value;
 				}
+			case IEventReferenceOperation eventRef: {
+					// Handles assignment to an event like 'Event = Handler;', which is a write to the underlying field,
+					// not a call to an event accessor method. There is no Roslyn API to access the field,
+					// so just visit the instance and the value. https://github.com/dotnet/roslyn/issues/40103
+					Visit (eventRef.Instance, state);
+					return Visit (operation.Value, state);
+				}
 			case IImplicitIndexerReferenceOperation indexerRef: {
 					// An implicit reference to an indexer where the argument is a System.Index
 					TValue instanceValue = Visit (indexerRef.Instance, state);
@@ -276,10 +283,6 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, IAssignmentOpe
 			// also produces warnings for ref params/locals/returns.
 			// https://github.com/dotnet/linker/issues/2632
 			// https://github.com/dotnet/linker/issues/2158
-			case IEventReferenceOperation:
-				// An event assignment is an assignment to the generated backing field for
-				// auto-implemented events. There is no Roslyn API to access the field, so
-				// skip this. https://github.com/dotnet/roslyn/issues/40103
 				Visit (targetOperation, state);
 				break;
 
@@ -359,6 +362,26 @@ TValue ProcessAssignment (IAssignmentOperation operation, LocalDataFlowState<TVa
 			return value;
 		}
 
+		public override TValue VisitEventAssignment (IEventAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
+		{
+			var eventReference = (IEventReferenceOperation) operation.EventReference;
+			TValue instanceValue = Visit (eventReference.Instance, state);
+			TValue value = Visit (operation.HandlerValue, state);
+			if (operation.Adds) {
+				IMethodSymbol? addMethod = eventReference.Event.AddMethod;
+				Debug.Assert (addMethod != null);
+				if (addMethod != null)
+					HandleMethodCall (addMethod, instanceValue, ImmutableArray.Create (value), operation);
+				return value;
+			} else {
+				IMethodSymbol? removeMethod = eventReference.Event.RemoveMethod;
+				Debug.Assert (removeMethod != null);
+				if (removeMethod != null)
+					HandleMethodCall (removeMethod, instanceValue, ImmutableArray.Create (value), operation);
+				return value;
+			}
+		}
+
 		TValue GetFlowCaptureValue (IFlowCaptureReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
 		{
 			Debug.Assert (!IsLValueFlowCapture (operation.Id),
@@ -465,43 +488,58 @@ public override TValue VisitInvocation (IInvocationOperation operation, LocalDat
 
 		public override TValue VisitDelegateCreation (IDelegateCreationOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
 		{
-			if (operation.Target is IFlowAnonymousFunctionOperation lambda) {
-				VisitFlowAnonymousFunction (lambda, state);
-
-				// Instance of a lambda or local function should be the instance of the containing method.
-				// Don't need to track a dataflow value, since the delegate creation will warn if the
-				// lambda or local function has an annotated this parameter.
-				var instance = TopValue;
-				return HandleDelegateCreation (lambda.Symbol, instance, operation);
-			}
+			Visit (operation.Target, state);
 
-			Debug.Assert (operation.Target is IMemberReferenceOperation,
-				$"{operation.Target.GetType ()}: {operation.Syntax.GetLocation ().GetLineSpan ()}");
-			if (operation.Target is not IMemberReferenceOperation memberReference)
-				return TopValue;
+			IMethodSymbol? targetMethodSymbol = null;
+			switch (operation.Target) {
+				case IFlowAnonymousFunctionOperation lambda:
+					// Tracking lambdas is handled by normal visiting logic for IFlowAnonymousFunctionOperation.
 
-			TValue instanceValue = Visit (memberReference.Instance, state);
+					// Instance of a lambda or local function should be the instance of the containing method.
+					// Don't need to track a dataflow value, since the delegate creation will warn if the
+					// lambda or local function has an annotated this parameter.
+					targetMethodSymbol = lambda.Symbol;
+					break;
+				case IMethodReferenceOperation methodReference:
+					IMethodSymbol method = methodReference.Method.OriginalDefinition;
+					if (method.ContainingSymbol is IMethodSymbol) {
+						// Track references to local functions
+						var localFunction = method;
+						Debug.Assert (localFunction.MethodKind == MethodKind.LocalFunction);;
+						var localFunctionCFG = ControlFlowGraph.GetLocalFunctionControlFlowGraphInScope (localFunction);
+						InterproceduralState.TrackMethod (new MethodBodyValue (localFunction, localFunctionCFG));
+					}
+					targetMethodSymbol = method;
+					break;
+				case IMemberReferenceOperation:
+				case IInvocationOperation:
+					// No method symbol.
+					break;
+				default:
+					// Unimplemented case that might need special handling.
+					// Fail in debug mode only.
+					Debug.Fail ($"{operation.Target.GetType ()}: {operation.Target.Syntax.GetLocation ().GetLineSpan ()}");
+					break;
+			}
 
-			if (memberReference.Member is not IMethodSymbol method)
+			if (targetMethodSymbol == null)
 				return TopValue;
 
-			// Track references to local functions
-			if (method.OriginalDefinition.ContainingSymbol is IMethodSymbol) {
-				var localFunction = method.OriginalDefinition;
-				Debug.Assert (localFunction.MethodKind == MethodKind.LocalFunction);;
-				var localFunctionCFG = ControlFlowGraph.GetLocalFunctionControlFlowGraphInScope (localFunction);
-				InterproceduralState.TrackMethod (new MethodBodyValue (localFunction, localFunctionCFG));
-			}
-
-			return HandleDelegateCreation (method, instanceValue, operation);
+			return HandleDelegateCreation (targetMethodSymbol, operation);
 		}
 
-		public abstract TValue HandleDelegateCreation (IMethodSymbol methodReference, TValue instance, IOperation operation);
+		public abstract TValue HandleDelegateCreation (IMethodSymbol methodReference, IOperation operation);
 
 		public override TValue VisitPropertyReference (IPropertyReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
 		{
-			if (!operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read))
+			if (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write)) {
+				// Property references may be passed as ref/out parameters.
+				// Enable this assert once we have support for deconstruction assignments.
+				// https://github.com/dotnet/linker/issues/3158
+				// Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Reference),
+				// $"{operation.Syntax.GetLocation ().GetLineSpan ()}");
 				return TopValue;
+			}
 
 			// Accessing property for reading is really a call to the getter
 			// The setter case is handled in assignment operation since here we don't have access to the value to pass to the setter
@@ -516,11 +554,27 @@ public override TValue VisitPropertyReference (IPropertyReferenceOperation opera
 			return HandleMethodCall (getMethod!, instanceValue, arguments.ToImmutableArray (), operation);
 		}
 
-		public override TValue VisitImplicitIndexerReference (IImplicitIndexerReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
+		public override TValue VisitEventReference (IEventReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
 		{
+			// Writing to an event should not go through this path.
+			Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read));
 			if (!operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read))
 				return TopValue;
 
+			Visit (operation.Instance, state);
+			// Accessing event for reading retrieves the event delegate from the event's backing field,
+			// so there is no method call to handle.
+			return TopValue;
+		}
+
+		public override TValue VisitImplicitIndexerReference (IImplicitIndexerReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
+		{
+			if (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write)) {
+				// Implicit indexer references may be passed as ref/out parameters.
+				Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Reference));
+				return TopValue;
+			}
+
 			TValue instanceValue = Visit (operation.Instance, state);
 			TValue indexArgumentValue = Visit (operation.Argument, state);
 

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

@@ -0,0 +1,48 @@
+// 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.Generic;
+using System.Collections.Immutable;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace ILLink.RoslynAnalyzer
+{
+	public readonly struct DataFlowAnalyzerContext
+	{
+		private readonly Dictionary<RequiresAnalyzerBase, ImmutableArray<ISymbol>> _enabledAnalyzers;
+
+		public IEnumerable<RequiresAnalyzerBase> EnabledRequiresAnalyzers => _enabledAnalyzers.Keys;
+
+		public ImmutableArray<ISymbol> GetSpecialIncompatibleMembers (RequiresAnalyzerBase analyzer)
+		{
+			if (!_enabledAnalyzers.TryGetValue (analyzer, out var members))
+				throw new System.ArgumentException ($"Analyzer {analyzer.GetType ().Name} is not in the cache");
+			return members;
+		}
+
+		public readonly bool EnableTrimAnalyzer { get; }
+
+		public readonly bool AnyAnalyzersEnabled => EnableTrimAnalyzer || _enabledAnalyzers.Count > 0;
+
+		DataFlowAnalyzerContext (Dictionary<RequiresAnalyzerBase, ImmutableArray<ISymbol>> enabledAnalyzers, bool enableTrimAnalyzer)
+		{
+			_enabledAnalyzers = enabledAnalyzers;
+			EnableTrimAnalyzer = enableTrimAnalyzer;
+		}
+
+		public static DataFlowAnalyzerContext Create (AnalyzerOptions options, Compilation compilation, ImmutableArray<RequiresAnalyzerBase> requiresAnalyzers)
+		{
+			var enabledAnalyzers = new Dictionary<RequiresAnalyzerBase, ImmutableArray<ISymbol>> ();
+			foreach (var analyzer in requiresAnalyzers) {
+				if (analyzer.IsAnalyzerEnabled (options)) {
+					var incompatibleMembers = analyzer.GetSpecialIncompatibleMembers (compilation);
+					enabledAnalyzers.Add (analyzer, incompatibleMembers);
+				}
+			}
+			return new DataFlowAnalyzerContext (
+				enabledAnalyzers,
+				options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer));
+		}
+	}
+}

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

@@ -25,6 +25,12 @@ public class DynamicallyAccessedMembersAnalyzer : DiagnosticAnalyzer
 		internal const string DynamicallyAccessedMembersAttribute = nameof (DynamicallyAccessedMembersAttribute);
 		public const string attributeArgument = "attributeArgument";
 		public const string FullyQualifiedDynamicallyAccessedMembersAttribute = "System.Diagnostics.CodeAnalysis." + DynamicallyAccessedMembersAttribute;
+		public static Lazy<ImmutableArray<RequiresAnalyzerBase>> RequiresAnalyzers { get; } = new Lazy<ImmutableArray<RequiresAnalyzerBase>> (GetRequiresAnalyzers);
+		static ImmutableArray<RequiresAnalyzerBase> GetRequiresAnalyzers () =>
+			ImmutableArray.Create<RequiresAnalyzerBase> (
+				new RequiresAssemblyFilesAnalyzer (),
+				new RequiresUnreferencedCodeAnalyzer (),
+				new RequiresDynamicCodeAnalyzer ());
 
 		public static ImmutableArray<DiagnosticDescriptor> GetSupportedDiagnostics ()
 		{
@@ -49,6 +55,12 @@ public static ImmutableArray<DiagnosticDescriptor> GetSupportedDiagnostics ()
 			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnrecognizedTypeNameInTypeGetType));
 			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnrecognizedParameterInMethodCreateInstance));
 			diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.ParametersOfAssemblyCreateInstanceCannotBeAnalyzed));
+
+			foreach (var requiresAnalyzer in RequiresAnalyzers.Value) {
+				foreach (var diagnosticDescriptor in requiresAnalyzer.SupportedDiagnostics)
+					diagDescriptorsArrayBuilder.Add (diagnosticDescriptor);
+			}
+
 			return diagDescriptorsArrayBuilder.ToImmutable ();
 
 			void AddRange (DiagnosticId first, DiagnosticId last)
@@ -72,20 +84,23 @@ public override void Initialize (AnalysisContext context)
 				context.EnableConcurrentExecution ();
 			context.ConfigureGeneratedCodeAnalysis (GeneratedCodeAnalysisFlags.ReportDiagnostics);
 			context.RegisterCompilationStartAction (context => {
-				if (!context.Options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer, context.Compilation))
+				var dataFlowAnalyzerContext = DataFlowAnalyzerContext.Create (context.Options, context.Compilation, RequiresAnalyzers.Value);
+				if (!dataFlowAnalyzerContext.AnyAnalyzersEnabled)
 					return;
 
 				context.RegisterOperationBlockAction (context => {
-					if (context.OwningSymbol.IsInRequiresUnreferencedCodeAttributeScope (out _))
-						return;
-
 					foreach (var operationBlock in context.OperationBlocks) {
 						TrimDataFlowAnalysis trimDataFlowAnalysis = new (context, operationBlock);
 						trimDataFlowAnalysis.InterproceduralAnalyze ();
-						foreach (var diagnostic in trimDataFlowAnalysis.TrimAnalysisPatterns.CollectDiagnostics ())
+						foreach (var diagnostic in trimDataFlowAnalysis.CollectDiagnostics (dataFlowAnalyzerContext))
 							context.ReportDiagnostic (diagnostic);
 					}
 				});
+
+				// Remaining actions are only for DynamicallyAccessedMembers analysis.
+				if (!dataFlowAnalyzerContext.EnableTrimAnalyzer)
+					return;
+
 				// Examine generic instantiations in base types and interface list
 				context.RegisterSymbolAction (context => {
 					var type = (INamedTypeSymbol) context.Symbol;