[Xamarin.Android.Tools.Bytecode] Hide Kotlin synthetic constructors (#…

…700)

Fixes: #694

Context: dotnet/android#3776

When using a Kotlin default constructor like:

	class MaterialDialog(
	  val windowContext: Context,
	  val dialogBehavior: DialogBehavior = DEFAULT_BEHAVIOR
	) : Dialog(windowContext, inferTheme(windowContext, dialogBehavior))
	{ ... }

Kotlin will create 2 constructors, the "real" one, and a synthetic one
denoting which constructor is the default constructor using a
`kotlin.jvm.internal.DefaultConstructorMarker` parameter:

	<constructor deprecated="not deprecated" final="false"
	    name="MaterialDialog" static="false" visibility="public"
	    bridge="false" synthetic="false"
	    jni-signature="(Landroid/content/Context;Lcom/afollestad/materialdialogs/DialogBehavior;)V">
	  <parameter name="windowContext" type="android.content.Context" jni-type="Landroid/content/Context;" not-null="true"/>
	  <parameter name="dialogBehavior" type="com.afollestad.materialdialogs.DialogBehavior" jni-type="Lcom/afollestad/materialdialogs/DialogBehavior;" not-null="true"/>
	</constructor>
	<constructor deprecated="not deprecated" final="false"
	    name="MaterialDialog" static="false" visibility="public"
	    bridge="false" synthetic="true"
	    jni-signature="(Landroid/content/Context;Lcom/afollestad/materialdialogs/DialogBehavior;ILkotlin/jvm/internal/DefaultConstructorMarker;)V">
	  <parameter name="p0" type="android.content.Context" jni-type="Landroid/content/Context;"/>
	  <parameter name="p1" type="com.afollestad.materialdialogs.DialogBehavior" jni-type="Lcom/afollestad/materialdialogs/DialogBehavior;"/>
	  <parameter name="p2" type="int" jni-type="I"/>
	  <parameter name="p3" type="kotlin.jvm.internal.DefaultConstructorMarker" jni-type="Lkotlin/jvm/internal/DefaultConstructorMarker;"/>
	</constructor>

Additionally, the `kotlin.jvm.internal.DefaultConstructorMarker` type
is not available in the `Xamarin.Kotlin.StdLib` NuGet package, because
the type is `internal`.

Consequently, when trying to bind the constructor, `ApiXmlAdjuster`
reports this "error":

	Error while processing '[Constructor] MaterialDialog(android.content.Context p0, com.afollestad.materialdialogs.DialogBehavior p1, int p2, kotlin.jvm.internal.DefaultConstructorMarker p3)' in '[Class] com.afollestad.materialdialogs.MaterialDialog': Type 'kotlin.jvm.internal.DefaultConstructorMarker' was not found.

This is actually good, as we shouldn't bind this synthetic constructor,
but we should detect this Kotlin-ism and not emit the "error", as it
misleads users into thinking they need to do something to fix it.

Update `Xamarin.Android.Tools.Bytecode` to *hide* constructors for
which the final two parameter types are `int, DefaultConstructorMarker`,
thus ensuring that `ApiXmlAdjuster` & co. won't process such members
or attempt to resolve the unresolvable `DefaultConstructorMarker` type.

src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinFixups.cs

@@ -93,6 +93,12 @@ static void FixupJavaMethods (Methods methods)
 				method.AccessFlags = MethodAccessFlags.Private;
 			}
 
+			// Hide constructor if it's the synthetic DefaultConstructorMarker one
+			foreach (var method in methods.Where (method => method.IsDefaultConstructorMarker ())) {
+				Log.Debug ($"Kotlin: Hiding synthetic default constructor in class '{method.DeclaringType?.ThisClass.Name.Value}' with signature '{method.Descriptor}'");
+				method.AccessFlags = ((method.AccessFlags ^ MethodAccessFlags.Public) & method.AccessFlags) | MethodAccessFlags.Private;
+			}
+
 			// Better parameter names in extension methods
 			foreach (var method in methods.Where (m => m.IsPubliclyVisible && m.AccessFlags.HasFlag (MethodAccessFlags.Static)))
 				FixupExtensionMethod (method);

src/Xamarin.Android.Tools.Bytecode/Kotlin/KotlinUtilities.cs

