[Xamarin.Android.Tools.Bytecode] Kotlin internal prop visibility (#1151)

Fixes: #1139

Context: https://jetbrains.gitbooks.io/kotlin-reference-for-kindle/content/properties.html

Kotlin does not allow classes to explicitly contain fields:

> Classes in Kotlin cannot have fields.

They can *implicitly* contain fields, but not explicitly.
Syntax that *look like* a field to those who don't know Kotlin:

	/* partial */ class EmojiPickerView {
	    private var onEmojiPickedListener: Consumer<EmojiViewItem>? = null
	}

are in fact *properties*, which may or may not involve a compiler-
generated backing field.

When a property is declared in Kotlin, Java `get*` and/or `set*`
methods are generated as needed.  In the case where the property is
`internal` -- which is not supported by Java -- `public` getters or
setters are generated.  We wish to "hide" these as they are not
intended to be part of the public API.  That is, they would not be
callable by Kotlin code.

However, consider these code fragments from [`EmojiPickerView.kt`][0]:

	/* partial */ class EmojiPickerView {
	    // Line 100
	    private var onEmojiPickedListener: Consumer<EmojiViewItem>? = null

	    // Lines 317-319
	    fun setOnEmojiPickedListener(onEmojiPickedListener: Consumer<EmojiViewItem>?) {
	        this.onEmojiPickedListener = onEmojiPickedListener
	    }
	}

Default member visibility in Kotlin is `public`, so this declares a
private `onEmojiPickedListener` property and a public
`setOnEmojiPickedListener()` method.

However, we were incorrectly determining visibility (678c4bd): we
treated `onEmojiPickedListener` and `setOnEmojiPickedListener()` as
if they were part of the same property.  As part of this association,
we saw that `onEmojiPickedListener` was private, and marked
`setOnEmojiPickedListener()` as private to follow suit.

This was incorrect, because `private` properties do not have setters
*at all*, so we should not attempt to hide anything for `private`
properties, only for `internal` properties.

With that incorrect association broken, that allows
`setOnEmojiPickedListener()` to be considered separately, and found
to have `public` visibility.

For example, this Kotlin code:

	private var type = 0
	internal var itype = 0

generates Java code equivalent to:

	private int type;
	private int itype;

	public final int getItype$main() {
	    return this.itype;
	}
	
	public final void setItype$main(int <set-?>) {
	this.itype = <set-?>;

Additionally, when matching `internal` properties to their getters
or setters, the generated name differs from the names given to
`public` getters and setters.

	// Kotlin: public var type = 0
	// Java:
	public final int getType () { ... }

	// Kotlin: internal var type = 0
	// Java:
	public final int getType$main () { ... }

Fix this scenario:

  * Do not attempt to hide getters/setters for `private` Kotlin
    properties.

  * Improve matching of generated names for `internal` Kotlin
    properties.

Additionally, our existing unit test in `NameShadowing.kt` was written
incorrectly:

	// Incorrect, return type of a function
	fun setType(type: Int) = { println (type); }

	// Correct, return type of void
	fun setType(type: Int) { println (type); }

The fixed `NameShadowing.kt` unit test fails without the other
changes here.  Additionally, add several more unit test cases to
cover `internal` properties and mangled getter/setter names.

Additionally, some enum values in `KotlinPropertyFlags` were
specified incorrectly which has been fixed.

[0]: https://github.com/androidx/androidx/blob/0d655214d339e006f4e13a85f55c78770c885f2e/emoji2/emoji2-emojipicker/src/main/java/androidx/emoji2/emojipicker/EmojiPickerView.kt

Author: jpobst

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

@@ -318,6 +318,10 @@ public class KotlinProperty : KotlinMethodBase
 		}
 
 		public override string? ToString () => Name;
+
+		public bool IsInternalVisibility => (Flags & KotlinPropertyFlags.VisibilityMask) == KotlinPropertyFlags.Internal;
+
+		public bool IsPrivateVisibility => (Flags & KotlinPropertyFlags.VisibilityMask) == KotlinPropertyFlags.Private;
 	}
 
 	public class KotlinType
@@ -688,6 +692,8 @@ public enum KotlinPropertyFlags
 		PrivateToThis =		0b00_00_100_0,
 		Local =			0b00_00_101_0,
 
