[dotnet-linker] Trim away the static constructor for protocol interfaces if we're registering protocols in the static registrar. (#21012)

When we implemented support for using default interface members for binding protocols, we also unintentionally introduced a size regression. This happened because we now tell the linker to keep all methods in a protocol interface, thus all the corresponding types end up marked as well, etc.

This had an additional side effect: depending on the types that weren't linked away anymore, the App Store could flag an app, saying that additional entitlements is needed. This is what's happening in #21002: the App Store detects that the app references the `application:didRegisterForRemoteNotificationsWithDeviceToken:` selector [1] (from the method `RegisteredForRemoteNotifications` on `IUIApplicationDelegate`) and tells the developer they probably need the push notification entitlement.

The good news is that we don't need these protocol interface methods at runtime if the optimization to register protocols with the static registrar is enabled (which it is by default).

In this PR I teach the optimizer to remove the DynamicDependency attributes keeping these protocol interface methods from being trimmed out.

## Size improvements

* monotouch-test build for Release/ios-arm64 shrinks by [2.9mb (-2.6%)](https://gist.github.com/rolfbjarne/5e8ca6ea6854dc4a46f8e838dff11e6b)
* A very simple app (tests/dotnet/MySimpleApp) shrinks by [176kb (-0.3%)](https://gist.github.com/rolfbjarne/f0e062900528eb499fd96d124d18376f)

[1]: This is somewhat speculative, but it's probably not far from what the App Store actually does.

Fixes #21002.

src/bgen/Generator.cs

@@ -5074,6 +5074,7 @@ public partial class Generator : IMemberGatherer {
 			foreach (var docId in docIds) {
 				print ($"[DynamicDependencyAttribute (\"{docId}\")]");
 			}
+			print ("[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]");
 			print ($"static I{TypeName} ()");
 			print ("{");
 			print ("\tGC.KeepAlive (null);"); // need to do _something_ (doesn't seem to matter what), otherwise the static cctor (and the DynamicDependency attributes) are trimmed away.

tests/monotouch-test/ObjCRuntime/RegistrarTest.cs

@@ -3,6 +3,7 @@ using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Drawing;
 using System.Linq;
+using System.Reflection;
 using System.Runtime.InteropServices;
 using System.Runtime.Versioning;
 using System.Threading;
@@ -5536,6 +5537,37 @@ namespace MonoTouchFixtures.ObjCRuntime {
 				Assert.AreEqual (EnumUL.b, ul, "out: UL");
 			}
 		}
+
+#if NET && HAS_UIKIT
+		[Test]
+		public void ProtocolsTrimmedAway ()
+		{
+			PreserveIUIApplicationDelegate (null);
+
+			// A little indirection to try to make the trimmer not be helpful and preserve all the methods on IUIApplicationDelegate.
+			AssertMemberCount (typeof (IUIApplicationDelegate));
+		}
+
+		void AssertMemberCount (Type type)
+		{
+			var members = type.GetMembers (BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.Instance);
+#if OPTIMIZEALL || NATIVEAOT
+			var expectNoMembers = true;
+#else
+			var expectNoMembers = false;
+#endif
+			if (expectNoMembers) {
+				Assert.AreEqual (0, members.Length, $"All members should be trimmed away in {type.FullName}:\n\t{string.Join ("\n\t", members.Select (v => v.ToString ()))}");
+			} else {
+				Assert.AreNotEqual (0, members.Length, $"All members should not be trimmed away in {type.FullName}");
+			}
+		}
+
+		void PreserveIUIApplicationDelegate (IUIApplicationDelegate obj)
+		{
+			GC.KeepAlive (obj);
+		}
+#endif // NET && HAS_UIKIT
 	}
 
 #if !__WATCHOS__

tools/dotnet-linker/Steps/CollectUnmarkedMembers.cs

@@ -26,6 +26,12 @@ namespace Xamarin.Linker {
 			if (!Annotations.IsMarked (type))
 				LinkContext.AddLinkedAwayType (type);
 
+			if (type.IsInterface &&
+				Configuration.DerivedLinkContext.App.Optimizations.RegisterProtocols == true &&
+				type.HasCustomAttribute (LinkContext, Namespaces.Foundation, "ProtocolAttribute")) {
+				Configuration.DerivedLinkContext.StoreProtocolMethods (type);
+			}
+
 			if (type.HasInterfaces) {
 				foreach (var iface in type.Interfaces) {
 					if (Annotations.IsMarked (iface))

tools/dotnet-linker/Steps/PreMarkDispatcher.cs

@@ -7,6 +7,7 @@ namespace Xamarin.Linker.Steps {
 	class PreMarkDispatcher : SubStepsDispatcher {
 		public PreMarkDispatcher ()
 			: base (new BaseSubStep [] {
+				new SetBeforeFieldInitStep (),
 				new CollectUnmarkedMembersSubStep (),
 				new StoreAttributesStep ()
 				})

tools/dotnet-linker/Steps/SetBeforeFieldInitStep.cs

@@ -0,0 +1,55 @@
+using Mono.Linker.Steps;
+using Xamarin.Linker;
+
+using Mono.Cecil;
+using Mono.Tuner;
+
+#nullable enable
+
+namespace Xamarin.Linker.Steps {
+	public class SetBeforeFieldInitStep : ConfigurationAwareSubStep {
+		protected override string Name { get; } = "Set BeforeFieldInit";
+		protected override int ErrorCode { get; } = 2380;
+
+		public override SubStepTargets Targets {
+			get {
+				return SubStepTargets.Type;
+			}
+		}
+
+		protected override void Process (TypeDefinition type)
+		{
+			// If we're registering protocols, we want to remove the static
+			// constructor on the protocol interface, because it's not needed
+			// (because we've removing all the DynamicDependency attributes
+			// from the cctor).
+			//
+			// However, just removing the static constructor from the type
+			// causes problems later on in the trimming process, so we want
+			// the trimmer to just not mark it.
+			//
+			// The trimmer marks it, because it has a static constructor, so
+			// we're in a bit of a cyclic dependency here.
+			//
+			// This is complicated by a few facts:
+			// - When we optimize the cctor (i.e. removing the
+			//   DynamicDependency attributes), the cctor is already marked.
+			// - Adding a MarkHandler that processes types doesn't work
+			//   either, because it may be called after the cctor is marked:
+			//   https://github.com/dotnet/runtime/blob/6177a9f920861900681cfda2b6cc66ac3557e93b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs#L1928-L1952
+			//
+			// So this is a pre-mark step that just sets
+			// IsBeforeFieldInit=true for interfaces we want trimmed away by
+			// the linker.
+
+			if (Configuration.DerivedLinkContext.App.Optimizations.RegisterProtocols != true)
+				return;
+
+			if (!type.IsBeforeFieldInit && type.IsInterface && type.HasMethods) {
+				var cctor = type.GetTypeConstructor ();
+				if (cctor is not null && cctor.IsBindingImplOptimizableCode (LinkContext))
+					type.IsBeforeFieldInit = true;
+			}
+		}
+	}
+}

tools/linker/CoreOptimizeGeneratedCode.cs

@@ -755,6 +755,9 @@ namespace Xamarin.Linker {
 				return; // nothing else to do here.
 			}
 
+			if (ProcessProtocolInterfaceStaticConstructor (method))
+				return;
+
 			var instructions = method.Body.Instructions;
 			for (int i = 0; i < instructions.Count; i++) {
 				var ins = instructions [i];
@@ -1191,6 +1194,19 @@ namespace Xamarin.Linker {
 			return ins;
 		}
 
+		static Instruction SkipNops (Instruction ins)
+		{
+			if (ins is null)
+				return null;
+
+			while (ins.OpCode == OpCodes.Nop) {
+				if (ins.Next is null)
+					return null;
+				ins = ins.Next;
+			}
+			return ins;
+		}
+
 		int ProcessIsARM64CallingConvention (MethodDefinition caller, Instruction ins)
 		{
 			const string operation = "inline Runtime.IsARM64CallingConvention";
@@ -1345,5 +1361,66 @@ namespace Xamarin.Linker {
 			}
 			return caller.Module.ImportReference (block_ctor_def);
 		}
+
+		bool ProcessProtocolInterfaceStaticConstructor (MethodDefinition method)
+		{
+			// The static cctor in protocol interfaces exists only to preserve the protocol's members, for inspection by the registrar(s).
+			// If we're registering protocols, then we don't need to preserve protocol members, because the registrar
+			// already knows everything about it => we can remove the static cctor.
+
+			if (!(method.DeclaringType.IsInterface && method.DeclaringType.IsInterface && method.IsStatic && method.IsConstructor && method.HasBody))
+				return false;
+
+			if (Optimizations.RegisterProtocols != true) {
+				Driver.Log (4, "Did not optimize static constructor in the protocol interface {0}: the 'register-protocols' optimization is disabled.", method.DeclaringType.FullName);
+				return false;
+			}
+
+			if (!method.DeclaringType.HasCustomAttributes || !method.DeclaringType.CustomAttributes.Any (v => v.AttributeType.Is ("Foundation", "ProtocolAttribute"))) {
+				Driver.Log (4, "Did not optimize static constructor in the protocol interface {0}: no Protocol attribute found.", method.DeclaringType.FullName);
+				return false;
+			}
+
+			var ins = SkipNops (method.Body.Instructions.First ());
+			if (ins is null) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_A /* Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). */, method.DeclaringType.FullName));
+				return false;
+			} else if (ins.OpCode != OpCodes.Ldnull) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset));
+				return false;
+			}
+
+			ins = SkipNops (ins.Next);
+			var callGCKeepAlive = ins;
+			if (ins is null) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_A /* Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). */, method.DeclaringType.FullName));
+				return false;
+			} else if (callGCKeepAlive.OpCode != OpCodes.Call || !(callGCKeepAlive.Operand is MethodReference methodOperand) || methodOperand.Name != "KeepAlive" || !methodOperand.DeclaringType.Is ("System", "GC")) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset));
+				return false;
+			}
+
+			ins = SkipNops (ins.Next);
+			if (ins is null) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_A /* Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon). */, method.DeclaringType.FullName));
+				return false;
+			} else if (ins.OpCode != OpCodes.Ret) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset));
+				return false;
+			}
+
+			ins = SkipNops (ins.Next);
+			if (ins is not null) {
+				ErrorHelper.Show (ErrorHelper.CreateWarning (LinkContext.App, 2112, method, ins, Errors.MX2112_B /* Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}. */, method.DeclaringType.FullName, ins.OpCode, ins.Offset));
+				return false;
+			}
+
+			// We can just remove the entire method, however that confuses the linker later on, so just empty it out and remove all the attributes.
+			Driver.Log (4, "Optimized static constructor in the protocol interface {0} (static constructor was cleared and custom attributes removed)", method.DeclaringType.FullName);
+			method.Body.Instructions.Clear ();
+			method.Body.Instructions.Add (Instruction.Create (OpCodes.Ret));
+			method.CustomAttributes.Clear ();
+			return true;
+		}
 	}
 }

