Preserve static type info for return value of ctor (dotnet#101212)

Instead of tracking the return value as "TopValue" or "unknown", this models the constructor as returning a value with a static type when called with newobj, letting us undo the workaround from dotnet#101031.
michaelgsharp · May 8, 2024 · 99bba64 · 99bba64
1 parent d9e7f2a
commit 99bba64
Show file tree

Hide file tree

Showing 23 changed files with 129 additions and 55 deletions.
diff --git a/src/coreclr/tools/Common/Compiler/Dataflow/MethodProxy.cs b/src/coreclr/tools/Common/Compiler/Dataflow/MethodProxy.cs
@@ -66,6 +66,8 @@ internal partial ImmutableArray<GenericParameterProxy> GetGenericParameters()
             return builder.ToImmutableArray();
         }
 
+        internal partial bool IsConstructor() => Method.IsConstructor;
+
         internal partial bool IsStatic() => Method.Signature.IsStatic;
 
         internal partial bool HasImplicitThis() => !Method.Signature.IsStatic;

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
@@ -951,12 +951,12 @@ internal partial bool MethodRequiresDataFlowAnalysis(MethodProxy 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)
+        internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 #pragma warning restore CA1822 // Mark members as static
-            => new MethodReturnValue(method.Method, dynamicallyAccessedMemberTypes);
+            => new MethodReturnValue(method.Method, isNewObj, dynamicallyAccessedMemberTypes);
 
-        internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method)
-            => GetMethodReturnValue(method, GetReturnParameterAnnotation(method.Method));
+        internal partial MethodReturnValue GetMethodReturnValue(MethodProxy method, bool isNewObj)
+            => GetMethodReturnValue(method, isNewObj, GetReturnParameterAnnotation(method.Method));
 
         internal partial GenericParameterValue GetGenericParameterValue(GenericParameterProxy genericParameter)
             => new GenericParameterValue(genericParameter.GenericParameter, GetGenericParameterAnnotation(genericParameter.GenericParameter));

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
@@ -39,6 +39,7 @@ public HandleCallAction(
         {
             _reflectionMarker = reflectionMarker;
             _operation = operation;
+            _isNewObj = operation == ILOpcode.newobj;
             _diagnosticContext = diagnosticContext;
             _callingMethod = callingMethod;
             _annotations = annotations;
@@ -337,12 +338,6 @@ private partial bool TryHandleIntrinsic (
                         if (Intrinsics.GetIntrinsicIdForMethod(_callingMethod) == IntrinsicId.RuntimeReflectionExtensions_GetMethodInfo)
                             break;
 
-                        if (param.IsEmpty())
-                        {
-                            // The static value is unknown and the below `foreach` won't execute
-                            _reflectionMarker.Dependencies.Add(_reflectionMarker.Factory.ReflectedDelegate(null), "Delegate.Method access on unknown delegate type");
-                        }
-
                         foreach (var valueNode in param.AsEnumerable())
                         {
                             TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
@@ -390,7 +385,7 @@ private partial bool TryHandleIntrinsic (
                             if (staticType is null || (!staticType.IsDefType && !staticType.IsArray))
                             {
                                 // We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
-                                AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod));
+                                AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj));
                             }
                             else if (staticType.IsSealed() || staticType.IsTypeOf("System", "Delegate"))
                             {
@@ -425,7 +420,7 @@ private partial bool TryHandleIntrinsic (
                                 // Return a value which is "unknown type" with annotation. For now we'll use the return value node
                                 // for the method, which means we're loosing the information about which staticType this
                                 // started with. For now we don't need it, but we can add it later on.
-                                AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, annotation));
+                                AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj, annotation));
                             }
                         }
                     }

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodReturnValue.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodReturnValue.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using ILCompiler.Dataflow;
 using ILLink.Shared.DataFlow;