+		VisibilityMask =        0b00_00_111_0,
+
 		Final =			0b00_00_000_0,
 		Open =			0b00_01_000_0,
 		Abstract =		0b00_10_000_0,
@@ -698,15 +704,15 @@ public enum KotlinPropertyFlags
 		Delegation =		0b10_00_000_0,
 		Synthesized =		0b11_00_000_0,
 
-		IsVar =			0b_000000001_000_00_000_0,
-		HasGetter =		0b_000000010_000_00_000_0,
-		HasSetter =		0b_000000100_000_00_000_0,
-		IsConst =		0b_000001000_000_00_000_0,
-		IsLateInit =		0b_000010000_000_00_000_0,
-		HasConstant =		0b_000100000_000_00_000_0,
-		IsExternalProperty =	0b_001000000_000_00_000_0,
-		IsDelegated =		0b_010000000_000_00_000_0,
-		IsExpectProperty =	0b_100000000_000_00_000_0
+		IsVar =			0b_000000001_00_00_000_0,
+		HasGetter =		0b_000000010_00_00_000_0,
+		HasSetter =		0b_000000100_00_00_000_0,
+		IsConst =		0b_000001000_00_00_000_0,
+		IsLateInit =		0b_000010000_00_00_000_0,
+		HasConstant =		0b_000100000_00_00_000_0,
+		IsExternalProperty =	0b_001000000_00_00_000_0,
+		IsDelegated =		0b_010000000_00_00_000_0,
+		IsExpectProperty =	0b_100000000_00_00_000_0
 	}
 
 	[Flags]

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

