[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
 }
 
 publicoverridestring?ToString()=>Name;
+
+publicboolIsInternalVisibility=>(Flags&KotlinPropertyFlags.VisibilityMask)==KotlinPropertyFlags.Internal;
+
+publicboolIsPrivateVisibility=>(Flags&KotlinPropertyFlags.VisibilityMask)==KotlinPropertyFlags.Private;
 }
 
 publicclassKotlinType
@@ -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(getterisnull&&setterisnull)
 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)
 
 staticMethodInfo?FindJavaPropertyGetter(KotlinFilekotlinClass,KotlinPropertyproperty,ClassFileklass)
 {
-varpossible_methods=klass.Methods.Where(method =>string.Compare(method.GetMethodNameWithoutSuffix(),$"get{property.Name}",StringComparison.OrdinalIgnoreCase)==0&&
+// Private properties do not have getters
+if(property.IsPrivateVisibility)
+returnnull;
+
+// Public/protected getters look like "getFoo"
+// Internal getters look like "getFoo$main"
+varpossible_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)
 
 staticMethodInfo?FindJavaPropertySetter(KotlinFilekotlinClass,KotlinPropertyproperty,ClassFileklass)
 {
-varpossible_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)
+returnnull;
+
+// Public/protected setters look like "setFoo"
+// Internal setters look like "setFoo$main"
+varpossible_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));
 
 returnpossible_methods.FirstOrDefault();
 }

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

@@ -1,5 +1,6 @@
 usingSystem;
 usingSystem.Collections.Generic;
+usingSystem.Diagnostics.CodeAnalysis;
 usingSystem.Linq;
 usingSystem.Text;
 usingSystem.Threading.Tasks;
@@ -8,6 +9,18 @@ namespace Xamarin.Android.Tools.Bytecode
 {
 publicstaticclassKotlinUtilities
 {
+[return:NotNullIfNotNull(nameof(value))]
+publicstaticstring?Capitalize(thisstring?value)
+{
+if(string.IsNullOrWhiteSpace(value))
+returnvalue;
+
+if(value.Length<1)
+returnvalue;
+
+returnchar.ToUpperInvariant(value[0])+value.Substring(1);
+}
+
 publicstaticstringConvertKotlinTypeSignature(KotlinType?type,KotlinFile?metadata=null,boolconvertUnsignedToPrimitive=true)
 {
 if(typeisnull)

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 {
 privatevar hitCount =0
 funhitCount(): Int= hitCount
 
-//Property and setter
+//Private property and explicit getter/setter
 privatevar type =0
-funsetType(type:Int) = { println (type); }
+fungetType(): Int= type
+funsetType(type:Int) { }
+
+// Private immutable property and explicit getter/setter
+privateval type2 =0
+fungetType2(): Int= type2
+funsetType2(type:Int) { }
+
+// Internal property and explicit getter/setter
+internalvar itype =0
+fungetItype(): Int= itype
+funsetItype(type:Int) { }
+
+// Internal immutable property and explicit getter/setter
+internalval itype2 =0
+fungetItype2(): Int= itype2
+funsetItype2(type:Int) { }
 }