[DAM analyzer] Add support for properties and enable more tests

src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Diagnostics;
 using ILLink.RoslynAnalyzer.TrimAnalysis;
 using ILLink.Shared.DataFlow;
 using Microsoft.CodeAnalysis;
@@ -76,11 +77,15 @@ public void Transfer (BlockProxy block, LocalState<TValue> state)
 		public abstract void HandleAssignment (TValue source, TValue target, IOperation operation);
 
 		// This is called to handle instance method invocations, where "receiver" is the
-		// analyzed value for the object on which the instance method is called.
-		public abstract void HandleReceiverArgument (TValue receiver, IInvocationOperation operation);
+		// analyzed value for the object on which the instance method is called, and similarly
+		// for property references.
+		public abstract void HandleReceiverArgument (TValue receiver, IMethodSymbol targetMethod, IOperation operation);
 
 		public abstract void HandleArgument (TValue argument, IArgumentOperation operation);
 
+		// Called for property setters which are essentially like arguments passed to a method.
+		public abstract void HandlePropertySetterArgument (TValue value, IMethodSymbol setMethod, ISimpleAssignmentOperation operation);
+
 		// This takes an IOperation rather than an IReturnOperation because the return value
 		// may (must?) come from BranchValue of an operation whose FallThroughSuccessor is the exit block.
 		public abstract void HandleReturnValue (TValue returnValue, IOperation operation);
@@ -115,12 +120,19 @@ public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operati
 				// Doesn't get called for assignments to locals, which are handled above.
 				HandleAssignment (value, targetValue, operation);
 				break;
-			case IPropertyReferenceOperation:
+			case IPropertyReferenceOperation propertyRef:
+				// A property assignment is really a call to the property setter.
+				var setMethod = propertyRef.Property.SetMethod;
+				Debug.Assert (setMethod != null);
+				HandlePropertySetterArgument (value, setMethod!, operation);
+				break;
 			// TODO: when setting a property in an attribute, target is an IPropertyReference.
 			case IArrayElementReferenceOperation:
 				// TODO
 				break;
-			// TODO: DiscardOperation
+			case IDiscardOperation:
+				// Assignments like "_ = SomeMethod();" don't need dataflow tracking.
+				break;
 			default:
 				throw new NotImplementedException (operation.Target.GetType ().ToString ());
 			}
