Skip to content

[XAT.Bytecode] Fix issue where Kotlin public types inside internal types were not being hidden. #827

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

Merged
merged 1 commit into from
May 3, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Apr 26, 2021

Fixes: #826
Context: #682

Given the following Kotlin code:

internal MyClass {
  public MyInterface { }
}

The default Java representation of this would be:

public MyClass {
  public MyInterface { }
}

In order to prevent this being bound, our Kotlin fixups instead change it to

private MyClass {
  private MyInterface { }
}

However there is a bug preventing the internal interface from being marked private, because we are setting the visibility on the parent type's InnerClassAccessFlags. When we output the XML we do not check that, we instead look at the flags on the nested type's .class file.

This commit fixes this by finding the appropriate inner class .class file and updating the access flags there (recursively).

In our api.xml, types are flattened. Without this, generator will skip importing the parent type but will import the nested type(s). When it attempts to create the nested model, it cannot find the parent type, resulting in a message like:

warning BG8604: top ancestor MyClass not found for nested type MyClass.MyInterface

@jpobst jpobst marked this pull request as ready for review April 26, 2021 19:59

// Setting the inner class access flags above doesn't technically do anything, because we output
// from the inner class's ClassFile, so we need to find that and set it there.
var kf = classes.FirstOrDefault (c => c.ThisClass.Name.Value == klass.InnerClass.Name.Value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if classes should become a KeyedCollection<string, ClassFile>? I'm just worried that if classes gets "big", .FirstOrDefault() may become a time sink.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially, though this should be a rarely used code path.

kf.AccessFlags = SetVisibility (kf.AccessFlags, ClassAccessFlags.Private);
kf.InnerClass.InnerClassAccessFlags = SetVisibility (kf.InnerClass.InnerClassAccessFlags, ClassAccessFlags.Private);

foreach (var inner_class in kf.InnerClasses.Where (c => c.OuterClass.Name.Value == kf.ThisClass.Name.Value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this .Where() cause all of kf.InnerClasses to be included? How could kf.InnerClasses contain a type for which the inner type's c.OuterClass doesn't match the containing type?

Wouldn't this more easily & efficiently be expressed as:

foreach (var inner_class in kf.InnerClasses) {
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…or does kf.InnerClasses contain "grand-children" as well? For example, in the case of HidePublicInterfaceInInternalClass, will InternalClassWithNestedInterface.class specify InternalClassWithNestedInterface$NestedInterface$DoubleNestedInterface.class as an inner class, even though it's an inner class of InternalClassWithNestedInterface$NestedInterface.class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason the InnerClasses collection for an inner class contains itself. Without this check it infinitely recurses into itself.

That is, the InnerClasses for InternalClassWithNestedInterface$NestedInterface contains InternalClassWithNestedInterface$NestedInterface.

@jonpryor jonpryor merged commit 4ef5081 into main May 3, 2021
@jonpryor jonpryor deleted the internal-kotlin branch May 3, 2021 17:10
@jpobst jpobst added this to the 11.4 (16.11 / 8.11) milestone May 5, 2021
jpobst added a commit that referenced this pull request Jul 2, 2021
jonpryor pushed a commit that referenced this pull request Jul 2, 2021
)

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 pull request Jul 2, 2021
)

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
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public interfaces nested in internal Kotlin types not marked as internal
2 participants