@@ -16,9 +17,10 @@ namespace ILLink.Shared.TrimAnalysis
     /// </summary>
     internal partial record MethodReturnValue
     {
-        public MethodReturnValue(MethodDesc method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+        public MethodReturnValue(MethodDesc method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
         {
-            StaticType = method.Signature.ReturnType;
+            Debug.Assert(!isNewObj || method.IsConstructor, "isNewObj can only be true for constructors");
+            StaticType = isNewObj ? method.OwningType : method.Signature.ReturnType;
             Method = method;
             DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
         }

diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
@@ -118,7 +118,7 @@ protected override void Scan(MethodIL methodBody, ref InterproceduralState inter
             if (!methodBody.OwningMethod.Signature.ReturnType.IsVoid)
             {
                 var method = methodBody.OwningMethod;
-                var methodReturnValue = _annotations.GetMethodReturnValue(method);
+                var methodReturnValue = _annotations.GetMethodReturnValue(method, isNewObj: false);
                 if (methodReturnValue.DynamicallyAccessedMemberTypes != 0)
                     HandleAssignmentPattern(_origin, ReturnValue, methodReturnValue, method.GetDisplayName());
             }
@@ -315,7 +315,8 @@ public static MultiValue HandleCall(
             var callingMethodDefinition = callingMethodBody.OwningMethod;
             Debug.Assert(callingMethodDefinition == diagnosticContext.Origin.MemberDefinition);
 
-            var annotatedMethodReturnValue = reflectionMarker.Annotations.GetMethodReturnValue(calledMethod);
+            bool isNewObj = operation == ILOpcode.newobj;
+            var annotatedMethodReturnValue = reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, isNewObj);
             Debug.Assert(
                 RequiresReflectionMethodBodyScannerForCallSite(reflectionMarker.Annotations, calledMethod) ||
                 annotatedMethodReturnValue.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.None);

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs
@@ -103,11 +103,11 @@ public static DynamicallyAccessedMemberTypes GetTypeAnnotation(ITypeSymbol type)
 		internal partial bool MethodRequiresDataFlowAnalysis (MethodProxy method)
 			=> RequiresDataFlowAnalysis (method.Method);
 
-		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
-			=> new MethodReturnValue (method.Method, dynamicallyAccessedMemberTypes);
+		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+			=> new MethodReturnValue (method.Method, isNewObj, dynamicallyAccessedMemberTypes);
 
-		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method)
-			=> GetMethodReturnValue (method, GetMethodReturnValueAnnotation (method.Method));
+		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj)
+			=> GetMethodReturnValue (method, isNewObj, GetMethodReturnValueAnnotation (method.Method));
 
 		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter)
 			=> new GenericParameterValue (genericParameter.TypeParameterSymbol);

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
@@ -33,6 +33,7 @@ public HandleCallAction (
 		{
 			_owningSymbol = owningSymbol;
 			_operation = operation;
+			_isNewObj = operation.Kind == OperationKind.ObjectCreation;
 			_diagnosticContext = diagnosticContext;
 			_annotations = FlowAnnotations.Instance;
 			_reflectionAccessAnalyzer = default;
@@ -86,7 +87,7 @@ private partial bool TryHandleIntrinsic (
 
 						if (staticType is null) {
 							// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
-							AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod));
+							AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj));
 						} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.TypeKind == TypeKind.Array) {
 							// We can treat this one the same as if it was a typeof() expression
 
@@ -106,7 +107,7 @@ private partial bool TryHandleIntrinsic (
 							AddReturnValue (new SystemTypeValue (new (staticType)));
 						} else {
 							var annotation = FlowAnnotations.GetTypeAnnotation (staticType);
-							AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, annotation));
+							AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj, annotation));
 						}
 					}
 				break;

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodProxy.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodProxy.cs
@@ -46,6 +46,8 @@ internal partial ImmutableArray<GenericParameterProxy> GetGenericParameters ()
 			return builder.ToImmutableArray ();
 		}
 
+		internal partial bool IsConstructor () => Method.IsConstructor ();
+
 		internal partial bool IsStatic () => Method.IsStatic;
 
 		internal partial bool HasImplicitThis () => Method.HasImplicitThis ();

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodReturnValue.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/MethodReturnValue.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using ILLink.RoslynAnalyzer;
 using ILLink.Shared.DataFlow;
