Update linker and analyzer to handle DAM on indexable property parameters (#3081)

Don't assume there is only one parameter for set and none for get. Allow DAM on the index parameters of properties.
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
Co-authored-by: Sven Boemer <sbomer@gmail.com>

5 files changed, 190 insertions, 14 deletions

diff --git a/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs b/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
index fd9b8ee99..eee0441a6 100644
--- a/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
+++ b/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
@@ -199,7 +199,14 @@ namespace ILLink.RoslynAnalyzer.DataFlow
 						// For now, just don't warn. https://github.com/dotnet/linker/issues/2731
 						break;
 					}
-					HandleMethodCall (setMethod, instanceValue, ImmutableArray.Create (value), operation);
+
+					// 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> ();
+					foreach (var val in propertyRef.Arguments)
+						arguments.Add (Visit (val, state));
+					arguments.Add (value);
+
+					HandleMethodCall (setMethod, instanceValue, arguments.ToImmutableArray (), operation);
 					// The return value of a property set expression is the value,
 					// even though a property setter has no return value.
 					return value;
@@ -340,7 +347,13 @@ namespace ILLink.RoslynAnalyzer.DataFlow
 			// The setter case is handled in assignment operation since here we don't have access to the value to pass to the setter
 			TValue instanceValue = Visit (operation.Instance, state);
 			IMethodSymbol? getMethod = operation.Property.GetGetMethod ();