@@ -272,8 +272,8 @@ static void FixupProperty (MethodInfo? getter, MethodInfo? setter, KotlinPropert
 			if (getter is null && setter is null)
 				return;
 
-			// Hide property if it isn't Public/Protected
-			if (!metadata.Flags.IsPubliclyVisible ()) {
+			// Hide getters/setters if property is Internal
+			if (metadata.IsInternalVisibility) {
 
 				if (getter?.IsPubliclyVisible == true) {
 					Log.Debug ($"Kotlin: Hiding internal getter method {getter.DeclaringType?.ThisClass.Name.Value} - {getter.Name}");
@@ -384,7 +384,17 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 
 		static MethodInfo? FindJavaPropertyGetter (KotlinFile kotlinClass, KotlinProperty property, ClassFile klass)
 		{
-			var possible_methods = klass.Methods.Where (method => string.Compare (method.GetMethodNameWithoutSuffix (), $"get{property.Name}", StringComparison.OrdinalIgnoreCase) == 0 &&
+			// Private properties do not have getters
+			if (property.IsPrivateVisibility)
+				return null;
+
+			// Public/protected getters look like "getFoo"
+			// Internal getters look like "getFoo$main"
+			var possible_methods = property.IsInternalVisibility ?
+				klass.Methods.Where (method => method.Name.StartsWith ($"get{property.Name.Capitalize ()}$", StringComparison.Ordinal)) :
+				klass.Methods.Where (method => method.Name.Equals ($"get{property.Name.Capitalize ()}", StringComparison.Ordinal));
+
+			possible_methods = possible_methods.Where (method =>
 					method.GetParameters ().Length == 0 &&
 					property.ReturnType != null &&
 					TypesMatch (method.ReturnType, property.ReturnType, kotlinClass));
@@ -394,11 +404,21 @@ static void FixupField (FieldInfo? field, KotlinProperty metadata)
 
 		static MethodInfo? FindJavaPropertySetter (KotlinFile kotlinClass, KotlinProperty property, ClassFile klass)
 		{
-			var possible_methods = klass.Methods.Where (method => string.Compare (method.GetMethodNameWithoutSuffix (), $"set{property.Name}", StringComparison.OrdinalIgnoreCase) == 0 &&
-									      property.ReturnType != null &&
-									      method.GetParameters ().Length == 1 &&
-									      method.ReturnType.BinaryName == "V" &&
-									      TypesMatch (method.GetParameters () [0].Type, property.ReturnType, kotlinClass));
+			// Private properties do not have setters
+			if (property.IsPrivateVisibility)
+				return null;
+
+			// Public/protected setters look like "setFoo"
+			// Internal setters look like "setFoo$main"
+			var possible_methods = property.IsInternalVisibility ?
+				klass.Methods.Where (method => method.Name.StartsWith ($"set{property.Name.Capitalize ()}$", StringComparison.Ordinal)) :
+				klass.Methods.Where (method => method.Name.Equals ($"set{property.Name.Capitalize ()}", StringComparison.Ordinal));
+
+			possible_methods = possible_methods.Where (method => 
+						property.ReturnType != null &&
+						method.GetParameters ().Length == 1 &&
+						method.ReturnType.BinaryName == "V" &&
+						TypesMatch (method.GetParameters () [0].Type, property.ReturnType, kotlinClass));
 
 			return possible_methods.FirstOrDefault ();
 		}

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

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Text;
 using System.Threading.Tasks;
@@ -8,6 +9,18 @@ namespace Xamarin.Android.Tools.Bytecode
 {
 	public static class KotlinUtilities
 	{
+		[return: NotNullIfNotNull (nameof (value))]
+		public static string? Capitalize (this string? value)
+		{
+			if (string.IsNullOrWhiteSpace (value))
+				return value;
+
+			if (value.Length < 1)
+				return value;
+
+			return char.ToUpperInvariant (value [0]) + value.Substring (1);
+		}
+
 		public static string ConvertKotlinTypeSignature (KotlinType? type, KotlinFile? metadata = null, bool convertUnsignedToPrimitive = true)
 		{
 			if (type is null)

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

@@ -324,7 +324,29 @@ public void HandleKotlinNameShadowing ()
 
 			Assert.True (klass.Methods.Single (m => m.Name == "count").AccessFlags.HasFlag (MethodAccessFlags.Public));
 			Assert.True (klass.Methods.Single (m => m.Name == "hitCount").AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			// Private property and explicit getter/setter
+			// There is no generated getter/setter, and explicit ones should be public
+			Assert.True (klass.Methods.Single (m => m.Name == "getType").AccessFlags.HasFlag (MethodAccessFlags.Public));
 			Assert.True (klass.Methods.Single (m => m.Name == "setType").AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			// Private immutable property and explicit getter/setter
+			// There is no generated getter/setter, and explicit ones should be public
+			Assert.True (klass.Methods.Single (m => m.Name == "getType2").AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.True (klass.Methods.Single (m => m.Name == "setType2").AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			// Internal property and explicit getter/setter
+			// Generated getter/setter should not be public, and explicit ones should be public
+			Assert.False (klass.Methods.Single (m => m.Name == "getItype$main").AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.False (klass.Methods.Single (m => m.Name == "setItype$main").AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.True (klass.Methods.Single (m => m.Name == "getItype").AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.True (klass.Methods.Single (m => m.Name == "setItype").AccessFlags.HasFlag (MethodAccessFlags.Public));
+
+			// Internal immutable property and explicit getter/setter
+			// Generated getter should not be public, and explicit ones should be public
+			Assert.False (klass.Methods.Single (m => m.Name == "getItype2$main").AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.True (klass.Methods.Single (m => m.Name == "getItype2").AccessFlags.HasFlag (MethodAccessFlags.Public));
+			Assert.True (klass.Methods.Single (m => m.Name == "setItype2").AccessFlags.HasFlag (MethodAccessFlags.Public));
 		}
 
 		[Test]

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

@@ -8,7 +8,23 @@ public class NameShadowing {
 	private var hitCount = 0
 	fun hitCount(): Int = hitCount
 
-	// Property and setter
+	// Private property and explicit getter/setter
 	private var type = 0
-	fun setType(type: Int) = { println (type); }
+	fun getType(): Int = type
+	fun setType(type: Int) { }
+
+	// Private immutable property and explicit getter/setter
+	private val type2 = 0
+	fun getType2(): Int = type2
+	fun setType2(type: Int) { }
+
+	// Internal property and explicit getter/setter
+	internal var itype = 0
+	fun getItype(): Int = itype
+	fun setItype(type: Int) { }
+
+	// Internal immutable property and explicit getter/setter
+	internal val itype2 = 0
+	fun getItype2(): Int = itype2
+	fun setItype2(type: Int) { }
 }