Handle PInvoke analysis similar to other methods (#2091)

* PInvoke fix

* FB

* FB

* FB2

4 files changed, 259 insertions, 100 deletions

diff --git a/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/linker/Linker.Dataflow/MethodBodyScanner.cs
index 1ffce3d53..fa8ca8dd1 100644
--- a/src/linker/Linker.Dataflow/MethodBodyScanner.cs
+++ b/src/linker/Linker.Dataflow/MethodBodyScanner.cs
@@ -526,13 +526,13 @@ namespace Mono.Linker.Dataflow
 						}
 
 						// Pop arguments
-						PopUnknown (currentStack, signature.Parameters.Count, methodBody, operation.Offset);
+						if (signature.Parameters.Count > 0)
+							PopUnknown (currentStack, signature.Parameters.Count, methodBody, operation.Offset);
 
 						// Pop function pointer
 						PopUnknown (currentStack, 1, methodBody, operation.Offset);
 
-						// Push return value
-						if (signature.ReturnType.MetadataType != MetadataType.Void)
+						if (GetReturnTypeWithoutModifiers (signature.ReturnType).MetadataType != MetadataType.Void)
 							PushUnknown (currentStack);
 					}
 					break;
@@ -567,7 +567,9 @@ namespace Mono.Linker.Dataflow
 					break;
 
 				case Code.Ret: {
-						bool hasReturnValue = methodBody.Method.ReturnType.MetadataType != MetadataType.Void;
+
+						bool hasReturnValue = GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void;
+
 						if (currentStack.Count != (hasReturnValue ? 1 : 0)) {
 							WarnAboutInvalidILInMethod (methodBody, operation.Offset);
 						}
@@ -892,7 +894,7 @@ namespace Mono.Linker.Dataflow
 					else
 						methodReturnValue = newObjValue;
 				} else {
-					if (calledMethod.ReturnType.MetadataType != MetadataType.Void) {
+					if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) {
 						methodReturnValue = UnknownValue.Instance;
 					}
 				}
@@ -908,6 +910,14 @@ namespace Mono.Linker.Dataflow
 			}
 		}
 
+		protected static TypeReference GetReturnTypeWithoutModifiers (TypeReference returnType)
+		{
+			while (returnType is IModifierType) {
+				returnType = ((IModifierType) returnType).ElementType;
+			}
+			return returnType;
+		}
+
 		// Array types that are dynamically accessed should resolve to System.Array instead of its element type - which is what Cecil resolves to.
 		// Any data flow annotations placed on a type parameter which receives an array type apply to the array itself. None of the members in its
 		// element type should be marked.
diff --git a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
index 9459ce1d7..a453702d0 100644
--- a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
+++ b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
@@ -37,10 +37,10 @@ namespace Mono.Linker.Dataflow
 			if (methodDefinition == null)
 				return false;
 
