[Xamarin.Android.Tools.Bytecode] hide Kotlin internal nested types (#827

)

Fixes: #826

Context: #682

Given the following Kotlin code:

	internal class MyClass {
	  public interface MyInterface { }
	}

The default Java representation of this would be:

	public MyClass {
	  public MyInterface { }
	}

In order to prevent this from being bound, our Kotlin fixups
attempted to change the Java representation to:

	private class MyClass {
	  private interface MyInterface { }
	}

However there was a bug preventing the internal interface from being
marked as `private`, because we are setting the visibility on the
parent type's `InnerClassAccessFlags`, *not* the nested type itself.
When we output the XML we use use the declaring class'
`InnerClassAccessFlags`, we instead look at the flags on the nested
type's `.class` file, which was still `public`:

	<class name="MyClass" visibility="private" … />
	<class name="MyClass.MyInterface" visibility="public" … />

This would result in a `generator` warning when trying to bind
`MyClass.MyInterface`, as the parent `MyClass` type is skipped:

	warning BG8604: top ancestor MyClass not found for nested type MyClass.MyInterface

Fix this by finding the appropriate inner class `.class` file and
updating the access flags there, recursively:

	<class name="MyClass" visibility="private" … />
	<class name="MyClass.MyInterface" visibility="private" … />

Note: the [`InnerClasses` Attribute][0] for an inner class may
contain the declaring class.  For example, the `InnerClasses` for
`InternalClassWithNestedInterface$NestedInterface` contains
`InternalClassWithNestedInterface$NestedInterface`.

We must thus protect recursive `HideInternalInnerClass()` invocations
from processing the current type, to avoid infinite recursion.

[0]: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.6

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

@@ -30,7 +30,7 @@ public static void Fixup (IList<ClassFile> classes)
 					var class_metadata = (metadata as KotlinClass);
 
 					if (class_metadata != null) {
-						FixupClassVisibility (c, class_metadata);
+						FixupClassVisibility (c, class_metadata, classes);
 
 						if (!c.AccessFlags.IsPubliclyVisible ())
 							continue;
@@ -62,7 +62,7 @@ public static void Fixup (IList<ClassFile> classes)
 			}
 		}
 
-		static void FixupClassVisibility (ClassFile klass, KotlinClass metadata)
+		static void FixupClassVisibility (ClassFile klass, KotlinClass metadata, IList<ClassFile> classes)
 		{
 			// Hide class if it isn't Public/Protected
 			if (klass.AccessFlags.IsPubliclyVisible () && !metadata.Visibility.IsPubliclyVisible ()) {
@@ -83,15 +83,33 @@ static void FixupClassVisibility (ClassFile klass, KotlinClass metadata)
 				Log.Debug ($"Kotlin: Hiding internal class {klass.ThisClass.Name.Value}");
 				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 = SetVisibility (ic.InnerClassAccessFlags, ClassAccessFlags.Private);
-				}
+				foreach (var ic in klass.InnerClasses)
+					HideInternalInnerClass (ic, classes);
 
 				return;
 			}
 		}
 
+		static void HideInternalInnerClass (InnerClassInfo klass, IList<ClassFile> classes)
+		{
+			var existing = klass.InnerClassAccessFlags;
+
+			klass.InnerClassAccessFlags = SetVisibility (existing, ClassAccessFlags.Private);
+			Log.Debug ($"Kotlin: Hiding nested internal type {klass.InnerClass.Name.Value}");
+
+			// Setting the inner class access flags above doesn't technically do anything, because we output
+			// from the inner class's ClassFile, so we need to find that and set it there.
+			var kf = classes.FirstOrDefault (c => c.ThisClass.Name.Value == klass.InnerClass.Name.Value);
+
+			if (kf != null) {
+				kf.AccessFlags = SetVisibility (kf.AccessFlags, ClassAccessFlags.Private);
+				kf.InnerClass.InnerClassAccessFlags = SetVisibility (kf.InnerClass.InnerClassAccessFlags, ClassAccessFlags.Private);
+
+				foreach (var inner_class in kf.InnerClasses.Where (c => c.OuterClass.Name.Value == kf.ThisClass.Name.Value))
+					HideInternalInnerClass (inner_class, classes);
+			}
+		}
+
 		// Passing null for 'newVisibility' parameter means 'package-private'
 		static ClassAccessFlags SetVisibility (ClassAccessFlags existing, ClassAccessFlags? newVisibility)
 		{

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

@@ -310,5 +310,24 @@ public void HandleKotlinNameShadowing ()
 			Assert.True (klass.Methods.Single (m => m.Name == "hitCount").AccessFlags.HasFlag (MethodAccessFlags.Public));
 			Assert.True (klass.Methods.Single (m => m.Name == "setType").AccessFlags.HasFlag (MethodAccessFlags.Public));
 		}
+
+		[Test]
+		public void HidePublicInterfaceInInternalClass ()
+		{
+			var klass = LoadClassFile ("InternalClassWithNestedInterface.class");
+			var inner_interface = LoadClassFile ("InternalClassWithNestedInterface$NestedInterface.class");
+			var inner_inner_interface = LoadClassFile ("InternalClassWithNestedInterface$NestedInterface$DoubleNestedInterface.class");
+
+			KotlinFixups.Fixup (new [] { klass, inner_interface, inner_inner_interface });
+
+			var output = new XmlClassDeclarationBuilder (klass).ToXElement ().ToString ();
+			Assert.True (output.Contains ("visibility=\"public\""));
+
+			var output2 = new XmlClassDeclarationBuilder (inner_interface).ToXElement ().ToString ();
+			Assert.True (output2.Contains ("visibility=\"private\""));
+
+			var output3 = new XmlClassDeclarationBuilder (inner_inner_interface).ToXElement ().ToString ();
+			Assert.True (output3.Contains ("visibility=\"private\""));
+		}
 	}
 }

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

@@ -0,0 +1,5 @@
+internal class InternalClassWithNestedInterface {
+public interface NestedInterface {
+public interface DoubleNestedInterface
+}
+}