Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kotlin: public set of private var shouldn't be eliminated #1139

Closed
bryanlogan opened this issue Aug 24, 2023 · 0 comments · Fixed by #1151 or dotnet/android#8339
Closed

Kotlin: public set of private var shouldn't be eliminated #1139

bryanlogan opened this issue Aug 24, 2023 · 0 comments · Fixed by #1151 or dotnet/android#8339
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@bryanlogan
Copy link

bryanlogan commented Aug 24, 2023

Android application type

.NET Android (net7.0-android, etc.)

Affected platform version

VSMac 17.6.3 (build 421)

Description

Consider a class like this:

data class Foo(val value: String)

class FooManager {
    private var customFoo: Foo? = null
    private var defaultFoo = Foo("default")

    val foo
        get() = customFoo ?: defaultFoo

    fun setCustomFoo(value: Foo) {
        customFoo = value
    }
}

setCustomFoo is a public set of a private var, but it shouldn't be eliminated because it's not generated because of Kotlin's "internal".

https://github.com/xamarin/java.interop/blob/main/tests/Xamarin.Android.Tools.Bytecode-Tests/kotlin/NameShadowing.kt#L13
fun setType(type: Int) = { println (type); } should be fun setType(type: Int) { println (type); }

The equals make setType return Unit() and not void, so the test case does pass as expected. If we fix this, then the test case fails because setType is being removed.

I think the best course is to modify KotlinFixups::FixupProperty to only remove them if both the getter and setter exist and they're both public. There could still be some scenarios where this is inadvertently hiding functions, so maybe there needs to be a way to manually skip this check?

Steps to Reproduce

Fix the NameShadowing setType class and the test cases will fail.

Did you find any workaround?

Override and force it public: https://docs.microsoft.com/en-us/xamarin/android/platform/binding-java-library/customizing-bindings/java-bindings-metadata#visibility

Or changing the backing field in the library so the names don't conflict.

Relevant log output

No response

@jpobst jpobst transferred this issue from dotnet/android Aug 24, 2023
@jpobst jpobst changed the title public set of private var shouldn't be eliminated Kotlin: public set of private var shouldn't be eliminated Aug 24, 2023
@jpobst jpobst added the generator Issues binding a Java library (generator, class-parse, etc.) label Aug 24, 2023
@jpobst jpobst removed their assignment Aug 24, 2023
@jpobst jpobst added the bug Component does not function as intended label Sep 1, 2023
jonpryor pushed a commit that referenced this issue Oct 26, 2023
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
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Oct 26, 2023
Fixes: dotnet/java-interop#910
Fixes: dotnet/java-interop#1139

Changes: dotnet/java-interop@3c83179...1adb796

  * dotnet/java-interop@1adb7964: [generator] `generator --lang-features=emit-legacy-interface-invokers` (dotnet/java-interop#1145)
  * dotnet/java-interop@6bd7ae48: [Xamarin.Android.Tools.Bytecode] Kotlin internal prop visibility (dotnet/java-interop#1151)

Updates `class-parse` to better support parsing the visibility of
Kotlin properties.

Updates `generator` to emit optimized interface Invokers.

Add support for a new `$(AndroidEmitLegacyInterfaceInvokers)` MSBuild
property; when `true`, causes `generator` to emit legacy interface
Invokers.  The default value is `false`.
jonpryor added a commit to dotnet/android that referenced this issue Nov 7, 2023
Fixes: dotnet/java-interop#910
Fixes: dotnet/java-interop#1139

Changes: dotnet/java-interop@3c83179...38c8a82

  * dotnet/java-interop@38c8a827: [Xamarin.Android.Tools.Bytecode] Kotlin unsigned internal props (dotnet/java-interop#1156)
  * dotnet/java-interop@1adb7964: [generator] `generator --lang-features=emit-legacy-interface-invokers` (dotnet/java-interop#1145)
  * dotnet/java-interop@6bd7ae48: [Xamarin.Android.Tools.Bytecode] Kotlin internal prop visibility (dotnet/java-interop#1151)

Updates `class-parse` to better support parsing the visibility of
Kotlin properties.

Updates `generator` to emit optimized interface Invokers.
The previous interface invoker codegen strategy can be enabled by
setting `$(_AndroidEmitLegacyInterfaceInvokers)`=True.
jonathanpeppers pushed a commit that referenced this issue Nov 13, 2023
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
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
2 participants