Reference live ILLink analyzer (#93259)

In faf883d I inadvertently
turned off the trim analyzer for libraries, because the analyzer
bits don't flow to the consuming project with the ILLink.Tasks
ProjectReference. This adds a separate ProjectReference to use
the live analyzer. This addresses
#88901 for the ILLink
analyzers.

Using the live analyzer uncovered some bugs that are fixed in
this change:

- Invalid assert for field initializers with `throw` statements

- Invalid cast to `IMethodSymbol` for the ContainingSymbol of a
  local. Locals can occur in field initializers, when created
  from out params.

- Wrong warning code produced for assignment to annotated
  property in an initializer. This was being treated as a call to
  the setter instead of an assignment to the underlying field:

  `Type AnnotatedPropertyWithSetter { get; set; } = GetUnknown ();`

- Invalid assert for delegate creation where the delegate is
  created from anything other than a method (for example, a field
  that has an Action or Func type).

- Assert inside GetFlowCaptureValue for the LHS of a compound
  assignment. I couldn't think of a case yet where we actually
  need to model the result of the assignment, so I implemented
  support for at least visiting the LHS by going through the same
  path as normal assignments (avoiding GetFlowCaptureValue).

Author: sbomer

eng/liveILLink.targets

@@ -39,6 +39,13 @@
         <SetTargetFramework Condition="'$(MSBuildRuntimeType)' == 'Core'">TargetFramework=$(NetCoreAppToolCurrent)</SetTargetFramework>
         <SetTargetFramework Condition="'$(MSBuildRuntimeType)' != 'Core'">TargetFramework=$(NetFrameworkToolCurrent)</SetTargetFramework>
     </ProjectReference>
+
+    <!-- Need to reference the analyzer project separately, because there's no easy way to get it as a transitive reference of ILLink.Tasks.csproj -->
+    <ProjectReference Include="$(_ILLinkTasksSourceDir)..\ILLink.RoslynAnalyzer\ILLink.RoslynAnalyzer.csproj"
+                      ReferenceOutputAssembly="false"
+                      Private="false"
+                      OutputItemType="Analyzer"
+                      SetConfiguration="Configuaration=$(ToolsConfiguration)" />
   </ItemGroup>
 
 </Project>

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

@@ -76,14 +76,19 @@ public void Transfer (BlockProxy block, LocalDataFlowState<TValue, TValueLattice
 
 			// If not, the BranchValue represents a return or throw value associated with the FallThroughSuccessor of this block.
 			// (ConditionalSuccessor == null iff ConditionKind == None).
-			// If we get here, we should be analyzing a method body, not an attribute instance since attributes can't have throws or return statements
-			// Field/property initializers also can't have throws or return statements.
-			Debug.Assert (OwningSymbol is IMethodSymbol);
+			// If we get here, we should be analyzing code in a method or field/property initializer,
+			// not an attribute instance, since attributes can't have throws or return statements
+			Debug.Assert (OwningSymbol is IMethodSymbol or IFieldSymbol or IPropertySymbol,
+				$"{OwningSymbol.GetType ()}: {branchValueOperation.Syntax.GetLocation ().GetLineSpan ()}");
 
 			// The BranchValue for a thrown value is not involved in dataflow tracking.
 			if (block.Block.FallThroughSuccessor?.Semantics == ControlFlowBranchSemantics.Throw)
 				return;
 
+			// Field/property initializers can't have return statements.
+			Debug.Assert (OwningSymbol is IMethodSymbol,
+				$"{OwningSymbol.GetType ()}: {branchValueOperation.Syntax.GetLocation ().GetLineSpan ()}");
+
 			// Return statements with return values are represented in the control flow graph as
 			// a branch value operation that computes the return value.
 
@@ -125,9 +130,9 @@ bool IsReferenceToCapturedVariable (ILocalReferenceOperation localReference)
 
 			if (local.IsConst)
 				return false;
-
-			var declaringSymbol = (IMethodSymbol) local.ContainingSymbol;
-			return !ReferenceEquals (declaringSymbol, OwningSymbol);
+			Debug.Assert (local.ContainingSymbol is IMethodSymbol or IFieldSymbol, // backing field for property initializers
+				$"{local.ContainingSymbol.GetType ()}: {localReference.Syntax.GetLocation ().GetLineSpan ()}");
+			return !ReferenceEquals (local.ContainingSymbol, OwningSymbol);
 		}
 
 		TValue GetLocal (ILocalReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
@@ -159,7 +164,7 @@ void SetLocal (ILocalReferenceOperation operation, TValue value, LocalDataFlowSt
 			state.Set (local, newValue);
 		}
 
-		TValue ProcessSingleTargetAssignment (IOperation targetOperation, ISimpleAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state, bool merge)
+		TValue ProcessSingleTargetAssignment (IOperation targetOperation, IAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state, bool merge)
 		{
 			switch (targetOperation) {
 			case IFieldReferenceOperation:
@@ -187,9 +192,14 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, ISimpleAssignm
 						// This can happen in a constructor - there it is possible to assign to a property
 						// without a setter. This turns into an assignment to the compiler-generated backing field.
 						// To match the linker, this should warn about the compiler-generated backing field.
-						// For now, just don't warn. https://github.com/dotnet/linker/issues/2731
+						// For now, just don't warn. https://github.com/dotnet/runtime/issues/93277
 						break;
 					}
+					// Even if the property has a set method, if the assignment takes place in a property initializer,
+					// the write becomes a direct write to the underlying field. This should be treated the same as
+					// the case where there is no set method.
+					if (OwningSymbol is IPropertySymbol && (ControlFlowGraph.OriginalOperation is not IAttributeOperation))
+						break;
 
 					// Property may be an indexer, in which case there will be one or more index arguments followed by a value argument
 					ImmutableArray<TValue>.Builder arguments = ImmutableArray.CreateBuilder<TValue> ();
@@ -293,6 +303,20 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, ISimpleAssignm
 		}
 
 		public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
+		{
+			return ProcessAssignment (operation, state);
+		}
+
+		public override TValue VisitCompoundAssignment (ICompoundAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
+		{
+			return ProcessAssignment (operation, state);
+		}
+
+		// Note: this is called both for normal assignments and ICompoundAssignmentOperation.
+		// The resulting value of a compound assignment isn't important for our dataflow analysis
+		// (we don't model addition of integers, for example), so we just treat these the same
+		// as normal assignments.
+		TValue ProcessAssignment (IAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
 		{
 			var targetOperation = operation.Target;
 			if (targetOperation is not IFlowCaptureReferenceOperation flowCaptureReference)
@@ -305,7 +329,7 @@ public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operati
 			// for simplicity. This could be generalized if we encounter a dataflow behavior where this makes a difference.
 
 			Debug.Assert (IsLValueFlowCapture (flowCaptureReference.Id));
-			Debug.Assert (!flowCaptureReference.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read));
+			Debug.Assert (flowCaptureReference.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write));
 			var capturedReferences = state.Current.CapturedReferences.Get (flowCaptureReference.Id);
 			if (!capturedReferences.HasMultipleValues) {
 				// Single captured reference. Treat this as an overwriting assignment.
@@ -360,8 +384,31 @@ public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation
 				// Debug.Assert (IsLValueFlowCapture (operation.Id));
 				Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write),
 					$"{operation.Syntax.GetLocation ().GetLineSpan ()}");
+				Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Reference),
+					$"{operation.Syntax.GetLocation ().GetLineSpan ()}");
 				return TopValue;
 			}