-			return
-				GetIntrinsicIdForMethod (methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel ||
+			return GetIntrinsicIdForMethod (methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel ||
 				context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (methodDefinition) ||
-				context.Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (methodDefinition);
+				context.Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (methodDefinition)
+				|| methodDefinition.IsPInvokeImpl;
 		}
 
 		public static bool RequiresReflectionMethodBodyScannerForMethodBody (FlowAnnotations flowAnnotations, MethodDefinition methodDefinition)
@@ -75,7 +75,7 @@ namespace Mono.Linker.Dataflow
 		{
 			Scan (methodBody);
 
-			if (methodBody.Method.ReturnType.MetadataType != MetadataType.Void) {
+			if (GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void) {
 				var method = methodBody.Method;
 				var requiredMemberTypes = _context.Annotations.FlowAnnotations.GetReturnParameterAnnotation (method);
 				if (requiredMemberTypes != 0) {
@@ -1729,6 +1729,20 @@ namespace Mono.Linker.Dataflow
 					break;
 
 				default:
+
+					if (calledMethodDefinition.IsPInvokeImpl) {
+						// Is the PInvoke dangerous?
+						bool comDangerousMethod = IsComInterop (calledMethodDefinition.MethodReturnType, calledMethodDefinition.ReturnType);
+						foreach (ParameterDefinition pd in calledMethodDefinition.Parameters) {
+							comDangerousMethod |= IsComInterop (pd, pd.ParameterType);
+						}
+
+						if (comDangerousMethod) {
+							reflectionContext.AnalyzingPattern ();
+							reflectionContext.RecordUnrecognizedPattern (2050, $"P/invoke method '{calledMethodDefinition.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.");
+						}
+					}
+
 					if (requiresDataFlowAnalysis) {
 						reflectionContext.AnalyzingPattern ();
 						for (int parameterIndex = 0; parameterIndex < methodParams.Count; parameterIndex++) {
@@ -1755,7 +1769,7 @@ namespace Mono.Linker.Dataflow
 
 					// To get good reporting of errors we need to track the origin of the value for all method calls
 					// but except Newobj as those are special.
-					if (calledMethodDefinition.ReturnType.MetadataType != MetadataType.Void) {
+					if (GetReturnTypeWithoutModifiers (calledMethodDefinition.ReturnType).MetadataType != MetadataType.Void) {
 						methodReturnValue = CreateMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes);
 
 						return true;
@@ -1771,7 +1785,7 @@ namespace Mono.Linker.Dataflow
 			// 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.
 			if (methodReturnValue == null) {
-				if (calledMethod.ReturnType.MetadataType != MetadataType.Void) {
+				if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) {
 					methodReturnValue = CreateMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes);
 				}
 			}
@@ -1794,6 +1808,64 @@ namespace Mono.Linker.Dataflow
 			return true;
 		}
 
+		bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType)
+		{
+			// This is best effort. One can likely find ways how to get COM without triggering these alarms.
+			// AsAny marshalling of a struct with an object-typed field would be one, for example.
+
+			// This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR,
+			// not trying to handle invalid cases and distinctions that are not interesting wrt
+			// "is this COM?" question.
+
+			NativeType nativeType = NativeType.None;
+			if (marshalInfoProvider.HasMarshalInfo) {
+				nativeType = marshalInfoProvider.MarshalInfo.NativeType;
+			}
+
+			if (nativeType == NativeType.IUnknown || nativeType == NativeType.IDispatch || nativeType == NativeType.IntF) {
+				// This is COM by definition
+				return true;
+			}
+
+			if (nativeType == NativeType.None) {
+				// Resolve will look at the element type
+				var parameterTypeDef = _context.TryResolve (parameterType);
+
+				if (parameterTypeDef != null) {
+					if (parameterTypeDef.IsTypeOf ("System", "Array")) {
+						// System.Array marshals as IUnknown by default
+						return true;
+					} else if (parameterTypeDef.IsTypeOf ("System", "String") ||
+						parameterTypeDef.IsTypeOf ("System.Text", "StringBuilder")) {
+						// String and StringBuilder are special cased by interop
+						return false;
+					}
+
+					if (parameterTypeDef.IsValueType) {
+						// Value types don't marshal as COM
+						return false;
+					} else if (parameterTypeDef.IsInterface) {
+						// Interface types marshal as COM by default
+						return true;
+					} else if (parameterTypeDef.IsMulticastDelegate ()) {
+						// Delegates are special cased by interop
+						return false;
+					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "CriticalHandle")) {
+						// Subclasses of CriticalHandle are special cased by interop
+						return false;
+					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "SafeHandle")) {
+						// Subclasses of SafeHandle are special cased by interop
+						return false;
+					} else if (!parameterTypeDef.IsSequentialLayout && !parameterTypeDef.IsExplicitLayout) {
+						// Rest of classes that don't have layout marshal as COM
+						return true;
+					}
+				}
+			}
+
+			return false;
+		}
+
 		bool AnalyzeGenericInstatiationTypeArray (ValueNode arrayParam, ref ReflectionPatternContext reflectionContext, MethodReference calledMethod, IList<GenericParameter> genericParameters)
 		{
 			bool hasRequirements = false;
diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs
index 23a3d9f38..d17c8f6ce 100644
--- a/src/linker/Linker.Steps/MarkStep.cs
+++ b/src/linker/Linker.Steps/MarkStep.cs
@@ -3042,8 +3042,6 @@ namespace Mono.Linker.Steps
 
 			TypeDefinition returnTypeDefinition = _context.TryResolve (method.ReturnType);
 
-			bool didWarnAboutCom = false;
-
 			const bool includeStaticFields = false;
 			if (returnTypeDefinition != null) {
 				if (!returnTypeDefinition.IsImport) {
@@ -3051,19 +3049,6 @@ namespace Mono.Linker.Steps
 					MarkDefaultConstructor (returnTypeDefinition, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
 					MarkFields (returnTypeDefinition, includeStaticFields, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
 				}
-
-				// Best-effort attempt to find COM marshalling.
-				// It might seem reasonable to allow out parameters, but once we get a foreign object back (an RCW), it's going to look
-				// like a regular managed class/interface, but every method invocation that takes a class/interface implies COM
-				// marshalling. We can't detect that once we have an RCW.
-				if (method.IsPInvokeImpl) {
-					if (IsComInterop (method.MethodReturnType, method.ReturnType) && !didWarnAboutCom) {
-						_context.LogWarning (
-							$"P/invoke method '{method.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.",
-							2050, method, subcategory: MessageSubCategory.TrimAnalysis);
-						didWarnAboutCom = true;
-					}
-				}
 			}
 
 			if (method.HasThis && !method.DeclaringType.IsImport) {
@@ -3085,75 +3070,8 @@ namespace Mono.Linker.Steps
 							MarkDefaultConstructor (paramTypeDefinition, new DependencyInfo (DependencyKind.InteropMethodDependency, method));
 						}
 					}
-
-					// Best-effort attempt to find COM marshalling.
-					// It might seem reasonable to allow out parameters, but once we get a foreign object back (an RCW), it's going to look
-					// like a regular managed class/interface, but every method invocation that takes a class/interface implies COM
-					// marshalling. We can't detect that once we have an RCW.
-					if (method.IsPInvokeImpl) {
-						if (IsComInterop (pd, paramTypeReference) && !didWarnAboutCom) {
-							_context.LogWarning ($"P/invoke method '{method.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.", 2050, method);
-							didWarnAboutCom = true;
-						}
-					}
-				}
-			}
-		}
-
-		bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType)
-		{
-			// This is best effort. One can likely find ways how to get COM without triggering these alarms.
-			// AsAny marshalling of a struct with an object-typed field would be one, for example.
-
-			// This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR,
-			// not trying to handle invalid cases and distinctions that are not interesting wrt
-			// "is this COM?" question.
-
-			NativeType nativeType = NativeType.None;
-			if (marshalInfoProvider.HasMarshalInfo) {
-				nativeType = marshalInfoProvider.MarshalInfo.NativeType;
-			}
-
-			if (nativeType == NativeType.IUnknown || nativeType == NativeType.IDispatch || nativeType == NativeType.IntF) {
-				// This is COM by definition
-				return true;
-			}
-
-			if (nativeType == NativeType.None) {
-				if (parameterType.IsTypeOf ("System", "Array")) {
-					// System.Array marshals as IUnknown by default
-					return true;
-				} else if (parameterType.IsTypeOf ("System", "String") ||
-					parameterType.IsTypeOf ("System.Text", "StringBuilder")) {
-					// String and StringBuilder are special cased by interop
-					return false;
-				}
-
-				var parameterTypeDef = _context.TryResolve (parameterType);
-				if (parameterTypeDef != null) {
-					if (parameterTypeDef.IsValueType) {
-						// Value types don't marshal as COM
-						return false;
-					} else if (parameterTypeDef.IsInterface) {
-						// Interface types marshal as COM by default
-						return true;
-					} else if (parameterTypeDef.IsMulticastDelegate ()) {
-						// Delegates are special cased by interop
-						return false;
-					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "CriticalHandle")) {
-						// Subclasses of CriticalHandle are special cased by interop
-						return false;
-					} else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "SafeHandle")) {
-						// Subclasses of SafeHandle are special cased by interop
-						return false;
-					} else if (!parameterTypeDef.IsSequentialLayout && !parameterTypeDef.IsExplicitLayout) {
-						// Rest of classes that don't have layout marshal as COM
-						return true;
-					}
 				}
 			}