@@ -11,16 +12,17 @@ namespace ILLink.Shared.TrimAnalysis
 {
 	internal partial record MethodReturnValue
 	{
-		public MethodReturnValue (IMethodSymbol methodSymbol)
-			: this (methodSymbol, FlowAnnotations.GetMethodReturnValueAnnotation (methodSymbol))
+		public MethodReturnValue (IMethodSymbol methodSymbol, bool isNewObj)
+			: this (methodSymbol, isNewObj, FlowAnnotations.GetMethodReturnValueAnnotation (methodSymbol))
 		{
 		}
 
-		public MethodReturnValue (IMethodSymbol methodSymbol, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
+		public MethodReturnValue (IMethodSymbol methodSymbol, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 		{
+			Debug.Assert (!isNewObj || methodSymbol.MethodKind == MethodKind.Constructor, "isNewObj can only be true for constructors");
 			MethodSymbol = methodSymbol;
 			DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
-			StaticType = new (methodSymbol.ReturnType);
+			StaticType = new (isNewObj ? methodSymbol.ContainingType : methodSymbol.ReturnType);
 		}
 
 		public readonly IMethodSymbol MethodSymbol;

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
@@ -116,7 +116,7 @@ public override MultiValue VisitConversion (IConversionOperation operation, Stat
 			var value = base.VisitConversion (operation, state);
 
 			if (operation.OperatorMethod != null)
-				return operation.OperatorMethod.ReturnType.IsTypeInterestingForDataflow () ? new MethodReturnValue (operation.OperatorMethod) : value;
+				return operation.OperatorMethod.ReturnType.IsTypeInterestingForDataflow () ? new MethodReturnValue (operation.OperatorMethod, isNewObj: false) : value;
 
 			// TODO - is it possible to have annotation on the operator method parameters?
 			// if so, will these be checked here?
@@ -341,7 +341,7 @@ public override void HandleReturnValue (MultiValue returnValue, IOperation opera
 				return;
 
 			if (method.ReturnType.IsTypeInterestingForDataflow ()) {
-				var returnParameter = new MethodReturnValue (method);
+				var returnParameter = new MethodReturnValue (method, isNewObj: false);
 
 				TrimAnalysisPatterns.Add (
 					new TrimAnalysisAssignmentPattern (returnValue, returnParameter, operation, OwningSymbol, featureContext),

diff --git a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/FlowAnnotations.cs b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/FlowAnnotations.cs
@@ -19,9 +19,9 @@ partial class FlowAnnotations
 	{
 		internal partial bool MethodRequiresDataFlowAnalysis (MethodProxy method);
 
-		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
+		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes);
 
-		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method);
+		internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj);
 
 		internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter);
 

diff --git a/src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs b/src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs
@@ -26,6 +26,7 @@ internal partial struct HandleCallAction
 		private readonly DiagnosticContext _diagnosticContext;
 		private readonly FlowAnnotations _annotations;
 		private readonly RequireDynamicallyAccessedMembersAction _requireDynamicallyAccessedMembersAction;
+		private readonly bool _isNewObj;
 
 		public bool Invoke (MethodProxy calledMethod, MultiValue instanceValue, IReadOnlyList<MultiValue> argumentValues, IntrinsicId intrinsicId, out MultiValue methodReturnValue)
 		{
@@ -37,11 +38,12 @@ public bool Invoke (MethodProxy calledMethod, MultiValue instanceValue, IReadOnl
 
 			// As a convenience, if the code above didn't set the return value (and the method has a return value),
 			// we will set it to be an unknown value with the return type of the method.
-			var annotatedMethodReturnValue = _annotations.GetMethodReturnValue (calledMethod);
+			var annotatedMethodReturnValue = _annotations.GetMethodReturnValue (calledMethod, _isNewObj);
 			bool returnsVoid = calledMethod.ReturnsVoid ();
-			methodReturnValue = maybeMethodReturnValue ?? (returnsVoid ?
-				MultiValueLattice.Top :
-				annotatedMethodReturnValue);
+			methodReturnValue = maybeMethodReturnValue ??
+				((returnsVoid && !_isNewObj)
+					? MultiValueLattice.Top
+					: annotatedMethodReturnValue);
 
 			// Validate that the return value has the correct annotations as per the method return value annotations
 			if (annotatedMethodReturnValue.DynamicallyAccessedMemberTypes != 0) {
@@ -80,7 +82,7 @@ bool TryHandleSharedIntrinsic (
 			MultiValue? returnValue = methodReturnValue = null;
 
 			bool requiresDataFlowAnalysis = _annotations.MethodRequiresDataFlowAnalysis (calledMethod);
-			var annotatedMethodReturnValue = _annotations.GetMethodReturnValue (calledMethod);
+			var annotatedMethodReturnValue = _annotations.GetMethodReturnValue (calledMethod, _isNewObj);
 			Debug.Assert (requiresDataFlowAnalysis || annotatedMethodReturnValue.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.None);
 
 			switch (intrinsicId) {
@@ -237,7 +239,7 @@ GenericParameterValue genericParam
 									&& valueWithDynamicallyAccessedMembers.DynamicallyAccessedMemberTypes == DynamicallyAccessedMemberTypes.All)
 									returnMemberTypes = DynamicallyAccessedMemberTypes.All;
 
-								AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, returnMemberTypes));
+								AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, _isNewObj, returnMemberTypes));
 							}
 						}
 					}
@@ -544,7 +546,7 @@ GenericParameterValue genericParam
 
 									// We only applied the annotation based on binding flags, so we will keep the necessary types
 									// but we will not keep anything on them. So the return value has no known annotations on it
-									AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, DynamicallyAccessedMemberTypes.None));
+									AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, _isNewObj, DynamicallyAccessedMemberTypes.None));
 								}
 							}
 						} else if (value is NullValue) {
@@ -558,9 +560,9 @@ GenericParameterValue genericParam
 							// since All applies recursively to all nested type (see MarkStep.MarkEntireType).
 							// Otherwise we only mark the nested type itself, nothing on it, so the return value has no annotation on it.
 							if (value is ValueWithDynamicallyAccessedMembers { DynamicallyAccessedMemberTypes: DynamicallyAccessedMemberTypes.All })
-								AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, DynamicallyAccessedMemberTypes.All));
+								AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, _isNewObj, DynamicallyAccessedMemberTypes.All));
 							else