tools/mtouch/Errors.designer.cs

@@ -3923,6 +3923,24 @@ namespace Xamarin.Bundler {
             }
         }
         
+        /// <summary>
+        ///   Looks up a localized string similar to Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon)..
+        /// </summary>
+        public static string MX2112_A {
+            get {
+                return ResourceManager.GetString("MX2112_A", resourceCulture);
+            }
+        }
+        
+        /// <summary>
+        ///   Looks up a localized string similar to Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}..
+        /// </summary>
+        public static string MX2112_B {
+            get {
+                return ResourceManager.GetString("MX2112_B", resourceCulture);
+            }
+        }
+        
         /// <summary>
         ///   Looks up a localized string similar to Could not {0} the assembly &apos;{1}&apos;
         ///		.

tools/mtouch/Errors.resx

@@ -1327,6 +1327,13 @@
 		</value>
 	</data>
 
+	<data name="MX2112_A" xml:space="preserve">
+		<value>Could not optimize the static constructor in the interface {0} because it did not have the expected instruction sequence (found end of method too soon).</value>
+	</data>
+
+	<data name="MX2112_B" xml:space="preserve">
+		<value>Could not optimize the static constructor in the interface {0} because it had an unexpected instruction {1} at offset {2}.</value>
+	</data>
 	<!-- 2200 -> 2299 is used by/reserved for ExceptionalSubStep subclasses in the linker -->
 	<!-- 220x: PreserveSmartEnumConversionsSubStep -->
 	<!-- 221x: RemoveBitcodeIncompatibleCodeStep -->