Implement correct handling of new constraint in ilc (#82818)

This is done by treating the new constraint as a data flow annotation `PublicParameterlessConstructor` (which are supposed to be identical). The rest falls out from this change since all of the validation and marking will automatically kick in.

This change causes more methods to go through data flow (since that's what will actually perform the constraint validation/marking). I tested this on the ASP.NET API AOT template. Before this change it ran ~620 methods through data flow. With this change that number goes up to ~2100. The problem is tracked by #82603.

I measured compiler perf but didn't see any noticeable changes. Current thinking is that ~2100 is still not that much and most of those methods are pretty small and thus cheap to run data flow on.

Fixes #81720 - note that the repro still fails on AOT with this fix, but the failure is different (`System.InvalidOperationException: Sequence contains no matching element`). I verified that the missing .ctor is present in the app with the fix.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ArrayValue.cs

@@ -84,6 +84,15 @@ public override SingleValue DeepCopy()
             var newValue = new ArrayValue(Size.DeepCopy(), ElementType);
             foreach (var kvp in IndexValues)
             {
+#if DEBUG
+                // Since it's possible to store a reference to array as one of its own elements
+                // simple deep copy could lead to endless recursion.
+                // So instead we simply disallow arrays as element values completely - and treat that case as "too complex to analyze".
+                foreach (SingleValue v in kvp.Value.Value)
+                {
+                    System.Diagnostics.Debug.Assert(v is not ArrayValue);
+                }
+#endif
                 newValue.IndexValues.Add(kvp.Key, new ValueBasicBlockPair(kvp.Value.Value.Clone(), kvp.Value.BasicBlockIndex));
             }
 

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs

@@ -42,7 +42,7 @@ public AttributeDataFlow(Logger logger, NodeFactory factory, FlowAnnotations ann
             DependencyList? result = null;
 
             // First do the dataflow for the constructor parameters if necessary.
-            if (_annotations.RequiresDataflowAnalysis(method))
+            if (_annotations.RequiresDataflowAnalysisDueToSignature(method))
             {
                 var builder = ImmutableArray.CreateBuilder<object?>(arguments.FixedArguments.Length);
                 foreach (var argument in arguments.FixedArguments)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs

@@ -41,13 +41,18 @@ public FlowAnnotations(Logger logger, ILProvider ilProvider, CompilerGeneratedSt
             CompilerGeneratedState = compilerGeneratedState;
         }
 
-        public bool RequiresDataflowAnalysis(MethodDesc method)
+        /// <summary>
+        /// Determines if the method has any annotations on its parameters or return values.
+        /// </summary>
+        public bool RequiresDataflowAnalysisDueToSignature(MethodDesc method)
         {
             try
             {
                 method = method.GetTypicalMethodDefinition();
                 TypeAnnotations typeAnnotations = GetAnnotations(method.OwningType);
-                return typeAnnotations.HasGenericParameterAnnotation() || typeAnnotations.TryGetAnnotation(method, out _);
+                // This will return true even if the method has annotations on generic parameters,
+                // but the callers don't rely on that - it's just easier to leave it this way (and it makes very little difference)
+                return typeAnnotations.TryGetAnnotation(method, out _);
             }
             catch (TypeSystemException)
             {
@@ -68,37 +73,16 @@ public bool RequiresVirtualMethodDataflowAnalysis(MethodDesc method)
             }
         }
 
-        public bool RequiresDataflowAnalysis(FieldDesc field)
+        /// <summary>
+        /// Determines if the field has any annotations on itself (not looking at owning type).
+        /// </summary>
+        public bool RequiresDataflowAnalysisDueToSignature(FieldDesc field)
         {
             try
             {
                 field = field.GetTypicalFieldDefinition();
                 TypeAnnotations typeAnnotations = GetAnnotations(field.OwningType);
-                return typeAnnotations.HasGenericParameterAnnotation() || typeAnnotations.TryGetAnnotation(field, out _);
-            }
-            catch (TypeSystemException)
-            {
-                return false;
-            }
-        }
-
-        public bool RequiresGenericArgumentDataFlowAnalysis(GenericParameterDesc genericParameter)
-        {
-            try
-            {
-                return GetGenericParameterAnnotation(genericParameter) != DynamicallyAccessedMemberTypes.None;
-            }
-            catch (TypeSystemException)
-            {
-                return false;
-            }
-        }
-
-        public bool HasAnyAnnotations(TypeDesc type)
-        {
-            try
-            {
-                return !GetAnnotations(type.GetTypeDefinition()).IsDefault;
+                return typeAnnotations.TryGetAnnotation(field, out _);
             }
             catch (TypeSystemException)
             {
@@ -330,6 +314,11 @@ private static DynamicallyAccessedMemberTypes GetMemberTypesForDynamicallyAccess
                 return (DynamicallyAccessedMemberTypes)blobReader.ReadUInt32();
             }
 
+            private static DynamicallyAccessedMemberTypes GetMemberTypesForConstraints(GenericParameterDesc genericParameter)
+                => genericParameter.HasDefaultConstructorConstraint ?
+                    DynamicallyAccessedMemberTypes.PublicParameterlessConstructor :
+                    DynamicallyAccessedMemberTypes.None;
+
             protected override bool CompareKeyToValue(TypeDesc key, TypeAnnotations value) => key == value.Type;
             protected override bool CompareValueToValue(TypeAnnotations value1, TypeAnnotations value2) => value1.Type == value2.Type;
             protected override int GetKeyHashCode(TypeDesc key) => key.GetHashCode();
@@ -478,6 +467,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
                     {
                         GenericParameter genericParameterDef = reader.GetGenericParameter(genericParameter.Handle);
                         var annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute(reader, genericParameterDef.GetCustomAttributes());
+                        annotation |= GetMemberTypesForConstraints(genericParameter);
                         if (annotation != DynamicallyAccessedMemberTypes.None)
                         {
                             genericParameterAnnotations ??= new DynamicallyAccessedMemberTypes[method.Instantiation.Length];
@@ -639,6 +629,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
                         genericParameter = (attrs?[genericParameterIndex] as EcmaGenericParameter) ?? genericParameter;
                         GenericParameter genericParameterDef = reader.GetGenericParameter(genericParameter.Handle);
                         var annotation = GetMemberTypesForDynamicallyAccessedMembersAttribute(reader, genericParameterDef.GetCustomAttributes());
+                        annotation |= GetMemberTypesForConstraints(genericParameter);
                         if (annotation != DynamicallyAccessedMemberTypes.None)
                         {
                             typeGenericParameterAnnotations ??= new DynamicallyAccessedMemberTypes[ecmaType.Instantiation.Length];
@@ -959,7 +950,7 @@ public FieldAnnotation(FieldDesc field, DynamicallyAccessedMemberTypes annotatio
         }
 
         internal partial bool MethodRequiresDataFlowAnalysis(MethodProxy method)
-            => RequiresDataflowAnalysis(method.Method);
+            => RequiresDataflowAnalysisDueToSignature(method.Method);
 
 #pragma warning disable CA1822 // Other partial implementations are not in the ilc project
         internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

@@ -5,7 +5,6 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
-using System.Reflection.Metadata;
 
 using ILCompiler.Logging;
 
@@ -1378,7 +1377,7 @@ private void ScanStelem(
                     else
                     {
                         // When we know the index, we can record the value at that index.
-                        StoreMethodLocalValue(arrValue.IndexValues, valueToStore.Value, indexToStoreAtInt.Value, curBasicBlock, MaxTrackedArrayValues);
+                        StoreMethodLocalValue(arrValue.IndexValues, ArrayValue.SanitizeArrayElementValue(valueToStore.Value), indexToStoreAtInt.Value, curBasicBlock, MaxTrackedArrayValues);
                     }
                 }
             }

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

@@ -33,7 +33,7 @@ internal sealed class ReflectionMethodBodyScanner : MethodBodyScanner
         public static bool RequiresReflectionMethodBodyScannerForCallSite(FlowAnnotations flowAnnotations, MethodDesc method)
         {
             return Intrinsics.GetIntrinsicIdForMethod(method) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel ||
-                flowAnnotations.RequiresDataflowAnalysis(method) ||
+                flowAnnotations.RequiresDataflowAnalysisDueToSignature(method) ||
                 GenericArgumentDataFlow.RequiresGenericArgumentDataFlow(flowAnnotations, method) ||
                 method.DoesMethodRequire(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _) ||
                 method.DoesMethodRequire(DiagnosticUtilities.RequiresDynamicCodeAttribute, out _) ||
@@ -43,12 +43,12 @@ public static bool RequiresReflectionMethodBodyScannerForCallSite(FlowAnnotation
         public static bool RequiresReflectionMethodBodyScannerForMethodBody(FlowAnnotations flowAnnotations, MethodDesc methodDefinition)
         {
             return Intrinsics.GetIntrinsicIdForMethod(methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel ||
-                flowAnnotations.RequiresDataflowAnalysis(methodDefinition);
+                flowAnnotations.RequiresDataflowAnalysisDueToSignature(methodDefinition);
         }
 
         public static bool RequiresReflectionMethodBodyScannerForAccess(FlowAnnotations flowAnnotations, FieldDesc field)
         {
-            return flowAnnotations.RequiresDataflowAnalysis(field) ||
+            return flowAnnotations.RequiresDataflowAnalysisDueToSignature(field) ||
                 GenericArgumentDataFlow.RequiresGenericArgumentDataFlow(flowAnnotations, field) ||
                 field.DoesFieldRequire(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _) ||
                 field.DoesFieldRequire(DiagnosticUtilities.RequiresDynamicCodeAttribute, out _);
@@ -259,9 +259,10 @@ public static bool HandleCall(
             var callingMethodDefinition = callingMethodBody.OwningMethod;
             Debug.Assert(callingMethodDefinition == diagnosticContext.Origin.MemberDefinition);
 
-            bool requiresDataFlowAnalysis = reflectionMarker.Annotations.RequiresDataflowAnalysis(calledMethod);
             var annotatedMethodReturnValue = reflectionMarker.Annotations.GetMethodReturnValue(calledMethod);
-            Debug.Assert(requiresDataFlowAnalysis || annotatedMethodReturnValue.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.None);
+            Debug.Assert(
+                RequiresReflectionMethodBodyScannerForCallSite(reflectionMarker.Annotations, calledMethod) ||
+                annotatedMethodReturnValue.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.None);
 
             MultiValue? maybeMethodReturnValue = null;
 

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs

@@ -896,6 +896,9 @@ public MetadataManager ToAnalysisBasedMetadataManager()
 
         private void AddDataflowDependency(ref DependencyList dependencies, NodeFactory factory, MethodIL methodIL, string reason)
         {
+            if (ShouldSkipDataflowForMethod(methodIL))
+                return;
+
             MethodIL methodILDefinition = methodIL.GetMethodILDefinition();
             if (FlowAnnotations.CompilerGeneratedState.TryGetUserMethodForCompilerGeneratedMember(methodILDefinition.OwningMethod, out var userMethod))
             {
@@ -904,15 +907,35 @@ private void AddDataflowDependency(ref DependencyList dependencies, NodeFactory
                 // It is possible that this will try to add the DatadlowAnalyzedMethod node multiple times for the same method
                 // but that's OK since the node factory will only add actually one node.
                 methodILDefinition = FlowAnnotations.ILProvider.GetMethodIL(userMethod);
+                if (ShouldSkipDataflowForMethod(methodILDefinition))
+                    return;
             }
 
             // Data-flow (reflection scanning) for compiler-generated methods will happen as part of the
             // data-flow scan of the user-defined method which uses this compiler-generated method.
             if (CompilerGeneratedState.IsNestedFunctionOrStateMachineMember(methodILDefinition.OwningMethod))
                 return;
 
+            // Currently we add data flow for reasons which are not directly related to data flow
+            // or don't need full data flow processing.
+            // The prime example is that we run data flow for any method which contains an access
+            // to a member on a generic type with annotated generic parameters. These are relatively common
+            // as there are quite a few types which use the new/struct/unmanaged constraints (which are all treated as annotations).
+            // Technically we don't need to run data flow only because of this, since the result of analysis
+            // will not depend on stack modeling and of the other data flow functionality.
+            // See https://github.com/dotnet/runtime/issues/82603 for more details and some ideas.
+
             dependencies ??= new DependencyList();
             dependencies.Add(factory.DataflowAnalyzedMethod(methodILDefinition), reason);
+
+            // Some MethodIL implementations can't/don't provide the method definition version of the IL
+            // for example if the method is a stub generated by the compiler, the IL may look very different
+            // between each instantiation (and definition version may not even make sense).
+            // We will skip data flow for such methods for now, since currently dataflow can't handle instantiated
+            // generics.
+            static bool ShouldSkipDataflowForMethod(MethodIL method)
+                => method.GetMethodILDefinition() == method &&
+                method.OwningMethod.GetTypicalMethodDefinition() != method.OwningMethod;
         }
 
         private struct ReflectableEntityBuilder<T>

src/coreclr/tools/aot/ILLink.Shared/TrimAnalysis/ArrayValue.cs

@@ -1,6 +1,7 @@
 // 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.Linq;
 using ILLink.Shared.DataFlow;
 using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;
 
@@ -16,5 +17,32 @@ internal sealed partial record ArrayValue : SingleValue
 		public readonly SingleValue Size;
 
 		public partial bool TryGetValueByIndex (int index, out MultiValue value);
+
+		public static MultiValue SanitizeArrayElementValue (MultiValue input)
+		{
+			// We need to be careful about self-referencing arrays. It's easy to have an array which has one of the elements as itself:
+			// var arr = new object[1];
+			// arr[0] = arr;
+			//
+			// We can't deep copy this, as it's an infinite recursion. And we can't easily guard against it with checks to self-referencing
+			// arrays - as this could be two or more levels deep.
+			//
+			// We need to deep copy arrays because we don't have a good way to track references (we treat everything as a value type)
+			// and thus we could get bad results if the array is involved in multiple branches for example.
+			// That said, it only matters for us to track arrays to be able to get integers or Type values (and we really only need relatively simple cases)
+			//
+			// So we will simply treat array value as an element value as "too complex to analyze" and give up by storing Unknown instead
+
+			bool needSanitization = false;
+			foreach (var v in input) {
+				if (v is ArrayValue)
+					needSanitization = true;
+			}
+
+			if (!needSanitization)
+				return input;
+
+			return new (input.Select (v => v is ArrayValue ? UnknownValue.Instance : v));
+		}
 	}
 }

src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestDatabase.cs

@@ -24,6 +24,11 @@ public static IEnumerable<object[]> DynamicDependencies ()
 			return TestNamesBySuiteName ();
 		}
 
+		public static IEnumerable<object[]> Generics ()
+		{
+			return TestNamesBySuiteName ();
+		}
+
 		public static IEnumerable<object[]> LinkXml()
 		{
 			return TestNamesBySuiteName();

src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs

@@ -23,6 +23,13 @@ public void DynamicDependencies (string t)
 			Run (t);
 		}
 
+		[Theory]
+		[MemberData (nameof (TestDatabase.Generics), MemberType = typeof (TestDatabase))]
+		public void Generics (string t)
+		{
+			Run (t);
+		}
+
 		[Theory]
 		[MemberData (nameof (TestDatabase.LinkXml), MemberType = typeof (TestDatabase))]
 		public void LinkXml (string t)

src/tests/nativeaot/SmokeTests/TrimmingBehaviors/DependencyInjectionPattern.cs

@@ -24,6 +24,13 @@ public static int Run()
         var created = customFactory.Create();
         Assert.Equal(42, created.GetValue());
 
+        services.RegisterService(typeof(ICustomFactoryWithConstraint<>), typeof(CustomFactoryWithConstraintImpl<>));
+        services.RegisterService(typeof(ICustomFactoryWithConstraintWrapper), typeof(CustomFactoryWithConstraintWrapperImpl));
+
+        var customFactoryWithConstraintWrapper = services.GetService<ICustomFactoryWithConstraintWrapper>();
+        var createdWithConstraint = customFactoryWithConstraintWrapper.Create();
+        Assert.Equal(42, createdWithConstraint.GetValue());
+
         return 100;
     }
 }
@@ -173,5 +180,47 @@ public FactoryCreated()
         _value = 42;
     }
 
+    public int GetValue() => _value;
+}
+
+interface ICustomFactoryWithConstraintWrapper
+{
+    FactoryWithConstraintCreated Create();
+}
+
+class CustomFactoryWithConstraintWrapperImpl : ICustomFactoryWithConstraintWrapper
+{
+    private FactoryWithConstraintCreated _value;
+
+    public CustomFactoryWithConstraintWrapperImpl(ICustomFactoryWithConstraint<FactoryWithConstraintCreated> factory)
+    {
+        _value = factory.Create();
+    }
+
+    public FactoryWithConstraintCreated Create() => _value;
+}
+
+interface ICustomFactoryWithConstraint<T> where T : new()
+{
+    T Create();
+}
+
+class CustomFactoryWithConstraintImpl<T> : ICustomFactoryWithConstraint<T> where T : new()
+{
+    public T Create()
+    {
+        return Activator.CreateInstance<T>();
+    }
+}
+
+class FactoryWithConstraintCreated
+{
+    int _value;
+
+    public FactoryWithConstraintCreated()
+    {
+        _value = 42;
+    }
+
     public int GetValue() => _value;
 }

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ArrayValue.cs

@@ -64,6 +64,15 @@ public override SingleValue DeepCopy ()
 		{
 			var newArray = new ArrayValue (Size);
 			foreach (var kvp in IndexValues) {
+#if DEBUG
+				// Since it's possible to store a reference to array as one of its own elements
+				// simple deep copy could lead to endless recursion.
+				// So instead we simply disallow arrays as element values completely - and treat that case as "too complex to analyze".
+				foreach (SingleValue v in kvp.Value) {
+					System.Diagnostics.Debug.Assert (v is not ArrayValue);
+				}
+#endif
+
 				newArray.IndexValues.Add (kvp.Key, kvp.Value.Clone ());
 			}
 

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs

@@ -79,7 +79,7 @@ public override MultiValue VisitArrayCreation (IArrayCreationOperation operation
 			var elements = operation.Initializer?.ElementValues.Select (val => Visit (val, state)).ToArray () ?? System.Array.Empty<MultiValue> ();
 			foreach (var array in arrayValue.Cast<ArrayValue> ()) {
 				for (int i = 0; i < elements.Length; i++) {
-					array.IndexValues.Add (i, elements[i]);
+					array.IndexValues.Add (i, ArrayValue.SanitizeArrayElementValue(elements[i]));
 				}
 			}
 
@@ -229,9 +229,9 @@ public override void HandleArrayElementWrite (MultiValue arrayValue, MultiValue
 						arr.IndexValues.Clear ();
 					} else {
 						if (arr.IndexValues.TryGetValue (index.Value, out _)) {
-							arr.IndexValues[index.Value] = valueToWrite;
+							arr.IndexValues[index.Value] = ArrayValue.SanitizeArrayElementValue(valueToWrite);
 						} else if (arr.IndexValues.Count < MaxTrackedArrayValues) {
-							arr.IndexValues[index.Value] = valueToWrite;
+							arr.IndexValues[index.Value] = ArrayValue.SanitizeArrayElementValue(valueToWrite);
 						}
 					}
 				}