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.

Sign up for GitHub

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

Jump to bottom

Public types nested in internal Kotlin types not marked as internal #682

Closed
jpobst opened this issue Jul 24, 2020 · 2 comments · Fixed by #723
Closed

Public types nested in internal Kotlin types not marked as internal #682

jpobst opened this issue Jul 24, 2020 · 2 comments · Fixed by #723
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jpobst
Copy link
Contributor

jpobst commented Jul 24, 2020 •
edited
Loading

Context: dotnet/android#4955

Given a public nested type inside a private/internal type in Kotlin:

internal data class ParentType () {
    enum class ChildEnum { VALUE1, VALUE2 }
}

We correctly hide ParentType but we do not hide ChildEnum because we do not check parent visibility. This results in a warning such as this when generator is trying nest the ChildEnum object inside to ParentType object:

warning BG8604: top ancestor ParentType not found for nested type ParentType.ChildEnum

The good news is ChildEnum will not be bound due to the warning, so the final binding is correct, but we should handle this correctly.
@jpobst jpobst mentioned this issue Jul 24, 2020
Closed
@jpobst jpobst added bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.) labels Jul 24, 2020
@jpobst jpobst mentioned this issue Sep 18, 2020
Merged
jonpryor pushed a commit that referenced this issue Sep 18, 2020
@jpobst
[Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (#723)
5136ef9 
Fixes: #682

Context: dotnet/android#4955

We added support for reading Kotlin-provided metadata in commit
439bd83.  Part of this was recognizing when a type was
"Kotlin-internal" and marking it as `private` or "package-private" to
prevent `generator` from attempting to bind it. 

However, we didn't consider *nested* types of `private` or
"package-private" types in 439bd83, leaving the nested type
visibility unchanged.  This could result in a warning when binding
such nested types:

	warning BG8604: top ancestor ParentType not found for nested type ParentType.ChildEnum 

Note: this doesn't prevent building -- unless `csc /warnaserror` is
used -- and the type is not bound, so we are doing the "correct" thing.
This warning *does* create a "less-than-desirable" user experience,
possibly confusing users.

Update `generator` to change the visibility of nested types within
non-`public` types so that `generator` doesn't attempt to bind the
nested types.  This avoids the BG8604 warning.
jonpryor pushed a commit that referenced this issue Sep 18, 2020
@jpobst @jonpryor
[Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (#723)
1a19ec0 
Fixes: #682

Context: dotnet/android#4955

We added support for reading Kotlin-provided metadata in commit
439bd83.  Part of this was recognizing when a type was
"Kotlin-internal" and marking it as `private` or "package-private" to
prevent `generator` from attempting to bind it. 

However, we didn't consider *nested* types of `private` or
"package-private" types in 439bd83, leaving the nested type
visibility unchanged.  This could result in a warning when binding
such nested types:

	warning BG8604: top ancestor ParentType not found for nested type ParentType.ChildEnum 

Note: this doesn't prevent building -- unless `csc /warnaserror` is
used -- and the type is not bound, so we are doing the "correct" thing.
This warning *does* create a "less-than-desirable" user experience,
possibly confusing users.

Update `generator` to change the visibility of nested types within
non-`public` types so that `generator` doesn't attempt to bind the
nested types.  This avoids the BG8604 warning.
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Sep 21, 2020
@jonpryor
Bump to xamarin/Java.Interop/d16-8@79d95334
d6a7bbc 
Fixes: dotnet/java-interop#682
Fixes: dotnet/java-interop#717

Context: dotnet/java-interop#719

Changes: dotnet/java-interop@a807961...79d9533

  * dotnet/java-interop@79d95334: [generator] Use GC.KeepAlive for reference type method parameters. (dotnet#722)
  * dotnet/java-interop@1a19ec04: [Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (dotnet#723)
  * dotnet/java-interop@24a9abdb: [Xamarin.Android.Tools.ApiXmlAdjuster] Find app.android.IntentService (dotnet#718)
@jonpryor jonpryor mentioned this issue Sep 21, 2020
Merged
jonpryor added a commit to dotnet/android that referenced this issue Sep 22, 2020
@jonpryor
Bump to xamarin/Java.Interop/d16-8@79d95334 (#5139)
5e36e6e 
Fixes: dotnet/java-interop#682
Fixes: dotnet/java-interop#717

Context: dotnet/java-interop#719

Changes: dotnet/java-interop@a807961...79d9533

  * dotnet/java-interop@79d95334: [generator] Use GC.KeepAlive for reference type method parameters. (#722)
  * dotnet/java-interop@1a19ec04: [Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (#723)
  * dotnet/java-interop@24a9abdb: [Xamarin.Android.Tools.ApiXmlAdjuster] Find app.android.IntentService (#718)
@brendanzagaeski
Copy link
Contributor

Release status update

A new Preview version of Xamarin.Android has now been published that includes the fix for this item. The fix is not yet included in a Release version. I will update this item again when a Release version is available that includes the fix.

Fix included in Xamarin.Android SDK version 11.1.0.15.

Fix included on Windows in Visual Studio 2019 version 16.8 Preview 4. To try the Preview version that includes the fix, check for the latest updates in Visual Studio Preview.

Fix included on macOS in Visual Studio 2019 for Mac version 8.8 Preview 4. To try the Preview version that includes the fix, check for the latest updates on the Preview updater channel.

@jingliancui jingliancui mentioned this issue Oct 16, 2020
Closed
jonpryor pushed a commit to dotnet/android that referenced this issue Oct 20, 2020
@jpobst
Bump to xamarin/java.interop/master@b991bb86 (#5198)
03108f1 
Fixes: dotnet/java-interop#461
Fixes: dotnet/java-interop#682
Fixes: dotnet/java-interop#717
Fixes: dotnet/java-interop#719
Fixes: dotnet/java-interop#728

Changes: dotnet/java-interop@ac914ce...b991bb8

  * dotnet/java-interop@b991bb86: [generator] Revert change to use auto-properties in EventArgs classes (#736)
  * dotnet/java-interop@ee50d89b: Bump to xamarin/xamarin-android-tools/master@f2af06f2 (#733)
  * dotnet/java-interop@a0b895c1: [build] Suppress NuGet warnings (#730)
  * dotnet/java-interop@8b1b0507: [generator] Fix parsing of complex generic types (#729)
  * dotnet/java-interop@ee7afeed: [generator] Prevent generating duplicate EventArgs classes (#726)
  * dotnet/java-interop@1f21f38c: [generator] Use GC.KeepAlive for reference type method parameters. (#725)
  * dotnet/java-interop@5136ef98: [Xamarin.Android.Tools.Bytecode] Hide Kotlin nested types inside (#723)
  * dotnet/java-interop@53d60513: [jnimarshalmethod-gen] Fix registration on Windows (#721)
  * dotnet/java-interop@5a834d42: [jnimarshalmethod-gen] Avoid creating AppDomains (#720)
  * dotnet/java-interop@a76edb8c: [Xamarin.Android.Tools.ApiXmlAdjuster] Find app.android.IntentService (#718)
  * dotnet/java-interop@6cde0877: [Java.Interop] Emit a reference assembly for Java.Interop.dll (#716)
  * dotnet/java-interop@b858dc59: [generator] Provide line/col numbers for api.xml warnings (#715)
  * dotnet/java-interop@9be92a04: [ci] Don't kick off CI for documentation only changes. (#712)
  * dotnet/java-interop@03c22722: [jnimarshalmethod-gen] Fix type resolution crash (#706)
@brendanzagaeski
Copy link
Contributor

Release status update

A new Release version of Xamarin.Android has now been published that includes the fix for this item.

Fix included in Xamarin.Android SDK version 11.1.0.17.

Fix included on Windows in Visual Studio 2019 version 16.8. To get the new version that includes the fix, check for the latest updates or install the most recent release from https://visualstudio.microsoft.com/downloads/.

Fix included on macOS in Visual Studio 2019 for Mac version 8.8. To get the new version that includes the fix, check for the latest updates on the Stable updater channel.

@mmarinchenko mmarinchenko mentioned this issue Apr 24, 2021
Open
@jpobst jpobst mentioned this issue Apr 26, 2021
Merged
jonpryor pushed a commit that referenced this issue May 3, 2021
@jpobst
[Xamarin.Android.Tools.Bytecode] hide Kotlin internal nested types (#827
4ef5081 
)

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
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Bytecode] Hide nested types inside hidden Kotlin types. dotnet/java-interop
2 participants
@jpobst @brendanzagaeski