+
+			if (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write)) {
+				// If we get here, it means we're visiting a flow capture reference that may be
+				// assigned to. Similar to the IsInitialization case, this can happen for an out param
+				// where the variable is declared before being passed as an out param, for example:
+
+				// string s;
+				// Method (out s, b ? 0 : 1);
+
+				// The second argument is necessary to create multiple branches so that the compiler
+				// turns both arguments into flow capture references, instead of just passing a local
+				// reference for s.
+
+				// This can also happen for a deconstruction assignments, where the write is not to a byref.
+				// Once the analyzer implements support for deconstruction assignments (https://github.com/dotnet/linker/issues/3158),
+				// we can try enabling this assert to ensure that this case is only hit for byrefs.
+				// Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Reference),
+				//     $"{operation.Syntax.GetLocation ().GetLineSpan ()}");
+				return TopValue;
+			}
+
 			return GetFlowCaptureValue (operation, state);
 		}
 
@@ -428,14 +475,14 @@ public override TValue VisitDelegateCreation (IDelegateCreationOperation operati
 				return HandleDelegateCreation (lambda.Symbol, instance, operation);
 			}
 
-			Debug.Assert (operation.Target is IMethodReferenceOperation);
-			if (operation.Target is not IMethodReferenceOperation methodReference)
+			Debug.Assert (operation.Target is IMemberReferenceOperation,
+				$"{operation.Target.GetType ()}: {operation.Syntax.GetLocation ().GetLineSpan ()}");
+			if (operation.Target is not IMemberReferenceOperation memberReference)
 				return TopValue;
 
