Fix warning for MakeGenericType annotation mismatch (#104921)

Fixes warning code when a generic type whose type parameters have DAM annotations is used with MakeGenericType, over a type that doesn't have matching annotations. The code IL2070 used to mention the 'this' argument. Instead it should have been IL2071 which mentions the generic argument as the cause of the mismatch. Similar for MakeGenericMethod with IL2090 and IL2091.
dotnet · Jul 24, 2024 · efb09a5 · efb09a5
1 parent e75fc27
commit efb09a5
Show file tree

Hide file tree

Showing 16 changed files with 80 additions and 84 deletions.
diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
@@ -1,4 +1,4 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
@@ -958,6 +958,11 @@ internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method, bool
         internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method, bool isNewObj)
             => GetMethodReturnValue(method, isNewObj, GetReturnParameterAnnotation(method.Method));
 
+#pragma warning disable CA1822 // Other partial implementations are not in the ilc project
+        internal partial GenericParameterValue GetGenericParameterValue(GenericParameterProxy genericParameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+#pragma warning restore CA1822 // Mark members as static
+            => new GenericParameterValue(genericParameter.GenericParameter, dynamicallyAccessedMemberTypes);
+
         internal partial GenericParameterValue GetGenericParameterValue(GenericParameterProxy genericParameter)
             => new GenericParameterValue(genericParameter.GenericParameter, GetGenericParameterAnnotation(genericParameter.GenericParameter));
 
@@ -970,18 +975,14 @@ internal partial MethodParameterValue GetMethodParameterValue(ParameterProxy par
             => GetMethodParameterValue(param, GetParameterAnnotation(param));
 
 #pragma warning disable CA1822 // Mark members as static - Should be an instance method for consistency
-        // overrideIsThis is needed for backwards compatibility with MakeGenericType/Method https://github.com/dotnet/linker/issues/2428
-        internal MethodParameterValue GetMethodThisParameterValue(MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
+        internal partial MethodParameterValue GetMethodThisParameterValue(MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
         {
-            if (!method.HasImplicitThis() && !overrideIsThis)
+            if (!method.HasImplicitThis())
                 throw new InvalidOperationException($"Cannot get 'this' parameter of method {method.GetDisplayName()} with no 'this' parameter.");
-            return new MethodParameterValue(new ParameterProxy(method, (ParameterIndex)0), dynamicallyAccessedMemberTypes, overrideIsThis);
+            return new MethodParameterValue(new ParameterProxy(method, (ParameterIndex)0), dynamicallyAccessedMemberTypes);
         }
 #pragma warning restore CA1822 // Mark members as static
 
-        internal partial MethodParameterValue GetMethodThisParameterValue(MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
-            => GetMethodThisParameterValue(method, dynamicallyAccessedMemberTypes, false);
-
         internal partial MethodParameterValue GetMethodThisParameterValue(MethodProxy method)
         {
             if (!method.HasImplicitThis())

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodParameterValue.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodParameterValue.cs
@@ -14,12 +14,11 @@ namespace ILLink.Shared.TrimAnalysis
     /// </summary>
     internal partial record MethodParameterValue
     {
-        public MethodParameterValue(ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
+        public MethodParameterValue(ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
         {
             StaticType = param.ParameterType;
             Parameter = param;
             DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
-            _overrideIsThis = overrideIsThis;
         }
 
         public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }

diff --git a/src/libraries/System.Data.Common/src/System/Data/DataRowExtensions.cs b/src/libraries/System.Data.Common/src/System/Data/DataRowExtensions.cs
@@ -155,7 +155,7 @@ private static class UnboxT<T>
                 }
                 return NonNullableField;
 
-                [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2090:MakeGenericMethod",
+                [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2091:MakeGenericMethod",
                     Justification = "'NullableField<TElem> where TElem : struct' implies 'TElem : new()'. Nullable does not make use of new() so it is safe." +
                     "The warning is only issued when IsDynamicCodeSupported is true.")]
                 static Func<object, T?> CreateWhenDynamicCodeSupported()

diff --git a/...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs b/...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverterFactory.cs
@@ -46,7 +46,7 @@ public static JsonConverter<T> Create<T>(EnumConverterOptions converterOptions,
         }
 
         [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
-        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2071:UnrecognizedReflectionPattern",
             Justification = "'EnumConverter<T> where T : struct' implies 'T : new()', so the trimmer is warning calling MakeGenericType here because enumType's constructors are not annotated. " +
             "But EnumConverter doesn't call new T(), so this is safe.")]
         public static JsonConverter Create(Type enumType, EnumConverterOptions converterOptions, JsonNamingPolicy? namingPolicy, JsonSerializerOptions options)

diff --git a/...Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs b/...Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs
@@ -43,7 +43,7 @@ public static JsonConverter CreateValueConverter(Type valueTypeToConvert, JsonCo
                 culture: null)!;
         }
 
-        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2071:UnrecognizedReflectionPattern",
             Justification = "'NullableConverter<T> where T : struct' implies 'T : new()', so the trimmer is warning calling MakeGenericType here because valueTypeToConvert's constructors are not annotated. " +
             "But NullableConverter doesn't call new T(), so this is safe.")]
         [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
@@ -109,6 +109,9 @@ internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, boo
 		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj)
 			=> GetMethodReturnValue (method, isNewObj, GetMethodReturnValueAnnotation (method.Method));
 
+		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+			=> new GenericParameterValue (genericParameter.TypeParameterSymbol, dynamicallyAccessedMemberTypes);
+
 		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter)
 			=> new GenericParameterValue (genericParameter.TypeParameterSymbol);
 
@@ -119,14 +122,6 @@ internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy m
 			return GetMethodParameterValue (new ParameterProxy (method, (ParameterIndex) 0), dynamicallyAccessedMemberTypes);
 		}
 
-		// overrideIsThis is needed for backwards compatibility with MakeGenericType/Method https://github.com/dotnet/linker/issues/2428
-		internal MethodParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
-		{
-			if (!method.HasImplicitThis () && !overrideIsThis)
-				throw new InvalidOperationException ($"Cannot get 'this' parameter of method {method.GetDisplayName ()} with no 'this' parameter.");
-			return new MethodParameterValue (new ParameterProxy (method, (ParameterIndex) 0), dynamicallyAccessedMemberTypes, overrideIsThis);
-		}
-
 		internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy method)
 		{
 			if (!method.HasImplicitThis ())

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/GenericParameterValue.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/GenericParameterValue.cs
@@ -15,9 +15,18 @@ namespace ILLink.Shared.TrimAnalysis
 	/// </summary>
 	internal partial record GenericParameterValue
 	{
-		public GenericParameterValue (ITypeParameterSymbol typeParameterSymbol) => GenericParameter = new (typeParameterSymbol);
+		public GenericParameterValue (ITypeParameterSymbol typeParameterSymbol, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+		{
+			GenericParameter = new (typeParameterSymbol);
+			DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
+		}
 
-		public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes => GenericParameter.TypeParameterSymbol.GetDynamicallyAccessedMemberTypes ();
+		public GenericParameterValue (ITypeParameterSymbol typeParameterSymbol)
+			: this (typeParameterSymbol, typeParameterSymbol.GetDynamicallyAccessedMemberTypes ())
+		{
+		}
+
+		public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }
 
 		public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch ()
 			=> new string[] { GenericParameter.TypeParameterSymbol.Name, GenericParameter.TypeParameterSymbol.ContainingSymbol.GetDisplayName () };

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodParameterValue.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodParameterValue.cs
@@ -17,12 +17,11 @@ public MethodParameterValue (IMethodSymbol methodSymbol, ParameterIndex paramete
 		public MethodParameterValue (ParameterProxy parameter)
 			: this (parameter, FlowAnnotations.GetMethodParameterAnnotation (parameter)) { }
 
-		public MethodParameterValue (ParameterProxy parameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
+		public MethodParameterValue (ParameterProxy parameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 		{
 			Parameter = parameter;
 			DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
 			StaticType = parameter.ParameterType;
-			_overrideIsThis = overrideIsThis;
 		}
 
 		public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }

diff --git a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/FlowAnnotations.cs b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/FlowAnnotations.cs
@@ -25,6 +25,8 @@ partial class FlowAnnotations
 
 		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter);
 
+		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
+
 		internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
 
 		internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy method);

diff --git a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
@@ -909,7 +909,7 @@ ValueWithDynamicallyAccessedMembers valueWithDynamicallyAccessedMembers
 						} else {
 							// Any other type - perform generic parameter validation
 							var genericParameterValues = GetGenericParameterValues (typeValue.RepresentedType.GetGenericParameters ());
-							if (!AnalyzeGenericInstantiationTypeArray (argumentValues[0], calledMethod, genericParameterValues)) {
+							if (!AnalyzeGenericInstantiationTypeArray (argumentValues[0], genericParameterValues)) {
 								_diagnosticContext.AddDiagnostic (DiagnosticId.MakeGenericType, calledMethod.GetDisplayName ());
 							}
 						}
@@ -1242,7 +1242,7 @@ private IEnumerable<MultiValue> ProcessGetMethodByName (TypeProxy type, string m
 				yield return NullValue.Instance;
 		}
 
-		private bool AnalyzeGenericInstantiationTypeArray (in MultiValue arrayParam, in MethodProxy calledMethod, ImmutableArray<GenericParameterValue> genericParameters)
+		private bool AnalyzeGenericInstantiationTypeArray (in MultiValue arrayParam, ImmutableArray<GenericParameterValue> genericParameters)
 		{
 			bool hasRequirements = false;
 			foreach (var genericParameter in genericParameters) {
@@ -1280,10 +1280,7 @@ private bool AnalyzeGenericInstantiationTypeArray (in MultiValue arrayParam, in
 
 				for (int i = 0; i < size.Value; i++) {
 					if (array.TryGetValueByIndex (i, out MultiValue value)) {
-						// https://github.com/dotnet/linker/issues/2428
-						// We need to report the target as "this" - as that was the previous behavior
-						// but with the annotation from the generic parameter.
-						var targetValue = _annotations.GetMethodThisParameterValue (calledMethod, GetGenericParameterEffectiveMemberTypes (genericParameters[i]), overrideIsThis: true);
+						var targetValue = _annotations.GetGenericParameterValue (genericParameters[i].GenericParameter, GetGenericParameterEffectiveMemberTypes (genericParameters[i]));
 						_requireDynamicallyAccessedMembersAction.Invoke (value, targetValue);
 					}
 				}
@@ -1315,7 +1312,7 @@ private void ValidateGenericMethodInstantiation (
 			}
 
 			var genericParameterValues = GetGenericParameterValues (genericMethod.GetGenericParameters ());
-			if (!AnalyzeGenericInstantiationTypeArray (genericParametersArray, reflectionMethod, genericParameterValues)) {
+			if (!AnalyzeGenericInstantiationTypeArray (genericParametersArray, genericParameterValues)) {
 				_diagnosticContext.AddDiagnostic (DiagnosticId.MakeGenericMethod, reflectionMethod.GetDisplayName ());
 			}
 		}

diff --git a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/MethodParameterValue.cs b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/MethodParameterValue.cs
@@ -13,9 +13,6 @@ namespace ILLink.Shared.TrimAnalysis
 {
 	internal sealed partial record MethodParameterValue : ValueWithDynamicallyAccessedMembers, IValueWithStaticType
 	{
-		// _overrideIsThis is needed for backwards compatibility with MakeGenericType/Method https://github.com/dotnet/linker/issues/2428
-		private readonly bool _overrideIsThis;
-
 		public TypeProxy? StaticType { get; }
 
 		public ParameterProxy Parameter { get; }
@@ -26,7 +23,7 @@ public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch
 		public override string ToString ()
 			=> this.ValueToString (Parameter.Method.Method, Parameter.Index, DynamicallyAccessedMemberTypes);
 
-		public bool IsThisParameter () => _overrideIsThis || Parameter.IsImplicitThis;
+		public bool IsThisParameter () => Parameter.IsImplicitThis;
 
 		public override SingleValue DeepCopy () => this; // This value is immutable
 

diff --git a/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs b/src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs
@@ -693,6 +693,11 @@ internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, boo
 		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj)
 			=> GetMethodReturnValue (method, isNewObj, GetReturnParameterAnnotation (method.Method));
 
+#pragma warning disable CA1822 // Mark members as static - Should be an instance method for consistency
+		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+			=> new GenericParameterValue (genericParameter.GenericParameter, dynamicallyAccessedMemberTypes);
+#pragma warning restore CA1822 // Mark members as static
+
 		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter)
 			=> new GenericParameterValue (genericParameter.GenericParameter, GetGenericParameterAnnotation (genericParameter.GenericParameter));
 
@@ -703,18 +708,14 @@ internal partial MethodParameterValue GetMethodParameterValue (ParameterProxy pa
 			=> GetMethodParameterValue (param, GetParameterAnnotation (param));
 
 #pragma warning disable CA1822 // Mark members as static - Should be an instance method for consistency
-		// overrideIsThis is needed for backwards compatibility with MakeGenericType/Method https://github.com/dotnet/linker/issues/2428
-		internal MethodParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
+		internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 		{
-			if (!method.HasImplicitThis () && !overrideIsThis)
+			if (!method.HasImplicitThis ())
 				throw new InvalidOperationException ($"Cannot get 'this' parameter of method {method.GetDisplayName ()} with no 'this' parameter.");
-			return new MethodParameterValue (method.Method.DeclaringType, new ParameterProxy (method, (ParameterIndex) 0), dynamicallyAccessedMemberTypes, overrideIsThis);
+			return new MethodParameterValue (method.Method.DeclaringType, new ParameterProxy (method, (ParameterIndex) 0), dynamicallyAccessedMemberTypes);
 		}
 #pragma warning restore CA1822 // Mark members as static
 
-		internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
-			=> GetMethodThisParameterValue (method, dynamicallyAccessedMemberTypes, false);
-
 		internal partial MethodParameterValue GetMethodThisParameterValue (MethodProxy method)
 		{
 			if (!method.HasImplicitThis ())

diff --git a/src/tools/illink/src/linker/Linker.Dataflow/MethodParameterValue.cs b/src/tools/illink/src/linker/Linker.Dataflow/MethodParameterValue.cs
@@ -15,12 +15,11 @@ namespace ILLink.Shared.TrimAnalysis
 	/// </summary>
 	internal partial record MethodParameterValue
 	{
-		public MethodParameterValue (TypeDefinition? staticType, ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
+		public MethodParameterValue (TypeDefinition? staticType, ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 		{
 			StaticType = staticType == null ? null : new (staticType);
 			DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
 			Parameter = param;
-			_overrideIsThis = overrideIsThis;
 		}
 
 		public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }

diff --git a/...ols/illink/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs b/...ols/illink/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs
@@ -739,8 +739,7 @@ static void DynamicallyAccessedMembers ()
 				typeof (AnnotatedGenerics).RequiresPublicMethods ();
 			}
 
-			// This should produce IL2071 https://github.com/dotnet/linker/issues/2144
-			[ExpectedWarning ("IL2070", "MakeGenericMethod")]
+			[ExpectedWarning ("IL2071", "'T'")]
 			static void InstantiateGeneric (Type type = null)
 			{
 				// This should warn due to MakeGenericMethod - in this case the generic parameter is unannotated type