-								AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, DynamicallyAccessedMemberTypes.None));
+								AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, _isNewObj, DynamicallyAccessedMemberTypes.None));
 						}
 					}
 				}
@@ -832,7 +834,7 @@ GenericParameterValue genericParam
 						} else if (typeNameValue is ValueWithDynamicallyAccessedMembers valueWithDynamicallyAccessedMembers && valueWithDynamicallyAccessedMembers.DynamicallyAccessedMemberTypes != 0) {
 							// Propagate the annotation from the type name to the return value. Annotation on a string value will be fulfilled whenever a value is assigned to the string with annotation.
 							// So while we don't know which type it is, we can guarantee that it will fulfill the annotation.
-							AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, valueWithDynamicallyAccessedMembers.DynamicallyAccessedMemberTypes));
+							AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, _isNewObj, valueWithDynamicallyAccessedMembers.DynamicallyAccessedMemberTypes));
 						} else {
 							_diagnosticContext.AddDiagnostic (DiagnosticId.UnrecognizedTypeNameInTypeGetType, calledMethod.GetDisplayName ());
 							AddReturnValue (MultiValueLattice.Top);
@@ -956,7 +958,7 @@ GenericParameterValue genericParam
 									propagatedMemberTypes |= DynamicallyAccessedMemberTypes.Interfaces;
 							}
 
-							AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, propagatedMemberTypes));
+							AddReturnValue (_annotations.GetMethodReturnValue (calledMethod, _isNewObj, propagatedMemberTypes));
 						} else if (value is SystemTypeValue systemTypeValue) {
 							if (TryGetBaseType (systemTypeValue.RepresentedType, out var baseType))
 								AddReturnValue (new SystemTypeValue (baseType.Value));

diff --git a/src/tools/illink/src/ILLink.Shared/TypeSystemProxy/MethodProxy.cs b/src/tools/illink/src/ILLink.Shared/TypeSystemProxy/MethodProxy.cs
@@ -56,6 +56,7 @@ internal bool HasParameterOfType (ParameterIndex parameterIndex, string fullType
 		internal partial bool HasGenericParameters ();
 		internal partial bool HasGenericParametersCount (int genericParameterCount);
 		internal partial ImmutableArray<GenericParameterProxy> GetGenericParameters ();
+		internal partial bool IsConstructor ();
 		internal partial bool IsStatic ();
 		internal partial bool HasImplicitThis ();
 		internal partial bool ReturnsVoid ();