-			TValue instanceValue = Visit (methodReference.Instance, state);
-			IMethodSymbol? method = methodReference.Method;
-			Debug.Assert (method != null);
-			if (method == null)
+			TValue instanceValue = Visit (memberReference.Instance, state);
+
+			if (memberReference.Member is not IMethodSymbol method)
 				return TopValue;
 
 			// Track references to local functions

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs

@@ -28,6 +28,7 @@ public static void Main ()
 			AnnotatedGenerics.Test ();
 			AnnotationOnGenerics.Test ();
 			AnnotationOnInteropMethod.Test ();
+			DelegateCreation.Test ();
 		}
 
 		class AnnotatedField
@@ -874,6 +875,39 @@ public static void Test ()
 			}
 		}
 
+		class DelegateCreation
+		{
+			delegate void UnannotatedDelegate (Type type);
+
+			static Action<Type> field;
+
+			static Action<Type> Property { get; set; }
+
+			[ExpectedWarning ("IL2111", "LocalMethod")]
+			[ExpectedWarning ("IL2111")]
+			public static void Test ()
+			{
+				// Check that the analyzer is able to analyze delegate creation
+				// with various targets, without hitting an assert.
+				UnannotatedDelegate d;
+				d = new UnannotatedDelegate (field);
+				d(typeof(int));
+				d = new UnannotatedDelegate (Property);
+				d(typeof(int));
+
+				d = new UnannotatedDelegate (
+					([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t) =>
+					{ });
+				d(typeof(int));
+				d = new UnannotatedDelegate (LocalMethod);
+				d(typeof(int));
+
+				void LocalMethod (
+					[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type)
+				{ }
+			}
+		}
+
 		class TestType { }
 
 		[RequiresUnreferencedCode ("--RequiresUnreferencedCodeType--")]

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ConstructedTypesDataFlow.cs

@@ -28,6 +28,22 @@ static void DeconstructVariableNoAnnotation ((Type type, object instance) input)
 				type.RequiresPublicMethods ();
 			}
 
+			static (Type type, object instance) GetInput (int unused) => (typeof (string), null);
+
+			// https://github.com/dotnet/linker/issues/3158
+			[ExpectedWarning ("IL2077", ProducedBy = Tool.Trimmer | Tool.NativeAot)]
+			static void DeconstructVariableFlowCapture (bool b = true)
+			{
+				// This creates a control-flow graph where the tuple elements assigned to
+				// are flow capture references. This is only the case when the variable types
+				// are declared before the deconstruction assignment, and the assignment creates
+				// a branch in the control-flow graph.
+				Type type;
+				object instance;
+				(type, instance) = GetInput (b ? 0 : 1);
+				type.RequiresPublicMethods ();
+			}
+
 			record TypeAndInstance (
 				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
 				[property: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
@@ -97,6 +113,7 @@ static void DeconstructRecordManualWithMismatchAnnotation (TypeAndInstanceRecord
 			public static void Test ()
 			{
 				DeconstructVariableNoAnnotation ((typeof (string), null));
+				DeconstructVariableFlowCapture ();
 				DeconstructRecordWithAnnotation (new (typeof (string), null));
 				DeconstructClassWithAnnotation (new (typeof (string), null));
 				DeconstructRecordManualWithAnnotation (new (typeof (string), null));

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ConstructorDataFlow.cs

@@ -35,6 +35,20 @@ public DataFlowInConstructor ()
 			[ExpectedWarning ("IL2072", nameof (GetUnknown), nameof (RequireAll), CompilerGeneratedCode = true)]
 			int Property { get; } = RequireAll (GetUnknown ());
 
+			[ExpectedWarning ("IL2074", nameof (GetUnknown), nameof (annotatedField), CompilerGeneratedCode = true)]
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
+			Type annotatedField = GetUnknown ();
+
+			[ExpectedWarning ("IL2074", nameof (GetUnknown), nameof (AnnotatedProperty), CompilerGeneratedCode = true,
+				ProducedBy = Tool.Trimmer | Tool.NativeAot)] // https://github.com/dotnet/runtime/issues/93277
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
+			Type AnnotatedProperty { get; } = GetUnknown ();
+
+			[ExpectedWarning ("IL2074", nameof (GetUnknown), nameof (AnnotatedPropertyWithSetter), CompilerGeneratedCode = true,
+				ProducedBy = Tool.Trimmer | Tool.NativeAot)] // https://github.com/dotnet/runtime/issues/93277
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
+			Type AnnotatedPropertyWithSetter { get; set; } = GetUnknown ();
+
 			// The analyzer dataflow visitor asserts that we only see a return value
 			// inside of an IMethodSymbol. This testcase checks that we don't hit asserts
 			// in case the return statement is in a lambda owned by a field.
@@ -63,6 +77,18 @@ public DataFlowInConstructor ()
 
 			static int Execute(Func<int> f) => f();
 
+			int fieldWithThrowStatementInInitializer = string.Empty.Length == 0 ? throw new Exception() : 0;
+
+			int PropertyWithThrowStatementInInitializer { get; } = string.Empty.Length == 0 ? throw new Exception() : 0;
+
+			[ExpectedWarning ("IL2067", nameof (TryGetUnknown), nameof (RequireAll), CompilerGeneratedCode = true,
+				ProducedBy = Tool.Trimmer | Tool.NativeAot)] // https://github.com/dotnet/linker/issues/2158
+			int fieldWithLocalReferenceInInitializer = TryGetUnknown (out var type) ? RequireAll (type) : 0;
+
+			[ExpectedWarning ("IL2067", nameof (TryGetUnknown), nameof (RequireAll), CompilerGeneratedCode = true,
+				ProducedBy = Tool.Trimmer | Tool.NativeAot)] // https://github.com/dotnet/linker/issues/2158
+			int PropertyWithLocalReferenceInInitializer { get; } = TryGetUnknown (out var type) ? RequireAll (type) : 0;
+
 			public static void Test ()
 			{
 				var instance = new DataFlowInConstructor ();
@@ -91,6 +117,12 @@ public static void Test ()
 
 		static Type GetUnknown () => null;
 
+		static bool TryGetUnknown (out Type type)
+		{
+			type = null;
+			return true;
+		}
+
 		static int RequireAll ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type type) => 0;
 	}
 }

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MethodByRefParameterDataFlow.cs

@@ -36,6 +36,7 @@ public static void Main ()
 			Type nullType4 = null;
 			TestAssigningToRefParameter_Mismatch (nullType4, ref nullType4);
 			TestPassingRefsWithImplicitThis ();
+			TestPassingCapturedOutParameter ();
 			LocalMethodsAndLambdas.Test ();
 		}
 
@@ -183,6 +184,25 @@ static void TestPassingRefsWithImplicitThis ()
 			param3.RequiresPublicFields ();
 		}
 
+		static bool TryGetAnnotatedValueWithExtraUnusedParameter (
+			[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] out Type typeWithMethods,
+			int unused)
+		{
+			typeWithMethods = null;
+			return false;
+		}
+
+		static void TestPassingCapturedOutParameter (bool b = true)
+		{
+			Type typeWithMethods;
+			// The ternary operator for the second argument causes _both_ arguments to
+			// become flow-capture references. The ternary operator introduces two separate
+			// branches, where a capture is created for typeWithMethods before the branch
+			// out. This capture is then passed as the first argument.
+			TryGetAnnotatedValueWithExtraUnusedParameter (out typeWithMethods, b ? 0 : 1);
+			typeWithMethods.RequiresPublicMethods ();
+		}
+
 		[return: DynamicallyAccessedMembersAttribute (DynamicallyAccessedMemberTypes.PublicFields)]
 		static Type GetTypeWithFields () => null;
 

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MethodByRefReturnDataFlow.cs

@@ -18,6 +18,9 @@ public static void Main ()
 			AssignDirectlyToAnnotatedTypeReference ();
 			AssignToCapturedAnnotatedTypeReference ();
 			AssignToAnnotatedTypeReferenceWithRequirements ();
+			TestCompoundAssignment (typeof (int));
+			TestCompoundAssignmentCapture (typeof (int));
+			TestCompoundAssignmentMultipleCaptures (typeof (int), typeof (int));
 		}
 
 		[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
@@ -68,6 +71,32 @@ static void AssignToAnnotatedTypeReferenceWithRequirements ()
 			ReturnAnnotatedTypeWithRequirements (GetWithPublicMethods ()) = GetWithPublicFields ();
 		}
 
