[class-parse, generator] Allow showing Kotlin internals via metadata (#…

…793)

Fixes: #790

Kotlin compiled for Java Bytecode is an interesting beast, as it has
features which aren't directly supported by Java bytecode.

One such feature is visibility: Kotlin supports an [`internal`][0]
visibility on types and members:

	internal class Example

Java doesn't have a direct equivalent to `internal`; instead,
Java Bytecode uses a visibility of *`public`*:

	% javap Example.class 
	public final class Example {
	  public Example();
	}

Commit 439bd83 added support to `Xamarin.Android.Tools.Bytecode.dll`
for parsing Kotlin metadata.  The result is that Kotlin `internal`
are marked as *`private`*, which causes them to be *skipped* and not
present within `api.xml`, because `class-parse`
[doesn't write out `private` members][1].

The result was that `Metadata.xml` could not be used to mark Kotlin
`internal` types as `public`, as they didn't exist within `api.xml`,
and thus couldn't serve as a "target" for `Metadata.xml`.

Improve support for using `Metadata.xml` to update Kotlin `internal`
types by making the following changes:

  * `XmlClassDeclarationBuilder` now emits *all* types, even
    `private` types.  This includes Kotlin `internal` types which
    were changed to have `private` visibility.

  * Kotlin `internal` members are emitted into `api.xml` with a new
    `//*/@visibility` value of `kotlin-internal`.

These changes allow the Kotlin declaration:

	internal class Example {
	    public fun pubFun() {}
	    internal fun intFun() {}
	}

to be emitted into `api.xml` a'la:

	<class name="Example" visibility="private" …>
	  <method name="pubFun" visibility="public" …/>
	  <method name="intFun$main" visibility="kotlin-internal" …/>
	</class>

[0]: https://kotlinlang.org/docs/visibility-modifiers.html#modules
[0]: https://github.com/xamarin/java.interop/blob/b46598a254c20060b107312564e0ec0aee9e33d6/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs#L32-L33

src/Xamarin.Android.Tools.Bytecode/ClassFile.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;

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

@@ -70,36 +70,56 @@ static void FixupClassVisibility (ClassFile klass, KotlinClass metadata)
 				// Interfaces should be set to "package-private"
 				if (klass.AccessFlags.HasFlag (ClassAccessFlags.Interface)) {
 					Log.Debug ($"Kotlin: Setting internal interface {klass.ThisClass.Name.Value} to package-private");
-					klass.AccessFlags = SetPackagePrivate (klass.AccessFlags);
+					klass.AccessFlags = SetVisibility (klass.AccessFlags, null);
 
 					foreach (var ic in klass.InnerClasses) {
 						Log.Debug ($"Kotlin: Setting nested type {ic.InnerClass.Name.Value} in an internal interface to package-private");
-						ic.InnerClassAccessFlags = SetPackagePrivate (ic.InnerClassAccessFlags);
+						ic.InnerClassAccessFlags = SetVisibility (ic.InnerClassAccessFlags, null);
 					}
 
 					return;
 				}
 
 				Log.Debug ($"Kotlin: Hiding internal class {klass.ThisClass.Name.Value}");
-				klass.AccessFlags = ClassAccessFlags.Private;
+				klass.AccessFlags = SetVisibility (klass.AccessFlags, ClassAccessFlags.Private);
 
 				foreach (var ic in klass.InnerClasses) {
 					Log.Debug ($"Kotlin: Hiding nested internal type {ic.InnerClass.Name.Value}");
-					ic.InnerClassAccessFlags = ClassAccessFlags.Private;
+					ic.InnerClassAccessFlags = SetVisibility (ic.InnerClassAccessFlags, ClassAccessFlags.Private);
 				}
 
 				return;
 			}
 		}
 
