Fix branch removal in compiler generated code (#3088)

Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.

This does have one possibly negative impact: the issue described in https://github.com/dotnet/linker/issues/2937 is now consistent and happens always.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see https://github.com/dotnet/linker/issues/3087

Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.

In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).

2 files changed, 251 insertions, 8 deletions

diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeInPreservedAssemblyWithWarning.cs b/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeInPreservedAssemblyWithWarning.cs
index b66b0cd30..f05f1b7fb 100644
--- a/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeInPreservedAssemblyWithWarning.cs
+++ b/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeInPreservedAssemblyWithWarning.cs
@@ -26,12 +26,13 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 			WithLocalFunction ();
 		}
 
+		// The compiler generated state will see the modified body,
+		// and will not associate the local function with the user method.
+		// Generic argument warnings from the local function will not be suppressed
+		// by RUC on the user method.
+
 		class Inner
 		{
-			// In this case the compiler generated state will see the modified body,
-			// and will not associate the local function with the user method.
-			// Generic argument warnings from the local function will not be suppressed
-			// by RUC on the user method.
 			[RequiresUnreferencedCode ("--" + nameof (Inner) + "." + nameof (WithLocalFunctionInner) + "--")]
 			public static void WithLocalFunctionInner ()
 			{
@@ -49,10 +50,6 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 			}
 		}
 
-		// In this case the compiler generated state will see the original body,
-		// and will associate the local function with the user method.
-		// Generic argument warnings from the local function will be suppressed
-		// by RUC on the user method.
 		[RequiresUnreferencedCode ("--" + nameof (WithLocalFunction) + "--")]
 		public static void WithLocalFunction ()
 		{
@@ -61,6 +58,7 @@ namespace Mono.Linker.Tests.Cases.DataFlow
 			}
 
 			// https://github.com/dotnet/linker/issues/2937