+		static int intField;
+
+		static ref int GetRefReturnInt ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) => ref intField;
+
+		// Ensure analyzer visits the a ref return in the LHS of a compound assignment.
+		[ExpectedWarning ("IL2067", nameof (GetRefReturnInt), nameof (DynamicallyAccessedMemberTypes) + "." + nameof (DynamicallyAccessedMemberTypes.All))]
+		public static void TestCompoundAssignment (Type t)
+		{
+			GetRefReturnInt (t) += 0;
+		}
+
+		// Ensure analyzer visits LHS of a compound assignment when the assignment target is a flow-capture reference.
+		[ExpectedWarning ("IL2067", nameof (GetRefReturnInt), nameof (DynamicallyAccessedMemberTypes) + "." + nameof (DynamicallyAccessedMemberTypes.All))]
+		public static void TestCompoundAssignmentCapture (Type t, bool b = true)
+		{
+			GetRefReturnInt (t) += b ? 0 : 1;
+		}
+
+		// Same as above, with assignment to a flow-capture reference that references multiple captured values.
+		[ExpectedWarning ("IL2067", nameof (GetRefReturnInt), nameof (DynamicallyAccessedMemberTypes) + "." + nameof (DynamicallyAccessedMemberTypes.All))]
+		[ExpectedWarning ("IL2067", nameof (GetRefReturnInt), nameof (DynamicallyAccessedMemberTypes) + "." + nameof (DynamicallyAccessedMemberTypes.All))]
+		public static void TestCompoundAssignmentMultipleCaptures (Type t, Type u, bool b = true)
+		{
+			(b ? ref GetRefReturnInt (t) : ref GetRefReturnInt (u)) += b ? 0 : 1;
+		}
+
 		[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
 		static Type GetWithPublicMethods () => null;