@@ -72,6 +72,26 @@ public static string GetMethodNameWithoutSuffix (this MethodInfo method)
 			return index >= 0 ? method.Name.Substring (0, index) : method.Name;
 		}
 
+		public static bool IsDefaultConstructorMarker (this MethodInfo method)
+		{
+			// A default constructor is synthetic and always has an int and a
+			// DefaultConstructorMarker as its final 2 parameters.
+			if (method.Name != "<init>")
+				return false;
+
+			if (!method.AccessFlags.HasFlag (MethodAccessFlags.Synthetic))
+				return false;
+
+			var parameters = method.GetParameters ();
+
+			if (parameters.Length < 2)
+				return false;
+
+			// Parameter list ends with `int, DefaultConstructorMarker`.
+			return parameters [parameters.Length - 2].Type.TypeSignature == "I" &&
+				parameters [parameters.Length - 1].Type.TypeSignature == "Lkotlin/jvm/internal/DefaultConstructorMarker;";
+		}
+
 		public static bool IsPubliclyVisible (this ClassAccessFlags flags) => flags.HasFlag (ClassAccessFlags.Public) || flags.HasFlag (ClassAccessFlags.Protected);
 
 		public static bool IsPubliclyVisible (this KotlinClassVisibility flags) => flags == KotlinClassVisibility.Public || flags == KotlinClassVisibility.Protected;

tests/Xamarin.Android.Tools.Bytecode-Tests/KotlinFixupsTests.cs

@@ -51,6 +51,32 @@ public void HideInternalConstructor ()
 			Assert.False (ctor.AccessFlags.HasFlag (MethodAccessFlags.Public));
 		}
 
+		[Test]
+		public void HideDefaultConstructorMarker ()
+		{
+			var klass = LoadClassFile ("DefaultConstructor.class");
+
+			// init ()
+			var ctor_0p = klass.Methods.Single (m => m.Name == "<init>" && m.GetParameters ().Length == 0);
+
+			// init (string name)
+			var ctor_1p = klass.Methods.Single (m => m.Name == "<init>" && m.GetParameters ().Length == 1);
+
+			// init (string p0, int p1, DefaultConstructorMarker p2)
+			var ctor_3p = klass.Methods.Single (m => m.Name == "<init>" && m.GetParameters ().Length == 3);
+
+			Assert.True (ctor_3p.AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			KotlinFixups.Fixup (new [] { klass });
+
+			// Assert that the normal constructors are still public
+			Assert.True (ctor_0p.AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.True (ctor_1p.AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			// Assert that the synthetic "DefaultConstructorMarker" constructor has been marked private
+			Assert.False (ctor_3p.AccessFlags.HasFlag (MethodAccessFlags.Public));
+		}
+
 		[Test]
 		public void HideImplementationMethod ()
 		{

tests/Xamarin.Android.Tools.Bytecode-Tests/Resources/JvmOverloadsConstructor.xml

@@ -75,44 +75,6 @@
           type="boolean"
           jni-type="Z" />
       </constructor>
-      <constructor
-        deprecated="not deprecated"
-        final="false"
-        name="JvmOverloadsConstructor"
-        static="false"
-        visibility="public"
-        bridge="false"
-        synthetic="true"
-        jni-signature="(LJvmOverloadsConstructor;IILjava/lang/String;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V">
-        <parameter
-          name="p0"
-          type="JvmOverloadsConstructor"
-          jni-type="LJvmOverloadsConstructor;" />
-        <parameter
-          name="p1"
-          type="int"
-          jni-type="I" />
-        <parameter
-          name="p2"
-          type="int"
-          jni-type="I" />
-        <parameter
-          name="p3"
-          type="java.lang.String"
-          jni-type="Ljava/lang/String;" />
-        <parameter
-          name="p4"
-          type="boolean"
-          jni-type="Z" />
-        <parameter
-          name="p5"
-          type="int"
-          jni-type="I" />
-        <parameter
-          name="p6"
-          type="kotlin.jvm.internal.DefaultConstructorMarker"
-          jni-type="Lkotlin/jvm/internal/DefaultConstructorMarker;" />
-      </constructor>
       <constructor
         deprecated="not deprecated"
         final="false"

tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/DefaultConstructor.kt

@@ -0,0 +1,2 @@
+class DefaultConstructor (name: String = "bob") {
+}