-		static ClassAccessFlags SetPackagePrivate (ClassAccessFlags flags)
+		// Passing null for 'newVisibility' parameter means 'package-private'
+		static ClassAccessFlags SetVisibility (ClassAccessFlags existing, ClassAccessFlags? newVisibility)
 		{
-			// Package-private is stored as "no visibility flags"
-			flags = (flags ^ ClassAccessFlags.Public) & flags;
-			flags = (flags ^ ClassAccessFlags.Protected) & flags;
-			flags = (flags ^ ClassAccessFlags.Private) & flags;
+			// First we need to remove any existing visibility flags,
+			// without modifying other flags like Abstract
+			existing = (existing ^ ClassAccessFlags.Public) & existing;
+			existing = (existing ^ ClassAccessFlags.Protected) & existing;
+			existing = (existing ^ ClassAccessFlags.Private) & existing;
 
-			return flags;
+			// Package-private is stored as "no visibility flags", so only add flag if specified
+			if (newVisibility.HasValue)
+				existing |= newVisibility.Value;
+
+			return existing;
+		}
+
+		static MethodAccessFlags SetVisibility (MethodAccessFlags existing, MethodAccessFlags newVisibility)
+		{
+			// First we need to remove any existing visibility flags,
+			// without modifying other flags like Abstract
+			existing = (existing ^ MethodAccessFlags.Public) & existing;
+			existing = (existing ^ MethodAccessFlags.Protected) & existing;
+			existing = (existing ^ MethodAccessFlags.Private) & existing;
+			existing = (existing ^ MethodAccessFlags.Internal) & existing;
+
+			existing |= newVisibility;
+
+			return existing;
 		}
 
 		static void FixupJavaMethods (Methods methods)
@@ -132,7 +152,7 @@ static void FixupConstructor (MethodInfo method, KotlinConstructor metadata)
 			// Hide constructor if it isn't Public/Protected
 			if (method.IsPubliclyVisible && !metadata.Flags.IsPubliclyVisible ()) {
 				Log.Debug ($"Kotlin: Hiding internal constructor {method.DeclaringType?.ThisClass.Name.Value} - {metadata.GetSignature ()}");
-				method.AccessFlags = MethodAccessFlags.Private;
+				method.AccessFlags = SetVisibility (method.AccessFlags, MethodAccessFlags.Internal);
 			}
 		}
 