-
-			return false;
 		}
 
 		protected virtual bool ShouldParseMethodBody (MethodDefinition method)
diff --git a/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs b/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs
index 40e661340..720ad738b 100644
--- a/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs
+++ b/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs
@@ -15,39 +15,198 @@ namespace Mono.Linker.Tests.Cases.Interop.PInvoke.Warnings
 		[UnconditionalSuppressMessage ("trim", "IL2026")]
 		static void Main ()
 		{
-			SomeMethodTakingInterface (null);
-			SomeMethodTakingObject (null);
-			GetInterface ();
-			CanSuppressWarningOnParameter (null);
-			CanSuppressWarningOnReturnType ();
-			CanSuppressWithRequiresUnreferencedCode (null);
+			Call_SomeMethodTakingInterface ();
+			Call_SomeMethodTakingObject ();
+			Call_SomeMethodTakingArray ();
+			Call_SomeMethodTakingString ();
+			Call_SomeMethodTakingStringBuilder ();
+			Call_SomeMethodTakingCriticalHandle ();
+			Call_SomeMethodTakingSafeHandle ();
+			Call_SomeMethodTakingExplicitLayoutClass ();
+			Call_SomeMethodTakingSequentialLayoutClass ();
+			Call_SomeMethodTakingAutoLayoutClass ();
+			Call_GetInterface ();
+			Call_CanSuppressWarningOnParameter ();
+			Call_CanSuppressWarningOnReturnType ();
+			Call_CanSuppressWithRequiresUnreferencedCode ();
+			Call_CanSuppressPInvokeWithRequiresUnreferencedCode ();
+			Call_PInvokeWithRequiresUnreferencedCode ();
 		}
 
 		[ExpectedWarning ("IL2050")]