+			[ExpectedWarning ("IL2091", ProducedBy = ProducedBy.Trimmer)]
 			void LocalWithWarning<T> ()
 			{
 				// No warning
diff --git a/test/Mono.Linker.Tests.Cases/UnreachableBlock/CompilerGeneratedCodeSubstitutions.cs b/test/Mono.Linker.Tests.Cases/UnreachableBlock/CompilerGeneratedCodeSubstitutions.cs
new file mode 100644
index 000000000..5b1edc376
--- /dev/null
+++ b/test/Mono.Linker.Tests.Cases/UnreachableBlock/CompilerGeneratedCodeSubstitutions.cs
@@ -0,0 +1,245 @@
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.Threading.Tasks;
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+
+namespace Mono.Linker.Tests.Cases.UnreachableBlock
+{
+	[SetupLinkerArgument ("--enable-opt", "ipconstprop")]
+
+	// Using Kept validation on compiler generated code is tricky as we would have to describe
+	// all of the compiler generated classes and members which are expected to be kept.
+	// So not using that here (at least until we come up with a better way to do this).
+	// Instead this test relies on RUC and warnings to detect "kept" and "removed" calls.
+	[SkipKeptItemsValidation]
+	[ExpectedNoWarnings]
+	class CompilerGeneratedCodeSubstitutions
+	{
+		static void Main ()
+		{
+			Lambda.Test ();
+			LocalFunction.Test ();
+			Iterator.Test ();
+			Async.Test ();
+		}
+
+		class Lambda
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchInLambda ()
+			{
+				var a = () => {
+					if (AlwaysFalse) {
+						RemovedMethod ();
+					} else {
+						UsedMethod ();
+					}
+				};
+
+				a ();
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchAroundLambda ()
+			{
+				Action a;
+				if (AlwaysFalse) {
+					a = () => RemovedMethod ();
+				} else {
+					a = () => UsedMethod ();
+				}
+
+				a ();
+			}
+
+			public static void Test ()
+			{
+				TestBranchInLambda ();
+				TestBranchAroundLambda ();
+			}
+		}
+
+		class LocalFunction
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchInLocalFunction ()
+			{
+				void LocalFunction ()
+				{
+					if (AlwaysFalse) {
+						RemovedMethod ();
+					} else {
+						UsedMethod ();
+					}
+				}
+
+				LocalFunction ();
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchAroundLocalFunction ()
+			{
+				Action a;
+				if (AlwaysFalse) {
+					void RemovedLocalFunction ()
+					{
+						RemovedMethod ();
+					}
+
+					RemovedLocalFunction ();
+				} else {
+					void UsedLocalFunction ()
+					{
+						UsedMethod ();
+					}
+
+					UsedLocalFunction ();
+				}
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchAroundUsageOfLocalFunction ()
+			{
+				Action a;
+				if (AlwaysFalse) {
+					RemovedLocalFunction ();
+				} else {
+					UsedLocalFunction ();
+				}
+
+				void RemovedLocalFunction ()
+				{
+					RemovedMethod ();
+				}
+
+				void UsedLocalFunction ()
+				{
+					UsedMethod ();
+				}
+			}
+
+			public static void Test ()
+			{
+				TestBranchInLocalFunction ();
+				TestBranchAroundLocalFunction ();
+				TestBranchAroundUsageOfLocalFunction ();
+			}
+		}
+
+		class Iterator
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static IEnumerable<int> TestBranchWithNormalCall ()
+			{
+				if (AlwaysFalse) {
+					RemovedMethod ();
+				} else {
+					UsedMethod ();
+				}
+
+				yield return 1;
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static IEnumerable<int> TestBranchWithYieldAfter ()
+			{
+				if (AlwaysFalse) {
+					RemovedMethod ();
+					yield return 1;
+				} else {
+					UsedMethod ();
+					yield return 1;
+				}
+
+				yield return 1;
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			// https://github.com/dotnet/linker/issues/3087
+			[ExpectedWarning ("IL2026", "--RemovedMethod--", CompilerGeneratedCode = true)]
+			static IEnumerable<int> TestBranchWithYieldBefore ()
+			{
+				if (AlwaysFalse) {
+					yield return 1;
+					RemovedMethod ();
+				} else {
+					yield return 1;
+					UsedMethod ();
+				}
+
+				yield return 1;
+			}
+
+			public static void Test ()
+			{
+				TestBranchWithNormalCall ();
+				TestBranchWithYieldAfter ();
+				TestBranchWithYieldBefore ();
+			}
+		}
+
+		class Async
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static async Task TestBranchWithNormalCall ()
+			{
+				if (AlwaysFalse) {
+					RemovedMethod ();
+				} else {
+					UsedMethod ();
+				}
+
+				await Task.FromResult (0);
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			// https://github.com/dotnet/linker/issues/3087
+			[ExpectedWarning ("IL2026", "--RemovedMethod--", CompilerGeneratedCode = true)]
+			static async Task TestBranchWithNormalCallAfterWAwait ()
+			{
+				if (AlwaysFalse) {
+					await Task.FromResult (0);
+					RemovedMethod ();
+				} else {
+					await Task.FromResult (0);
+					UsedMethod ();
+				}
+
+				await Task.FromResult (0);
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedAsyncMethod--", CompilerGeneratedCode = true)]
+			static async Task TestBranchWithAsyncCall ()
+			{
+				if (AlwaysFalse) {
+					await RemovedAsyncMethod ();
+				} else {
+					await UsedAsyncMethod ();
+				}
+			}
+
+			public static void Test ()
+			{
+				TestBranchWithNormalCall ().RunSynchronously (); ;
+				TestBranchWithNormalCallAfterWAwait ().RunSynchronously ();
+				TestBranchWithAsyncCall ().RunSynchronously ();
+			}
+		}
+
+		static bool AlwaysFalse => false;
+
+		[RequiresUnreferencedCode ("--UsedAsyncMethod--")]
+		static async Task UsedAsyncMethod () => await Task.FromResult (0);
+
+		[RequiresUnreferencedCode ("--RemovedAsyncMethod--")]
+		static async Task RemovedAsyncMethod () => await Task.FromResult (-1);
+
+		[RequiresUnreferencedCode ("--UsedMethod--")]
+		static void UsedMethod () { }
+
+		[RequiresUnreferencedCode ("--RemovedMethod--")]
+		static void RemovedMethod () { }
+	}
+}