[Xamarin.Android.Tools.Bytecode] Only hide getters/setters for internal properties.

[jpobst]

Fixes: #1139

When a property is declared in Kotlin, Java getters/setters are generated as needed. In the case where the property is internal, which is not supported by Java, public getters/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, we have the following code in androidx/emoji2/emojipicker/EmojiPickerView.kt:

private var onEmojiPickedListener: Consumer<EmojiViewItem>? = null

fun setOnEmojiPickedListener(onEmojiPickedListener: Consumer<EmojiViewItem>?) {
    this.onEmojiPickedListener = onEmojiPickedListener
}

In this case, we are hiding the "setter" because the property is private. However this isn't a generated setter, this is an explicit public setter that should remain public.

One mistake we are making is that private properties do not have generated setters, so we should not attempt to hide anything for private properties, only internal properties.

That is, this code:

private var type = 0
internal var itype = 0

generates:

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/setters, the generated name differs from the names given to public getters/setters.

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

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

Thus this PR makes 2 changes:

• Do not attempt to hide getters/setters for private Kotlin properties.
• Improve matching of generated names for internal Kotlin properties.

As #1139 points out, 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 unit test fails without the PR changes. 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.

[jonpryor]

Trying to verify/validate this is redonkulous:

• https://github.com/JetBrains/kotlin/blob/210cc6704109d70290f2944fb24bcdf58d4f2fae/core/metadata/src/org/jetbrains/kotlin/metadata/deserialization/Flags.java#L22
• https://github.com/JetBrains/kotlin/blob/210cc6704109d70290f2944fb24bcdf58d4f2fae/core/metadata/src/metadata.proto#L443-L444

Fortunately, instead of trying to make sense of their code, I can just have the Java compiler tell me:

import org.jetbrains.kotlin.metadata.deserialization.Flags;

class DumpKotlinFlags {
    public static void main(String[] args) {
        System.out.println("IS_EXTERNAL_PROPERTY=" + Flags.IS_EXTERNAL_PROPERTY.invert(0));
    }
}

% javac -cp ~/Downloads/kotlinc/lib/kotlin-compiler.jar dump-kotlin-flags.java
% java -cp ~/Downloads/kotlinc/lib/kotlin-compiler.jar:. DumpKotlinFlags
IS_EXTERNAL_PROPERTY=16384

16384 is 0x4000 which is 0b_0100_0000_0000_0000, which is 0b_001000000_00_00_000_0. Things match!

[jonpryor]

This feels fragile and prone to bite us, e.g. Capitalize("ioRequest")=="IoRequest".

I half wonder if looking for case- insensitive checks elsewhere would be "better", as with this approach we're "guessing" at the intended capitalization, at the cost of not allowing "case-differing properties".

[jpobst]

Agreed that this feels fragile, but it does seem to be what Kotlin does:

// Kotlin
internal val Capital1 = 0
internal val capital2 = 0
internal val cAPitAL3 = 0

// Generated Java
private final int Capital1;  
private final int capital2;  
private final int cAPitAL3;

public final int getCapital1$main() {
  return this.Capital1;
}
  
public final int getCapital2$main() {
  return this.capital2;
}
  
public final int getCAPitAL3$main() {
  return this.cAPitAL3;
}

"case-differing properties" do seem to be allowed:

// Kotlin
internal val Foo = 0
internal val FOO = 0

// Generated Java
private final int Foo;
private final int FOO;

public final int getFoo$main() {
  return this.Foo;
}
  
public final int getFOO$main() {
  return this.FOO;
}

The "good" news is that it does guard against case-differing property names that would create a clash:

// Kotlin
internal val foo = 0
internal val Foo = 0

error G6DDBDC0F: platform declaration clash: The following declarations have the same JVM signature (getFoo$main()I):
    fun `<get-Foo>`(): Int defined in NameShadowing
    fun `<get-foo>`(): Int defined in NameShadowing
  internal val foo = 0

[jonpryor]

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

Backing Fields
Classes in Kotlin cannot have fields.

[jonpryor]

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

Fixes: https://github.com/xamarin/java.interop/issues/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 (678c4bd2): 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