Trim analyzer: Implement intrinsics handling of object.GetType

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

@@ -4,7 +4,6 @@
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using ILCompiler;
-using ILCompiler.Dataflow;
 using ILLink.Shared.DataFlow;
 using Internal.TypeSystem;
 
@@ -16,7 +15,7 @@ namespace ILLink.Shared.TrimAnalysis
     /// <summary>
     /// A representation of a field. Typically a result of ldfld.
     /// </summary>
-    internal sealed partial record FieldValue : IValueWithStaticType
+    internal sealed partial record FieldValue
     {
         public FieldValue(FieldDesc field, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
         {
@@ -32,8 +31,6 @@ public FieldValue(FieldDesc field, DynamicallyAccessedMemberTypes dynamicallyAcc
         public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch()
             => new string[] { Field.GetDisplayName() };
 
-        public TypeDesc? StaticType { get; }
-
         public override SingleValue DeepCopy() => this; // This value is immutable
 
         public override string ToString() => this.ValueToString(Field, DynamicallyAccessedMemberTypes);

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

@@ -1041,7 +1041,7 @@ protected void StoreInReference(MultiValue target, MultiValue source, MethodIL m
                         HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState));
                         break;
                     case IValueWithStaticType valueWithStaticType:
-                        if (valueWithStaticType.StaticType is not null && FlowAnnotations.IsTypeInterestingForDataflow(valueWithStaticType.StaticType))
+                        if (valueWithStaticType.StaticType is not null && FlowAnnotations.IsTypeInterestingForDataflow(valueWithStaticType.StaticType.Value.Type))
                             throw new InvalidOperationException(MessageContainer.CreateErrorMessage(
                                 $"Unhandled StoreReference call. Unhandled attempt to store a value in {value} of type {value.GetType()}.",
                                 (int)DiagnosticId.LinkerUnexpectedError,

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

@@ -2,9 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics.CodeAnalysis;
-using ILCompiler.Dataflow;
 using ILLink.Shared.TypeSystemProxy;
-using Internal.TypeSystem;
 
 #nullable enable
 
@@ -14,7 +12,7 @@ namespace ILLink.Shared.TrimAnalysis
     /// <summary>
     /// A value that came from a method parameter - such as the result of a ldarg.
     /// </summary>
-    internal partial record MethodParameterValue : IValueWithStaticType
+    internal partial record MethodParameterValue
     {
         public MethodParameterValue(ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, bool overrideIsThis = false)
         {
@@ -25,7 +23,5 @@ public MethodParameterValue(ParameterProxy param, DynamicallyAccessedMemberTypes
         }
 
         public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }
-
-        public TypeDesc? StaticType { get; }
     }
 }

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

@@ -14,7 +14,7 @@ namespace ILLink.Shared.TrimAnalysis
     /// <summary>
     /// Return value from a method
     /// </summary>
-    internal partial record MethodReturnValue : IValueWithStaticType
+    internal partial record MethodReturnValue
     {
         public MethodReturnValue(MethodDesc method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
         {
@@ -30,8 +30,6 @@ public MethodReturnValue(MethodDesc method, DynamicallyAccessedMemberTypes dynam
         public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch()
             => new string[] { DiagnosticUtilities.GetMethodSignatureDisplayName(Method) };
 
-        public TypeDesc? StaticType { get; }
-
         public override SingleValue DeepCopy() => this; // This value is immutable
 
         public override string ToString() => this.ValueToString(Method, DynamicallyAccessedMemberTypes);

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

@@ -538,7 +538,7 @@ public override bool HandleCall(MethodIL callingMethodBody, MethodDesc calledMet
                             // In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
                             // currently it won't do.
 
-                            TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType;
+                            TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
                             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"

src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj

@@ -377,7 +377,6 @@
     <Compile Include="Compiler\Dataflow\HandleCallAction.cs" />
     <Compile Include="Compiler\Dataflow\HoistedLocalKey.cs" />
     <Compile Include="Compiler\Dataflow\InterproceduralState.cs" />
-    <Compile Include="Compiler\Dataflow\IValueWithStaticType.cs" />
     <Compile Include="Compiler\Dataflow\LocalVariableReferenceValue.cs" />
     <Compile Include="Compiler\Dataflow\MethodBodyScanner.cs" />
     <Compile Include="Compiler\Dataflow\MethodParameterValue.cs" />

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

@@ -11,7 +11,11 @@ namespace ILLink.Shared.TrimAnalysis
 {
 	internal partial record FieldValue
 	{
-		public FieldValue (IFieldSymbol fieldSymbol) => FieldSymbol = fieldSymbol;
+		public FieldValue (IFieldSymbol fieldSymbol)
+		{
+			FieldSymbol = fieldSymbol;
+			StaticType = new (fieldSymbol.Type);
+		}
 
 		public readonly IFieldSymbol FieldSymbol;
 

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

@@ -78,6 +78,23 @@ public static DynamicallyAccessedMemberTypes GetMethodReturnValueAnnotation (IMe
 			return returnDamt;
 		}
 
+		public static DynamicallyAccessedMemberTypes GetTypeAnnotation(ITypeSymbol type)
+		{
+			var typeAnnotation = type.GetDynamicallyAccessedMemberTypes ();
+
+			ITypeSymbol? baseType = type.BaseType;
+			while (baseType != null) {
+				typeAnnotation |= baseType.GetDynamicallyAccessedMemberTypes ();
+				baseType = baseType.BaseType;
+			}
+
+			foreach (var interfaceType in type.AllInterfaces) {
+				typeAnnotation |= interfaceType.GetDynamicallyAccessedMemberTypes ();
+			}
+
+			return typeAnnotation;
+		}
+
 #pragma warning disable CA1822 // Mark members as static - the other partial implementations might need to be instance methods
 
 		// TODO: This is relatively expensive on the analyzer since it doesn't cache the annotation information

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

@@ -21,6 +21,7 @@ public MethodParameterValue (ParameterProxy parameter, DynamicallyAccessedMember
 		{
 			Parameter = parameter;
 			DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
+			StaticType = parameter.ParameterType;
 			_overrideIsThis = overrideIsThis;
 		}
 

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

@@ -12,15 +12,15 @@ namespace ILLink.Shared.TrimAnalysis
 	internal partial record MethodReturnValue
 	{
 		public MethodReturnValue (IMethodSymbol methodSymbol)
+			: this (methodSymbol, FlowAnnotations.GetMethodReturnValueAnnotation (methodSymbol))
 		{
-			MethodSymbol = methodSymbol;
-			DynamicallyAccessedMemberTypes = FlowAnnotations.GetMethodReturnValueAnnotation (methodSymbol);
 		}
 
 		public MethodReturnValue (IMethodSymbol methodSymbol, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
 		{
 			MethodSymbol = methodSymbol;
 			DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
+			StaticType = new (methodSymbol.ReturnType);
 		}
 
 		public readonly IMethodSymbol MethodSymbol;

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

@@ -65,17 +65,7 @@ public TrimAnalysisMethodCallPattern Merge (ValueSetLattice<SingleValue> lattice
 		public IEnumerable<Diagnostic> CollectDiagnostics ()
 		{
 			DiagnosticContext diagnosticContext = new (Operation.Syntax.GetLocation ());
-			HandleCallAction handleCallAction = new (diagnosticContext, OwningSymbol, Operation);
-			MethodProxy method = new (CalledMethod);
-			IntrinsicId intrinsicId = Intrinsics.GetIntrinsicIdForMethod (method);
-			if (!handleCallAction.Invoke (method, Instance, Arguments, intrinsicId, out _)) {
-				// If this returns false it means the intrinsic needs special handling:
-				// case IntrinsicId.TypeDelegator_Ctor:
-				//    No diagnostics to report - this is an "identity" operation for data flow, can't produce diagnostics on its own
-				// case IntrinsicId.Array_Empty:
-				//    No diagnostics to report - constant value
-			}
-
+			TrimAnalysisVisitor.HandleCall(Operation, OwningSymbol, CalledMethod, Instance, Arguments, diagnosticContext, default, out var _);
 			return diagnosticContext.Diagnostics;
 		}
 	}

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

@@ -113,12 +113,11 @@ public override MultiValue VisitInstanceReference (IInstanceReferenceOperation i
 
 			// The instance reference operation represents a 'this' or 'base' reference to the containing type,
 			// so we get the annotation from the containing method.
-			if (instanceRef.Type != null && instanceRef.Type.IsTypeInterestingForDataflow ()) {
-				// 'this' is not allowed in field/property initializers, so the owning symbol should be a method.
-				Debug.Assert (OwningSymbol is IMethodSymbol);
-				if (OwningSymbol is IMethodSymbol method)
-					return new MethodParameterValue (method, (ParameterIndex) 0, method.GetDynamicallyAccessedMemberTypes ());
-			}
+			// 'this' is not allowed in field/property initializers, so the owning symbol should be a method.
+			// It can also happen that we see this for a static method - for example a delegate creation
+			// over a local function does this, even thought the "this" makes no sense inside a static scope.
+			if (OwningSymbol is IMethodSymbol method && !method.IsStatic)
+				return new MethodParameterValue (method, (ParameterIndex) 0, method.GetDynamicallyAccessedMemberTypes ());
 
 			return TopValue;
 		}
@@ -186,14 +185,10 @@ public override MultiValue VisitBinaryOperator (IBinaryOperation operation, Stat
 		// - value returned from a method
 
 		public override MultiValue GetFieldTargetValue (IFieldSymbol field)
-		{
-			return field.Type.IsTypeInterestingForDataflow () ? new FieldValue (field) : TopValue;
-		}
+			=> new FieldValue (field);
 
 		public override MultiValue GetParameterTargetValue (IParameterSymbol parameter)
-		{
-			return parameter.Type.IsTypeInterestingForDataflow () ? new MethodParameterValue (parameter) : TopValue;
-		}
+			=> new MethodParameterValue (parameter);
 
 		public override void HandleAssignment (MultiValue source, MultiValue target, IOperation operation)
 		{
@@ -255,28 +250,7 @@ public override MultiValue HandleMethodCall (IMethodSymbol calledMethod, MultiVa
 			//   to noise). ILLink has the same problem currently: https://github.com/dotnet/linker/issues/1952
 
 			var diagnosticContext = DiagnosticContext.CreateDisabled ();
-			var handleCallAction = new HandleCallAction (diagnosticContext, OwningSymbol, operation);
-			MethodProxy method = new (calledMethod);
-			var intrinsicId = Intrinsics.GetIntrinsicIdForMethod (method);
-
-			if (!handleCallAction.Invoke (method, instance, arguments, intrinsicId, out MultiValue methodReturnValue)) {
-				switch (intrinsicId) {
-				case IntrinsicId.Array_Empty:
-					methodReturnValue = ArrayValue.Create (0);
-					break;
-
-				case IntrinsicId.TypeDelegator_Ctor:
-					if (operation is IObjectCreationOperation)
-						methodReturnValue = arguments[0];
-					else
-						methodReturnValue = TopValue;
-
-					break;
-
-				default:
-					throw new InvalidOperationException ($"Unexpected method {calledMethod.GetDisplayName ()} unhandled by HandleCallAction.");
-				}
-			}
+			HandleCall (operation, OwningSymbol, calledMethod, instance, arguments, diagnosticContext, _multiValueLattice, out MultiValue methodReturnValue);
 
 			// This will copy the values if necessary
 			TrimAnalysisPatterns.Add (new TrimAnalysisMethodCallPattern (
@@ -296,6 +270,108 @@ public override MultiValue HandleMethodCall (IMethodSymbol calledMethod, MultiVa
 			return methodReturnValue;
 		}
 
+		internal static void HandleCall(
+			IOperation operation,
+			ISymbol owningSymbol,
+			IMethodSymbol calledMethod,
+			MultiValue instance,
+			ImmutableArray<MultiValue> arguments,
+			DiagnosticContext diagnosticContext,
+			ValueSetLattice<SingleValue> multiValueLattice,
+			out MultiValue methodReturnValue)
+		{
+			var handleCallAction = new HandleCallAction (diagnosticContext, owningSymbol, operation);
+			MethodProxy method = new (calledMethod);
+			var intrinsicId = Intrinsics.GetIntrinsicIdForMethod (method);
+
+			if (handleCallAction.Invoke (method, instance, arguments, intrinsicId, out methodReturnValue)) {
+				return;
+			}
+
+			MultiValue? maybeMethodReturnValue = default;
+
+			switch (intrinsicId) {
+			case IntrinsicId.Array_Empty:
+				AddReturnValue (ArrayValue.Create (0));
+				break;
+
+			case IntrinsicId.TypeDelegator_Ctor:
+				if (operation is IObjectCreationOperation)
+					AddReturnValue (arguments[0]);
+
+				break;
+
+			case IntrinsicId.Object_GetType: {
+					// In the analyzer if the instance is an empty set of values, it actually means "unknown" value
+					// which in this case we want to warn about.
+					MultiValue instanceValue = instance.IsEmpty () ? UnknownValue.Instance : instance;
+
+					foreach (var valueNode in instanceValue) {
+						// Note that valueNode can be statically typed as some generic argument type.
+						// For example:
+						//   void Method<T>(T instance) { instance.GetType().... }
+						// But it could be that T is annotated with for example PublicMethods:
+						//   void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
+						// In this case it's in theory possible to handle it, by treating the T basically as a base class
+						// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
+						// has to happen on the callsite, which doesn't know that GetType() will be used...).
+						// For now we're intentionally ignoring this case - it will produce a warning.
+						// The counter example is:
+						//   Method<Base>(new Derived);
+						// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
+						// currently it won't do.
+
+						// To emulate IL tools behavior (trimmer, NativeAOT compiler), we're going to intentionally "forget" the static type
+						// if it is a generic argument type.
+
+						ITypeSymbol? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
+						if (staticType?.TypeKind == TypeKind.TypeParameter)
+							staticType = null;
+
+						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 (new (calledMethod)));
+						} 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
+
+							// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
+							// on delegates anyway so reflection on something this approximation would miss is actually safe.
+
+							// We can also treat all arrays as "sealed" since it's not legal to derive from Array type (even though it is not sealed itself)
+
+							// We ignore the fact that the type can be annotated (see below for handling of annotated types)
+							// This means the annotations (if any) won't be applied - instead we rely on the exact knowledge
+							// of the type. So for example even if the type is annotated with PublicMethods
+							// but the code calls GetProperties on it - it will work - mark properties, don't mark methods
+							// since we ignored the fact that it's annotated.
+							// This can be seen a little bit as a violation of the annotation, but we already have similar cases
+							// where a parameter is annotated and if something in the method sets a specific known type to it
+							// we will also make it just work, even if the annotation doesn't match the usage.
+							AddReturnValue (new SystemTypeValue (new (staticType)));
+						} else {
+							var annotation = FlowAnnotations.GetTypeAnnotation (staticType);
+							AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (new (calledMethod), annotation));
+						}
+					}
+				}
+
+				break;
+
+			default:
+				Debug.Fail ($"Unexpected method {calledMethod.GetDisplayName ()} unhandled by HandleCallAction.");
+
+				// Do nothing even if we reach a point which we didn't expect - the analyzer should never crash as it's a too disruptive experience for the user.
+				break;
+			}
+
+			methodReturnValue = maybeMethodReturnValue ?? multiValueLattice.Top;
+
+			void AddReturnValue (MultiValue value)
+			{
+				maybeMethodReturnValue = (maybeMethodReturnValue is null) ? value : multiValueLattice.Meet ((MultiValue) maybeMethodReturnValue, value);
+			}
+		}
+
 		public override void HandleReturnValue (MultiValue returnValue, IOperation operation)
 		{
 			// Return statements should only happen inside of method bodies.

src/tools/illink/src/ILLink.Shared/TrimAnalysis/FieldValue.cs

@@ -4,7 +4,12 @@
 // This is needed due to NativeAOT which doesn't enable nullable globally yet
 #nullable enable
 
+using ILLink.Shared.TypeSystemProxy;
+
 namespace ILLink.Shared.TrimAnalysis
 {
-	internal sealed partial record FieldValue : ValueWithDynamicallyAccessedMembers;
+	internal sealed partial record FieldValue : ValueWithDynamicallyAccessedMembers, IValueWithStaticType
+	{
+		public TypeProxy? StaticType { get; }
+	}
 }

src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

@@ -151,7 +151,9 @@ GenericParameterValue genericParam
 			// This needs additional validation that the .ctor is called from a "newobj" instruction/operation
 			// so it can't be done easily in shared code yet.
 			case IntrinsicId.Array_Empty:
-				// Array.Empty<T> must for now be handled by the specific implementation since it requires instantiated generic method handling
+			// Array.Empty<T> must for now be handled by the specific implementation since it requires instantiated generic method handling
+			case IntrinsicId.Object_GetType:
+				// Object.GetType requires additional handling by the caller to implement type hierarchy marking and related diagnostics
 				methodReturnValue = MultiValueLattice.Top;
 				return false;