[Byecode] Hide internal Kotlin fields marked with @JvmField (#1004)

Fixes: #954

Context: https://github.com/Kotlin/kotlinx.coroutines/blob/f0874d1c5ce1e5c8f091be2981bea3b80168c14e/kotlinx-coroutines-core/jvm/src/scheduling/Tasks.kt#L71-L75

When binding `org.jetbrains.kotlinx.kotlinx-coroutines-core-jvm`, the
following Kotlin-internal declarations:

	@JvmField
	internal val NonBlockingContext:    TaskContext = TaskContextImpl(TASK_NON_BLOCKING)

	@JvmField
	internal val BlockingContext:       TaskContext = TaskContextImpl(TASK_PROBABLY_BLOCKING)

are not being marked as private in the Java API.  This causes binding
errors because their type (`TaskContext`) is a Kotlin-internal
interface which is removed from the Java API.

Internal Kotlin fields that are marked with [`@JvmField`][0] are
exposed as `public` in the compiled Bytecode so they can be consumed
by Java.

	// Kotlin
	@JvmField
	internal val city: String = "London"

	// Java
	@JvmField
	@NotNull
	public final String city = "London";

This is a bit of a conundrum because Kotlin `internal` means "can't
be consumed by other libraries" while `@JvmField` means "make this
consumable by other Java libraries". 🤷‍♂️ 

Let's honor the original Kotlin `internal` visibility to produce
consistent bindings.  (The `kotlin-internal` visibility can be
overridden with `generator` metadata if desired.)

We need to examine the encoded Kotlin metadata to determine if the
field should be marked as `kotlin-internal` the same way we do with
methods and properties.

[0]: https://kotlinlang.org/docs/java-to-kotlin-interop.html#instance-fields

Author: jpobst

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

@@ -43,7 +43,7 @@ public ClassFile (Stream stream)
 			thisClass       = stream.ReadNetworkUInt16 ();
 			superClass      = stream.ReadNetworkUInt16 ();
 			Interfaces      = new Interfaces (ConstantPool, stream);
-			Fields          = new Fields (ConstantPool, stream);
+			Fields          = new Fields (ConstantPool, this, stream);
 			Methods         = new Methods (ConstantPool, this, stream);
 			Attributes      = new AttributeCollection (ConstantPool, stream);
 

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

@@ -10,7 +10,7 @@ public sealed class Fields : Collection<FieldInfo> {
 
 		public  ConstantPool    ConstantPool        {get; private set;}
 
-		public Fields (ConstantPool constantPool, Stream stream)
+		public Fields (ConstantPool constantPool, ClassFile declaringClass, Stream stream)
 		{
 			if (constantPool == null)
 				throw new ArgumentNullException ("constantPool");
@@ -20,7 +20,7 @@ public Fields (ConstantPool constantPool, Stream stream)
 			ConstantPool    = constantPool;
 			var count       = stream.ReadNetworkUInt16 ();
 			for (int i = 0; i < count; ++i) {
-				Add (new FieldInfo (constantPool, stream));
+				Add (new FieldInfo (constantPool, declaringClass, stream));
 			}
 		}
 	}
@@ -31,13 +31,15 @@ public sealed class FieldInfo {
 		ushort              descriptorIndex;
 
 		public  ConstantPool        ConstantPool    {get; private set;}
-		public  FieldAccessFlags    AccessFlags     {get; private set;}
+		public  ClassFile           DeclaringType   {get; private set;}
+		public  FieldAccessFlags    AccessFlags     {get; set;}
 		public  AttributeCollection Attributes      {get; private set;}
 		public  string?             KotlinType      {get; set;}
 
-		public FieldInfo (ConstantPool constantPool, Stream stream)
+		public FieldInfo (ConstantPool constantPool, ClassFile declaringType, Stream stream)
 		{
 			ConstantPool    = constantPool;
+			DeclaringType   = declaringType;
 			AccessFlags     = (FieldAccessFlags) stream.ReadNetworkUInt16 ();
 			nameIndex       = stream.ReadNetworkUInt16 ();
 			descriptorIndex = stream.ReadNetworkUInt16 ();
@@ -61,6 +63,8 @@ public  string Descriptor {
 			var signature   = Attributes.Get<SignatureAttribute> ();
 			return signature != null ? signature.Value : null;
 		}
+
+		public bool IsPubliclyVisible => AccessFlags.HasFlag (FieldAccessFlags.Public) || AccessFlags.HasFlag (FieldAccessFlags.Protected);
 	}
 
 	[Flags]
@@ -74,5 +78,8 @@ public enum FieldAccessFlags {
 		Transient   = 0x0080,
 		Synthetic   = 0x1000,
 		Enum        = 0x4000,
+
+		// This is not a real Java FieldAccessFlags, it is used to denote Kotlin "internal" access.
+		Internal    = 0x10000000,
 	}
 }

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

@@ -129,6 +129,20 @@ static MethodAccessFlags SetVisibility (MethodAccessFlags existing, MethodAccess
 			return existing;
 		}
 
+		static FieldAccessFlags SetVisibility (FieldAccessFlags existing, FieldAccessFlags newVisibility)
+		{
+			// First we need to remove any existing visibility flags,
+			// without modifying other flags like Abstract
+			existing = (existing ^ FieldAccessFlags.Public) & existing;
+			existing = (existing ^ FieldAccessFlags.Protected) & existing;
+			existing = (existing ^ FieldAccessFlags.Private) & existing;
+			existing = (existing ^ FieldAccessFlags.Internal) & existing;
+
+			existing |= newVisibility;
+
+			return existing;
+		}
+
 		static void FixupJavaMethods (Methods methods)
 		{
 			// We do the following method level fixups here because we can operate on all methods,
@@ -294,6 +308,15 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 			if (field is null)
 				return;
 
+			// Hide field if it isn't Public/Protected
+			if (!metadata.Flags.IsPubliclyVisible ()) {
+
+				if (field.IsPubliclyVisible) {
+					Log.Debug ($"Kotlin: Hiding internal field {field.DeclaringType?.ThisClass.Name.Value} - {field.Name}");
+					field.AccessFlags = SetVisibility (field.AccessFlags, FieldAccessFlags.Internal);
+				}
+			}
+
 			// Handle erasure of Kotlin unsigned types
 			field.KotlinType = GetKotlinType (field.Descriptor, metadata.ReturnType?.ClassName);
 		}

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

@@ -593,6 +593,8 @@ static string EscapeLiteral (string value)
 
 		static string GetVisibility (FieldAccessFlags accessFlags)
 		{
+			if (accessFlags.HasFlag (FieldAccessFlags.Internal))
+				return "kotlin-internal";
 			if ((accessFlags & FieldAccessFlags.Public) != 0)
 				return "public";
 			if ((accessFlags & FieldAccessFlags.Protected) != 0)

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

@@ -135,6 +135,22 @@ public void HideInternalMethod ()
 			Assert.True (output.Contains ("visibility=\"kotlin-internal\""));
 		}
 
+		[Test]
+		public void HideInternalField ()
+		{
+			var klass = LoadClassFile ("InternalField.class");
+			var field = klass.Fields.First (m => m.Name == "city");
+
+			Assert.True (field.AccessFlags.HasFlag (FieldAccessFlags.Public));
+
+			KotlinFixups.Fixup (new [] { klass });
+
+			Assert.False (field.AccessFlags.HasFlag (FieldAccessFlags.Public));
+
+			var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString ();
+			Assert.True (output.Contains ("visibility=\"kotlin-internal\""));
+		}
+
 		[Test]
 		public void ParameterName ()
 		{

tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/InternalField.class

@@ -0,0 +1,4 @@
+class InternalField {
+	@JvmField
+	internal val city: String = "London"
+}