-			return HandleMethodCall (getMethod!, instanceValue, ImmutableArray<TValue>.Empty, operation);
+
+			// Property may be an indexer, in which case there will be one or more index arguments
+			ImmutableArray<TValue>.Builder arguments = ImmutableArray.CreateBuilder<TValue> ();
+			foreach (var val in operation.Arguments)
+				arguments.Add (Visit (val, state));
+
+			return HandleMethodCall (getMethod!, instanceValue, arguments.ToImmutableArray (), operation);
 		}
 
 		public override TValue VisitImplicitIndexerReference (IImplicitIndexerReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
diff --git a/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs b/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
index 7e24cbf4a..187ac40ea 100644
--- a/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
+++ b/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
@@ -299,7 +299,7 @@ namespace ILLink.RoslynAnalyzer
 				&& methodSymbol.GetDynamicallyAccessedMemberTypesOnReturnType () != DynamicallyAccessedMemberTypes.None
 				// None on parameter of 'set' matches unannotated
 				|| methodSymbol.MethodKind == MethodKind.PropertySet
-				&& methodSymbol.Parameters[0].GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
+				&& methodSymbol.Parameters[methodSymbol.Parameters.Length - 1].GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
 				context.ReportDiagnostic (Diagnostic.Create (
 					DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor),
 					methodSymbol.AssociatedSymbol!.Locations[0],
diff --git a/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs b/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
index d3146c157..207d42447 100644
--- a/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
+++ b/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
@@ -41,12 +41,16 @@ namespace ILLink.Shared.TrimAnalysis
 		{
 			var damt = parameter.GetDynamicallyAccessedMemberTypes ();
 
-			// Is this a property setter parameter?
 			var parameterMethod = (IMethodSymbol) parameter.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) {
+
+			// Is this a property setter `value` parameter?
+			if (parameterMethod!.MethodKind == MethodKind.PropertySet
+				&& damt == DynamicallyAccessedMemberTypes.None
+				&& parameter.Ordinal == parameterMethod.Parameters.Length - 1) {
 				var property = (IPropertySymbol) parameterMethod.AssociatedSymbol!;
 				Debug.Assert (property != null);
 				damt = property!.GetDynamicallyAccessedMemberTypes ();
diff --git a/src/linker/Linker.Dataflow/FlowAnnotations.cs b/src/linker/Linker.Dataflow/FlowAnnotations.cs
index f0c8cce9a..e14b1928c 100644
--- a/src/linker/Linker.Dataflow/FlowAnnotations.cs
+++ b/src/linker/Linker.Dataflow/FlowAnnotations.cs
@@ -231,7 +231,7 @@ namespace ILLink.Shared.TrimAnalysis
 				}
 			}
 
-			var annotatedMethods = new ArrayBuilder<MethodAnnotations> ();
+			var annotatedMethods = new List<MethodAnnotations> ();
 
 			// Next go over all methods with an explicit annotation
 			if (type.HasMethods) {
@@ -343,15 +343,28 @@ namespace ILLink.Shared.TrimAnalysis
 							ScanMethodBodyForFieldAccess (setMethod.Body, write: true, out backingFieldFromSetter);
 						}
 
-						if (annotatedMethods.Any (a => a.Method == setMethod)) {
+						MethodAnnotations? setterAnnotation = null;
+						foreach (var annotatedMethod in annotatedMethods) {
+							if (annotatedMethod.Method == setMethod)
+								setterAnnotation = annotatedMethod;
+						}
+
+						// If 'value' parameter is annotated, then warn. Other parameters can be annotated for indexable properties
+						if (setterAnnotation?.ParameterAnnotations?[^1] is not (null or DynamicallyAccessedMemberTypes.None)) {
 							_context.LogWarning (setMethod, DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor, property.GetDisplayName (), setMethod.GetDisplayName ());
 						} else {
 							int offset = setMethod.HasImplicitThis () ? 1 : 0;
-							if (setMethod.Parameters.Count > 0) {
-								DynamicallyAccessedMemberTypes[] paramAnnotations = new DynamicallyAccessedMemberTypes[setMethod.Parameters.Count + offset];
-								paramAnnotations[paramAnnotations.Length - 1] = annotation;
-								annotatedMethods.Add (new MethodAnnotations (setMethod, paramAnnotations, DynamicallyAccessedMemberTypes.None, null));
-							}
+							if (setterAnnotation is not null)
+								annotatedMethods.Remove (setterAnnotation.Value);
+
+							DynamicallyAccessedMemberTypes[] paramAnnotations;
+							if (setterAnnotation?.ParameterAnnotations is null)
+								paramAnnotations = new DynamicallyAccessedMemberTypes[setMethod.Parameters.Count + offset];
+							else
+								paramAnnotations = setterAnnotation.Value.ParameterAnnotations;
+
+							paramAnnotations[paramAnnotations.Length - 1] = annotation;
+							annotatedMethods.Add (new MethodAnnotations (setMethod, paramAnnotations, DynamicallyAccessedMemberTypes.None, null));
 						}
 					}
 
@@ -369,11 +382,21 @@ namespace ILLink.Shared.TrimAnalysis
 							// that the field (which ever it is) must be annotated as well.
 							ScanMethodBodyForFieldAccess (getMethod.Body, write: false, out backingFieldFromGetter);
 						}
+						MethodAnnotations? getterAnnotation = null;
+						foreach (var annotatedMethod in annotatedMethods) {
+							if (annotatedMethod.Method == getMethod)
+								getterAnnotation = annotatedMethod;
+						}
 
-						if (annotatedMethods.Any (a => a.Method == getMethod)) {
+						// If return value is annotated, then warn. Otherwise, parameters can be annotated for indexable properties
+						if (getterAnnotation?.ReturnParameterAnnotation is not (null or DynamicallyAccessedMemberTypes.None)) {
 							_context.LogWarning (getMethod, DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor, property.GetDisplayName (), getMethod.GetDisplayName ());
 						} else {
-							annotatedMethods.Add (new MethodAnnotations (getMethod, null, annotation, null));
+							int offset = getMethod.HasImplicitThis () ? 1 : 0;
+							if (getterAnnotation is not null)
+								annotatedMethods.Remove (getterAnnotation.Value);
+
+							annotatedMethods.Add (new MethodAnnotations (getMethod, getterAnnotation?.ParameterAnnotations, annotation, null));
 						}
 					}
 
diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/PropertyDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/PropertyDataFlow.cs
index 15508baca..f61de031b 100644
--- a/test/Mono.Linker.Tests.Cases/DataFlow/PropertyDataFlow.cs
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/PropertyDataFlow.cs
@@ -726,12 +726,148 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 				types[index].RequiresAll ();
 			}
 