@@ -153,7 +165,7 @@ public override TValue VisitInvocation (IInvocationOperation operation, LocalSta
 		{
 			if (operation.Instance != null) {
 				var instanceValue = Visit (operation.Instance, state);
-				HandleReceiverArgument (instanceValue, operation);
+				HandleReceiverArgument (instanceValue, operation.TargetMethod, operation);
 			}
 
 			foreach (var argument in operation.Arguments)
@@ -162,6 +174,37 @@ public override TValue VisitInvocation (IInvocationOperation operation, LocalSta
 			return TopValue;
 		}
 
+		public static IMethodSymbol GetPropertyMethod (IPropertyReferenceOperation operation)
+		{
+			// The IPropertyReferenceOperation doesn't tell us whether this reference is to the getter or setter.
+			// For this we need to look at the containing operation.
+			var parent = operation.Parent;
+			Debug.Assert (parent != null);
+			if (parent!.Kind == OperationKind.SimpleAssignment) {
+				var assignment = (ISimpleAssignmentOperation) parent;
+				if (assignment.Target == operation) {
+					var setMethod = operation.Property.SetMethod;
+					Debug.Assert (setMethod != null);
+					return setMethod!;
+				}
+				Debug.Assert (assignment.Value == operation);
+			}
+
+			var getMethod = operation.Property.GetMethod;
+			Debug.Assert (getMethod != null);
+			return getMethod!;
+		}
+
+		public override TValue VisitPropertyReference (IPropertyReferenceOperation operation, LocalState<TValue> state)
+		{
+			if (operation.Instance != null) {
+				var instanceValue = Visit (operation.Instance, state);
+				HandleReceiverArgument (instanceValue, GetPropertyMethod (operation), operation);
+			}
+
+			return TopValue;
+		}
+
 		public override TValue VisitArgument (IArgumentOperation operation, LocalState<TValue> state)
 		{
 			var value = Visit (operation.Value, state);

src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs

@@ -82,7 +82,9 @@ internal static bool TryGetOverriddenMember (this ISymbol? symbol, [NotNullWhen
 				memberOptions:
 					SymbolDisplayMemberOptions.IncludeParameters |
 					SymbolDisplayMemberOptions.IncludeExplicitInterface,
-				parameterOptions: SymbolDisplayParameterOptions.IncludeType
+				parameterOptions:
+					SymbolDisplayParameterOptions.IncludeType |
+					SymbolDisplayParameterOptions.IncludeParamsRefOut
 			);
 
 		public static string GetDisplayName (this ISymbol symbol)

src/ILLink.RoslynAnalyzer/TrimAnalysis/SymbolValue.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Text;
 using ILLink.Shared;
@@ -30,10 +31,41 @@ public record SymbolValue : ValueWithDynamicallyAccessedMembers
 
 		public SymbolValue (ITypeParameterSymbol typeParameter) => Source = typeParameter;
 
-		public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes =>
-			IsMethodReturn
-				? ((IMethodSymbol) Source).GetDynamicallyAccessedMemberTypesOnReturnType ()
-				: Source.GetDynamicallyAccessedMemberTypes ();
+		public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes {
+			get {
+				if (IsMethodReturn) {
+					var method = (IMethodSymbol) Source;
+					var returnDamt = method.GetDynamicallyAccessedMemberTypesOnReturnType ();
+					// Is this a property getter?
+					// If there are conflicts between the getter and the property annotation,
+					// the getter annotation wins. (But DAMT.None is ignored)
+					if (method.MethodKind is MethodKind.PropertyGet && returnDamt == DynamicallyAccessedMemberTypes.None) {
+						var property = (IPropertySymbol) method.AssociatedSymbol!;
+						Debug.Assert (property != null);
+						returnDamt = property!.GetDynamicallyAccessedMemberTypes ();
+					}
+
+					return returnDamt;
+				}
+
+				var damt = Source.GetDynamicallyAccessedMemberTypes ();
+
+				// Is this a property setter parameter?
+				if (Source.Kind == SymbolKind.Parameter) {
+					var parameterMethod = (IMethodSymbol) Source.ContainingSymbol;
+					Debug.Assert (parameterMethod != null);
+					// If there are conflicts between the setter and the property annotation,
+					// the setter annotation wins. (But DAMT.None is ignored)
+					if (parameterMethod!.MethodKind == MethodKind.PropertySet && damt == DynamicallyAccessedMemberTypes.None) {
+						var property = (IPropertySymbol) parameterMethod.AssociatedSymbol!;
+						Debug.Assert (property != null);
+						damt = property!.GetDynamicallyAccessedMemberTypes ();
+					}
+				}
+
+				return damt;
+			}
+		}
 
 		public override string ToString ()
 		{

src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs

@@ -15,7 +15,6 @@ public class TrimAnalysisVisitor : LocalDataFlowVisitor<MultiValue, ValueSetLatt
 	{
 		public readonly TrimAnalysisPatternStore TrimAnalysisPatterns;
 
-
 		public TrimAnalysisVisitor (
 			LocalStateLattice<MultiValue, ValueSetLattice<SingleValue>> lattice,
 			OperationBlockAnalysisContext context
@@ -42,6 +41,21 @@ public override MultiValue VisitInvocation (IInvocationOperation operation, Stat
 			return new MultiValue (new SymbolValue (operation.TargetMethod, isMethodReturn: true));
 		}
 
+		// Just like VisitInvocation for a method call, we need to visit a property method invocation
+		// in case it has an annotated return value.
+		public override MultiValue VisitPropertyReference (IPropertyReferenceOperation operation, StateValue state)
+		{
+			// Base logic visits the receiver
+			base.VisitPropertyReference (operation, state);
+
+			var propertyMethod = GetPropertyMethod (operation);
+			// Only the getter has a return value that may be annotated.
+			if (propertyMethod.MethodKind == MethodKind.PropertyGet)
+				return new MultiValue (new SymbolValue (propertyMethod, isMethodReturn: true));
+
+			return TopValue;
+		}
+
 		public override MultiValue VisitConversion (IConversionOperation operation, StateValue state)
 		{
 			var value = base.VisitConversion (operation, state);
@@ -86,7 +100,7 @@ public override MultiValue VisitTypeOf (ITypeOfOperation typeOfOperation, StateV
 
 		// Override handlers for situations where annotated locations may be involved in reflection access:
 		// - assignments
-		// - arguments passed to method parameters
+		// - arguments passed to method parameters (or implicitly passed to property setters)
 		//   this also needs to create the annotated value for parameters, because they are not represented
 		//   as 'IParameterReferenceOperation' when passing arguments
 		// - instance passed as explicit or implicit receiver to a method invocation
@@ -118,16 +132,23 @@ public override void HandleArgument (MultiValue argumentValue, IArgumentOperatio
 			));
 		}
 
-		public override void HandleReceiverArgument (MultiValue receieverValue, IInvocationOperation operation)
+		// Similar to HandleArgument, for an assignment operation that is really passing an argument to a property setter.
+		public override void HandlePropertySetterArgument (MultiValue value, IMethodSymbol setMethod, ISimpleAssignmentOperation operation)
 		{
-			if (operation.Instance == null)
-				return;
+			var parameter = new MultiValue (new SymbolValue (setMethod.Parameters[0]));
 
-			MultiValue implicitReceiverParameter = new MultiValue (new SymbolValue (operation.TargetMethod, isMethodReturn: false));
+			TrimAnalysisPatterns.Add (new TrimAnalysisPattern (value, parameter, operation));
+		}
+
+		// Can be called for an invocation or a propertyreference
+		// where the receiver is not null (so an instance method/property).
+		public override void HandleReceiverArgument (MultiValue receiverValue, IMethodSymbol targetMethod, IOperation operation)
+		{
+			MultiValue thisParameter = new MultiValue (new SymbolValue (targetMethod!, isMethodReturn: false));
 
 			TrimAnalysisPatterns.Add (new TrimAnalysisPattern (
-				receieverValue,
-				implicitReceiverParameter,
+				receiverValue,
+				thisParameter,
 				operation
 			));
 		}

test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs

@@ -13,7 +13,7 @@ public Task AnnotatedMembersAccessedViaReflection ()
 			return RunTest (nameof (AnnotatedMembersAccessedViaReflection));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task ApplyTypeAnnotations ()
 		{
 			return RunTest (nameof (ApplyTypeAnnotations));
@@ -25,37 +25,37 @@ public Task AssemblyQualifiedNameDataflow ()
 			return RunTest (nameof (AssemblyQualifiedNameDataflow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task AttributeConstructorDataflow ()
 		{
 			return RunTest (nameof (AttributeConstructorDataflow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task AttributeFieldDataflow ()
 		{
 			return RunTest (nameof (AttributeFieldDataflow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task AttributePropertyDataflow ()
 		{
 			return RunTest (nameof (AttributePropertyDataflow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task ByRefDataflow ()
 		{
 			return RunTest (nameof (ByRefDataflow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task ComplexTypeHandling ()
 		{
 			return RunTest (nameof (ComplexTypeHandling));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task DynamicDependencyDataflow ()
 		{
 			return RunTest (nameof (DynamicDependencyDataflow));
@@ -67,7 +67,7 @@ public Task EmptyArrayIntrinsicsDataFlow ()
 			return RunTest (nameof (EmptyArrayIntrinsicsDataFlow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task FieldDataFlow ()
 		{
 			return RunTest (nameof (FieldDataFlow));
@@ -97,7 +97,7 @@ public Task GetTypeDataFlow ()
 			return RunTest (nameof (GetTypeDataFlow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task IReflectDataflow ()
 		{
 			return RunTest (nameof (IReflectDataflow));
@@ -109,19 +109,19 @@ public Task LocalDataFlow ()
 			return RunTest (nameof (LocalDataFlow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task LocalDataFlowKeptMembers ()
 		{
 			return RunTest (nameof (LocalDataFlowKeptMembers));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task MemberTypes ()
 		{
 			return RunTest (nameof (MemberTypes));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task MemberTypesAllOnCopyAssembly ()
 		{
 			return RunTest (nameof (MemberTypesAllOnCopyAssembly));
@@ -145,13 +145,13 @@ public Task MethodReturnParameterDataFlow ()
 			return RunTest (nameof (MethodReturnParameterDataFlow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task MethodThisDataFlow ()
 		{
 			return RunTest (nameof (MethodThisDataFlow));
 		}
 
-		[Fact (Skip = "https://github.com/dotnet/linker/issues/2273")]
+		[Fact]
 		public Task PropertyDataFlow ()
 		{
 			return RunTest (nameof (PropertyDataFlow));

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

@@ -65,14 +65,30 @@ public static async Task RunTestFile (string suiteName, string testName, params
 		private static IEnumerable<string> GetTestDependencies (string rootSourceDir, SyntaxTree testSyntaxTree)
 		{
 			foreach (var attribute in testSyntaxTree.GetRoot ().DescendantNodes ().OfType<AttributeSyntax> ()) {
-				if (attribute.Name.ToString () != "SetupCompileBefore")
+				var attributeName = attribute.Name.ToString ();
+				if (attributeName != "SetupCompileBefore" && attributeName != "SandboxDependency")
 					continue;
 
-				var testNamespace = testSyntaxTree.GetRoot ().DescendantNodes ().OfType<NamespaceDeclarationSyntax> ().Single ().Name.ToString ();
+				var testNamespace = testSyntaxTree.GetRoot ().DescendantNodes ().OfType<NamespaceDeclarationSyntax> ().First ().Name.ToString ();
 				var testSuiteName = testNamespace.Substring (testNamespace.LastIndexOf ('.') + 1);
 				var args = LinkerTestBase.GetAttributeArguments (attribute);
-				foreach (var sourceFile in ((ImplicitArrayCreationExpressionSyntax) args["#1"]).DescendantNodes ().OfType<LiteralExpressionSyntax> ())
-					yield return Path.Combine (rootSourceDir, testSuiteName, LinkerTestBase.GetStringFromExpression (sourceFile));
+
+				switch (attributeName) {
+				case "SetupCompileBefore": {
+						foreach (var sourceFile in ((ImplicitArrayCreationExpressionSyntax) args["#1"]).DescendantNodes ().OfType<LiteralExpressionSyntax> ())
+							yield return Path.Combine (rootSourceDir, testSuiteName, LinkerTestBase.GetStringFromExpression (sourceFile));
+						break;
+					}
+				case "SandboxDependency": {
+						var sourceFile = LinkerTestBase.GetStringFromExpression (args["#0"]);
+						if (!sourceFile.EndsWith (".cs"))
+							throw new NotSupportedException ();
+						yield return Path.Combine (rootSourceDir, testSuiteName, sourceFile);
+						break;
+					}
+				default:
+					throw new InvalidOperationException ();
+				}
 			}
 		}