Skip to content

Public interfaces nested in internal Kotlin types not marked as internal #826

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Open
mmarinchenko opened this issue Apr 24, 2021 · 3 comments · Fixed by #827
Open

Public interfaces nested in internal Kotlin types not marked as internal #826

mmarinchenko opened this issue Apr 24, 2021 · 3 comments · Fixed by #827
Labels
generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@mmarinchenko
Copy link

Context: xamarin/GooglePlayServicesComponents#454 (comment)

If I'm not mistaken this issue is similar to #682 but is related to nested interfaces.
@mmarinchenko mmarinchenko mentioned this issue Apr 24, 2021
Closed
@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
@mmarinchenko
Copy link
Author

@jpobst, @jonpryor Thanks guys!

@mmarinchenko mmarinchenko mentioned this issue Jun 1, 2021
Open
This was referenced Jul 2, 2021
Closed
Merged
@jpobst
Copy link
Contributor

jpobst commented Jul 2, 2021

This fix had to be reverted because it was implemented incorrectly.

#854

@jpobst jpobst reopened this Jul 2, 2021
jonpryor pushed a commit that referenced this issue Jul 2, 2021
@jpobst
Revert "[Xamarin.Android.Tools.Bytecode] hide nested types (#827)" (#855
4a02bc3 
)

Revert commit 4ef5081.

Fixes: #854

Context: 4ef5081
Context: #826

[As noted][0] in [PR #827][1], there is some "weirdness" with what
appears in the [`InnerClasses` collection][2].  It turns out this is
due to a misunderstanding of what the `InnerClasses` collection
contains.

As per the [docs][2]]:

> If a class has members that are classes or interfaces, its
> `constant_pool` table (and hence its `InnerClasses` attribute) must
> refer to each such member, even if that member is not otherwise
> mentioned by the class.
> **These rules imply that a nested class or interface member will
> have `InnerClasses` information for each enclosing class and for
> each immediate member.**

(Emphasis added.)  That is, the `PagedList$Config$Builder$Companion`
class lists *both* its immediate containing type
`PagedList$Config$Builder` and the "parent-parent" containing type
`PagedList$Config` within `InnerClasses`.

The change made in commit 4ef5081 loops through `InnerClasses`,
marking them as `internal`, assuming they are all nested types.
This is causing us to hide *declaring* types when we are trying to
hide *nested* types.

This will require more investigation and the deadline for 16.11 is
~now, so we're just going to revert the original commit.

[0]: #827 (comment)
[1]: #827
[2]: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.6
jonpryor pushed a commit that referenced this issue Jul 2, 2021
@jpobst @jonpryor
Revert "[Xamarin.Android.Tools.Bytecode] hide nested types (#827)" (#855
48766c0 
)

Revert commit 4ef5081.

Fixes: #854

Context: 4ef5081
Context: #826

[As noted][0] in [PR #827][1], there is some "weirdness" with what
appears in the [`InnerClasses` collection][2].  It turns out this is
due to a misunderstanding of what the `InnerClasses` collection
contains.

As per the [docs][2]]:

> If a class has members that are classes or interfaces, its
> `constant_pool` table (and hence its `InnerClasses` attribute) must
> refer to each such member, even if that member is not otherwise
> mentioned by the class.
> **These rules imply that a nested class or interface member will
> have `InnerClasses` information for each enclosing class and for
> each immediate member.**

(Emphasis added.)  That is, the `PagedList$Config$Builder$Companion`
class lists *both* its immediate containing type
`PagedList$Config$Builder` and the "parent-parent" containing type
`PagedList$Config` within `InnerClasses`.

The change made in commit 4ef5081 loops through `InnerClasses`,
marking them as `internal`, assuming they are all nested types.
This is causing us to hide *declaring* types when we are trying to
hide *nested* types.

This will require more investigation and the deadline for 16.11 is
~now, so we're just going to revert the original commit.

[0]: #827 (comment)
[1]: #827
[2]: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.6
jonpryor added a commit to dotnet/android that referenced this issue Jul 2, 2021
@jonpryor
Bump to xamarin/Java.Interop/d16-11@48766c0d
7776c9f 
Fixes: dotnet/java-interop#854
Context: dotnet/java-interop#826

Changes: dotnet/java-interop@5e31647...48766c0

  * dotnet/java-interop@48766c0d: Revert "[Xamarin.Android.Tools.Bytecode] hide nested types (#827)" (#855)
@jpobst jpobst added bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.) labels Jul 9, 2021
@mmarinchenko
Copy link
Author

This fix had to be reverted because it was implemented incorrectly.

#854

Hi! A year has passed since then:) Do you guys have any plans to work on this issue?

@jpobst jpobst removed the bug Component does not function as intended label Apr 29, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Milestone
No milestone
2 participants
@jpobst @mmarinchenko