+			class IndexWithTypeWithDam
+			{
+				class DamOnIndexOnly
+				{
+					int this[[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] Type idx] {
+						get => throw new NotImplementedException ();
+					}
+
+					[ExpectedWarning ("IL2067", "this[Type].get", nameof (ParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Analyzer)]
+					[ExpectedWarning ("IL2067", "Item.get", nameof (ParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Trimmer)]
+					static void ParamDoesNotMeetRequirements (Type t)
+					{
+						var x = new IndexWithTypeWithDam ();
+						_ = x[t];
+					}
+
+					static void ParamDoesMeetRequirements ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] Type t)
+					{
+						var x = new IndexWithTypeWithDam ();
+						_ = x[t];
+					}
+
+					[ExpectedWarning ("IL2087", "this[Type].get", nameof (TypeParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Analyzer)]
+					[ExpectedWarning ("IL2087", "Item.get", nameof (TypeParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Trimmer)]
+					static void TypeParamDoesNotMeetRequirements<T> ()
+					{
+						var x = new IndexWithTypeWithDam ();
+						var t = typeof (T);
+						_ = x[t];
+					}
+
+					static void TypeParamDoesMeetRequirements<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] T> ()
+					{
+						var x = new IndexWithTypeWithDam ();
+						var t = typeof (T);
+						_ = x[t];
+					}
+
+					static void KnownTypeDoesMeetRequirements ()
+					{
+						var x = new IndexWithTypeWithDam ();
+						var t = typeof (int);
+						_ = x[t];
+					}
+					public static void Test ()
+					{
+						ParamDoesMeetRequirements (null);
+						ParamDoesNotMeetRequirements (null);
+						TypeParamDoesMeetRequirements<int> ();
+						TypeParamDoesNotMeetRequirements<int> ();
+						KnownTypeDoesMeetRequirements ();
+					}
+				}
+
+				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+				Type this[[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] Type idx] {
+					get => throw new NotImplementedException ();
+					set => throw new NotImplementedException ();
+				}
+
+				[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
+				static Type fieldWithMethods;
+
+				[ExpectedWarning ("IL2067", "this[Type].get", nameof (ParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Analyzer)]
+				[ExpectedWarning ("IL2067", "Item.get", nameof (ParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Trimmer)]
+				static void ParamDoesNotMeetRequirements (Type t)
+				{
+					var x = new IndexWithTypeWithDam ();
+					fieldWithMethods = x[t];
+				}
+
+				static void ParamDoesMeetRequirements ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] Type t)
+				{
+					var x = new IndexWithTypeWithDam ();
+					fieldWithMethods = x[t];
+				}
+
+				[ExpectedWarning ("IL2087", "this[Type].get", nameof (TypeParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Analyzer)]
+				[ExpectedWarning ("IL2087", "Item.get", nameof (TypeParamDoesNotMeetRequirements), ProducedBy = ProducedBy.Trimmer)]
+				static void TypeParamDoesNotMeetRequirements<T> ()
+				{
+					var x = new IndexWithTypeWithDam ();
+					var t = typeof (T);
+					fieldWithMethods = x[t];
+				}
+
+				static void TypeParamDoesMeetRequirements<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] T> ()
+				{
+					var x = new IndexWithTypeWithDam ();
+					var t = typeof (T);
+					fieldWithMethods = x[t];
+				}
+
+				static void KnownTypeDoesMeetRequirements ()
+				{
+					var x = new IndexWithTypeWithDam ();
+					var t = typeof (int);
+					fieldWithMethods = x[t];
+				}
+
+				[ExpectedWarning ("IL2067", "this[Type].set", nameof (t), "idx", ProducedBy = ProducedBy.Analyzer)]
+				[ExpectedWarning ("IL2067", "Item.set", nameof (t), "idx", ProducedBy = ProducedBy.Trimmer)]
+				static void ValueMeetsRequirementsIndexDoesNot (Type t)
+				{
+					var x = new IndexWithTypeWithDam ();
+					x[t] = fieldWithMethods;
+				}
+
+				[ExpectedWarning ("IL2067", "this[Type].set", nameof (tUnannotated), "value", ProducedBy = ProducedBy.Analyzer)]
+				[ExpectedWarning ("IL2067", "Item.set", nameof (tUnannotated), "value", ProducedBy = ProducedBy.Trimmer)]
+				static void ValueDoesNotMeetRequirementsIndexDoes ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] Type t, Type tUnannotated)
+				{
+					var x = new IndexWithTypeWithDam ();
+					x[t] = tUnannotated;
+				}
+
+				static void ValueAndIndexMeetRequirements ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] Type tFields, [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type tMethods)
+				{
+					var x = new IndexWithTypeWithDam ();
+					x[tFields] = tMethods;
+				}
+
+				public static void Test ()
+				{
+					ParamDoesMeetRequirements (null);
+					ParamDoesNotMeetRequirements (null);
+					TypeParamDoesMeetRequirements<int> ();
+					TypeParamDoesNotMeetRequirements<int> ();
+					KnownTypeDoesMeetRequirements ();
+					ValueMeetsRequirementsIndexDoesNot (null);
+					ValueDoesNotMeetRequirementsIndexDoes (null, null);
+					ValueAndIndexMeetRequirements (null, null);
+					DamOnIndexOnly.Test ();
+				}
+			}
 			public static void Test ()
 			{
 				TestRead ();
 				TestWrite ();
 				TestNullCoalescingAssignment ();
 				TestSpanIndexerAccess ();
+				IndexWithTypeWithDam.Test ();
 			}
 		}