+		static void Call_SomeMethodTakingInterface ()
+		{
+			SomeMethodTakingInterface (null);
+		}
 		[DllImport ("Foo")]
 		static extern void SomeMethodTakingInterface (IFoo foo);
 
 		[ExpectedWarning ("IL2050")]
+		static void Call_SomeMethodTakingObject ()
+		{
+			SomeMethodTakingObject (null);
+		}
 		[DllImport ("Foo")]
 		static extern void SomeMethodTakingObject ([MarshalAs (UnmanagedType.IUnknown)] object obj);
 
 		[ExpectedWarning ("IL2050")]
+		static void Call_SomeMethodTakingArray ()
+		{
+			SomeMethodTakingArray (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingArray (Array array);
+
+		static void Call_SomeMethodTakingStringBuilder ()
+		{
+			SomeMethodTakingStringBuilder (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingStringBuilder (StringBuilder str);
+
+		static void Call_SomeMethodTakingCriticalHandle ()
+		{
+			SomeMethodTakingCriticalHandle (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingCriticalHandle (MyCriticalHandle handle);
+
+
+		static void Call_SomeMethodTakingSafeHandle ()
+		{
+			SomeMethodTakingSafeHandle (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingSafeHandle (TestSafeHandle handle);
+
+		static void Call_SomeMethodTakingExplicitLayoutClass ()
+		{
+			SomeMethodTakingExplicitLayout (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingExplicitLayout (ExplicitLayout _class);
+
+		static void Call_SomeMethodTakingSequentialLayoutClass ()
+		{
+			SomeMethodTakingSequentialLayout (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingSequentialLayout (SequentialLayout _class);
+
+		[ExpectedWarning ("IL2050")]
+		static void Call_SomeMethodTakingAutoLayoutClass ()
+		{
+			SomeMethodTakingAutoLayout (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingAutoLayout (AutoLayout _class);
+
+
+		static void Call_SomeMethodTakingString ()
+		{
+			SomeMethodTakingString (null);
+		}
+		[DllImport ("Foo")]
+		static extern void SomeMethodTakingString (String str);
+
+		[ExpectedWarning ("IL2050")]
+		static void Call_GetInterface ()
+		{
+			GetInterface ();
+		}
 		[DllImport ("Foo")]
 		static extern IFoo GetInterface ();
 
 		[UnconditionalSuppressMessage ("trim", "IL2050")]
+		static void Call_CanSuppressWarningOnParameter ()
+		{
+			CanSuppressWarningOnParameter (null);
+		}
 		[DllImport ("Foo")]
 		static extern void CanSuppressWarningOnParameter ([MarshalAs (UnmanagedType.IUnknown)] object obj);
 
 		[UnconditionalSuppressMessage ("trim", "IL2050")]
+		static void Call_CanSuppressWarningOnReturnType ()
+		{
+			CanSuppressWarningOnReturnType ();
+		}
 		[DllImport ("Foo")]
 		static extern IFoo CanSuppressWarningOnReturnType ();
 
-		[ExpectedWarning ("IL2050")] // Issue https://github.com/mono/linker/issues/1989
 		[RequiresUnreferencedCode ("test")]
+		static void Call_CanSuppressWithRequiresUnreferencedCode ()
+		{
+			CanSuppressWithRequiresUnreferencedCode (null);
+		}
+
 		[DllImport ("Foo")]
 		static extern void CanSuppressWithRequiresUnreferencedCode (IFoo foo);
 
+		[RequiresUnreferencedCode ("test")]
+		static void Call_CanSuppressPInvokeWithRequiresUnreferencedCode ()
+		{
+			CanSuppressPInvokeWithRequiresUnreferencedCode (null);
+		}
+
+		[RequiresUnreferencedCode ("test")]
+		[DllImport ("Foo")]
+		static extern void CanSuppressPInvokeWithRequiresUnreferencedCode (IFoo foo);
+
+		[ExpectedWarning ("IL2050")]
+		[ExpectedWarning ("IL2026")]
+		static void Call_PInvokeWithRequiresUnreferencedCode ()
+		{
+			PInvokeWithRequiresUnreferencedCode (null);
+		}
+
+		[RequiresUnreferencedCode ("test")]
+		[DllImport ("Foo")]
+		static extern void PInvokeWithRequiresUnreferencedCode (IFoo foo);
+
 		interface IFoo { }
+
+		class TestSafeHandle : SafeHandle
+		{
+			public TestSafeHandle ()
+				: base (IntPtr.Zero, true)
+			{ }
+
+			public override bool IsInvalid => handle == IntPtr.Zero;
+
+			protected override bool ReleaseHandle ()
+			{
+				return true;
+			}
+		}
+
+		class MyCriticalHandle : CriticalHandle
+		{
+			public MyCriticalHandle () : base (new IntPtr (-1)) { }
+
+			public override bool IsInvalid {
+				get { return false; }
+			}
+
+			protected override bool ReleaseHandle ()
+			{
+				return false;
+			}
+		}
+
+		[StructLayout (LayoutKind.Explicit)]
+		public class ExplicitLayout
+		{
+		}
+
+		[StructLayout (LayoutKind.Sequential)]
+		public class SequentialLayout
+		{
+		}
+
+		[StructLayout (LayoutKind.Auto)]
+		public class AutoLayout
+		{
+		}
+
 	}
 }