@@ -144,7 +164,7 @@ static void FixupFunction (MethodInfo method, KotlinFunction metadata, KotlinCla
 			// Hide function if it isn't Public/Protected
 			if (!metadata.Flags.IsPubliclyVisible ()) {
 				Log.Debug ($"Kotlin: Hiding internal method {method.DeclaringType?.ThisClass.Name.Value} - {metadata.GetSignature ()}");
-				method.AccessFlags = MethodAccessFlags.Private;
+				method.AccessFlags = SetVisibility (method.AccessFlags, MethodAccessFlags.Internal);
 				return;
 			}
 
@@ -190,12 +210,12 @@ static void FixupProperty (MethodInfo getter, MethodInfo setter, KotlinProperty
 
 				if (getter?.IsPubliclyVisible == true) {
 					Log.Debug ($"Kotlin: Hiding internal getter method {getter.DeclaringType?.ThisClass.Name.Value} - {getter.Name}");
-					getter.AccessFlags = MethodAccessFlags.Private;
+					getter.AccessFlags = SetVisibility (getter.AccessFlags, MethodAccessFlags.Internal);
 				}
 
 				if (setter?.IsPubliclyVisible == true) {
 					Log.Debug ($"Kotlin: Hiding internal setter method {setter.DeclaringType?.ThisClass.Name.Value} - {setter.Name}");
-					setter.AccessFlags = MethodAccessFlags.Private;
+					setter.AccessFlags = SetVisibility (setter.AccessFlags, MethodAccessFlags.Internal);
 				}
 
 				return;

src/Xamarin.Android.Tools.Bytecode/Methods.cs

@@ -352,6 +352,9 @@ public enum MethodAccessFlags {
 		Abstract        = 0x0400,
 		Strict          = 0x0800,
 		Synthetic       = 0x1000,
+
+		// This is not a real Java MethodAccessFlags, it is used to denote Kotlin "internal" access.
+		Internal        = 0x10000000,
 	}
 
 	[Flags]

src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Globalization;
@@ -28,9 +28,6 @@ public XmlClassDeclarationBuilder (ClassFile classFile)
 
 		public XElement ToXElement ()
 		{
-			var visibility = GetClassVisibility (this.classFile.AccessFlags);
-			if (visibility == "private")
-				return null;
 			return new XElement (GetElementName (),
 					new XAttribute ("abstract",                 (classFile.AccessFlags & ClassAccessFlags.Abstract) != 0),
 					new XAttribute ("deprecated",               GetDeprecatedValue (classFile.Attributes)),
@@ -319,7 +316,7 @@ IEnumerable<XElement> GetImplementedInterfaces ()
 		IEnumerable<XElement> GetConstructors ()
 		{
 			return classFile.Methods.Where (m => m.Name == "<init>" 
-					&& (GetMethodVisibility(m.AccessFlags) == "public" || GetMethodVisibility(m.AccessFlags) == "protected"))
+					&& (GetMethodVisibility(m.AccessFlags) == "public" || GetMethodVisibility(m.AccessFlags) == "protected" || GetMethodVisibility (m.AccessFlags) == "kotlin-internal"))
 				.OrderBy (m => m.Name + m.Descriptor, StringComparer.OrdinalIgnoreCase)
 				.Select (c => GetMethod ("constructor", GetThisClassName (), c, null));
 		}
@@ -374,6 +371,8 @@ static XAttribute GetSynchronized (MethodInfo method)
 
 		static string GetVisibility (MethodAccessFlags accessFlags)
 		{
+			if (accessFlags.HasFlag (MethodAccessFlags.Internal))
+				return "kotlin-internal";
 			if ((accessFlags & MethodAccessFlags.Public) != 0)
 				return "public";
 			if ((accessFlags & MethodAccessFlags.Protected) != 0)
@@ -605,6 +604,8 @@ static string GetVisibility (FieldAccessFlags accessFlags)
 
 		static string GetMethodVisibility (MethodAccessFlags accessFlags)
 		{
+			if (accessFlags.HasFlag (MethodAccessFlags.Internal))
+				return "kotlin-internal";
 			if ((accessFlags & MethodAccessFlags.Public) != 0)
 				return "public";
 			if ((accessFlags & MethodAccessFlags.Protected) != 0)
@@ -628,7 +629,7 @@ static string GetClassVisibility (ClassAccessFlags accessFlags)
 		IEnumerable<XElement> GetMethods ()
 		{
 			return classFile.Methods.Where (m => !m.Name.StartsWith ("<", StringComparison.OrdinalIgnoreCase)
-						&& (GetMethodVisibility(m.AccessFlags) == "public" || GetMethodVisibility(m.AccessFlags) == "protected"))
+						&& (GetMethodVisibility(m.AccessFlags) == "public" || GetMethodVisibility(m.AccessFlags) == "protected" || GetMethodVisibility (m.AccessFlags) == "kotlin-internal"))
 				.OrderBy (m => m.Name + m.Descriptor, StringComparer.OrdinalIgnoreCase)
 				.Select (m => GetMethod ("method", m.Name, m,
 					returns: m.ReturnType.TypeSignature));

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

@@ -24,6 +24,9 @@ public void HideInternalClass ()
 
 			Assert.False (klass.AccessFlags.HasFlag (ClassAccessFlags.Public));
 			Assert.False (inner_class.InnerClassAccessFlags.HasFlag (ClassAccessFlags.Public));
+
+			var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString ();
+			Assert.True (output.Contains ("visibility=\"private\""));
 		}
 
 		[Test]
@@ -58,6 +61,9 @@ public void HideInternalConstructor ()
 			KotlinFixups.Fixup (new [] { klass });
 
 			Assert.False (ctor.AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString ();
+			Assert.True (output.Contains ("visibility=\"kotlin-internal\""));
 		}
 
 		[Test]
@@ -124,6 +130,9 @@ public void HideInternalMethod ()
 			KotlinFixups.Fixup (new [] { klass });
 
 			Assert.False (method.AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString ();
+			Assert.True (output.Contains ("visibility=\"kotlin-internal\""));
 		}
 
 		[Test]
@@ -154,6 +163,9 @@ public void HideInternalProperty ()
 
 			Assert.False (getter.AccessFlags.HasFlag (MethodAccessFlags.Public));
 			Assert.False (setter.AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString ();
+			Assert.True (output.Contains ("visibility=\"kotlin-internal\""));
 		}
 
 		[Test]

tests/generator-Tests/Unit-Tests/XmlApiImporterTests.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Linq;
 using System.Xml.Linq;
 using MonoDroid.Generation;
@@ -244,5 +245,26 @@ public void PreserveSourceLineInfo ()
 			Assert.AreEqual (2, method.LinePosition);
 			Assert.AreEqual ("obj/Debug/api.xml", method.SourceFile);
 		}
+
+		[Test]
+		public void IgnoreKotlinInternalMembers ()
+		{
+			var xml = XDocument.Parse (@"
+				<api>
+					<package name='com.example.test' jni-name='com/example/test'>
+						<class name='Test' visibility='public'>
+							<constructor name='Test' visibility='kotlin-internal' />
+							<method name='DoStuff' visibility='kotlin-internal' />
+							<field name='MyField' visibility='kotlin-internal' />
+						</class>
+					</package>
+				</api>");
+
+			var gens = new Parser (opt).Parse (xml, new List<string> (), "0", 0);
+			var klass = gens.Single ();
+
+			Assert.AreEqual (0, klass.Fields.Count);
+			Assert.AreEqual (0, klass.Methods.Count);
+		}
 	}
 }

tools/generator/Java.Interop.Tools.Generator.Importers/XmlApiImporter.cs

@@ -37,13 +37,16 @@ public static ClassGen CreateClass (XElement pkg, XElement elem, CodeGenerationO
 						klass.AddImplementedInterface (iname);
 						break;
 					case "method":
-						klass.AddMethod (CreateMethod (klass, child, options));
+						if (child.XGetAttribute ("visibility") != "kotlin-internal")
+							klass.AddMethod (CreateMethod (klass, child, options));
 						break;
 					case "constructor":
-						klass.Ctors.Add (CreateCtor (klass, child, options));
+						if (child.XGetAttribute ("visibility") != "kotlin-internal")
+							klass.Ctors.Add (CreateCtor (klass, child, options));
 						break;
 					case "field":
-						klass.AddField (CreateField (klass, child, options));
+						if (child.XGetAttribute ("visibility") != "kotlin-internal")
+							klass.AddField (CreateField (klass, child, options));
 						break;
 					case "typeParameters":
 						break; // handled at GenBaseSupport
@@ -230,10 +233,12 @@ public static InterfaceGen CreateInterface (XElement pkg, XElement elem, CodeGen
 						iface.AddImplementedInterface (iname);
 						break;
 					case "method":
-						iface.AddMethod (CreateMethod (iface, child, options));
+						if (child.XGetAttribute ("visibility") != "kotlin-internal")
+							iface.AddMethod (CreateMethod (iface, child, options));
 						break;
 					case "field":
-						iface.AddField (CreateField (iface, child, options));
+						if (child.XGetAttribute ("visibility") != "kotlin-internal")
+							iface.AddField (CreateField (iface, child, options));
 						break;
 					case "typeParameters":
 						break; // handled at GenBaseSupport