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

Regression hiding nested internal Kotlin types #854

Closed
jpobst opened this issue Jul 2, 2021 · 0 comments · Fixed by #855
Closed

Regression hiding nested internal Kotlin types #854

jpobst opened this issue Jul 2, 2021 · 0 comments · Fixed by #855
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 2, 2021 •
edited
Loading

Context: #827
Context: #826

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

Per the docs:

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.**

That is, a PagedList$Config$Builder$Companion class lists its grandparent type PagedList$Config and parent type PagedList$Config$Builder as InnerClasses.

The change made in PR #827 loops through InnerClasses, marking them as internal, assuming they are all nested types. This is causing us to hide parent types when we are trying to hide child types.

This will require more investigation and the deadline for 16.11 is ~now, so we're just going to revert the original commit for now.
@jpobst jpobst added bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.) labels Jul 2, 2021
This was referenced Jul 2, 2021
Merged
Open
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)
jonpryor pushed a commit to dotnet/android that referenced this issue Jul 21, 2021
@dotnet-maestro @jonathanpeppers
Bump to dotnet/installer@9c463710 6.0.100-rc.1.21369 (#6072)
1e5bfa3 
Fixes: dotnet/java-interop#854

Changes: dotnet/installer@e8b3b6b...9c46371

    % git diff --shortstat e8b3b6be...9c463710
     103 files changed, 2301 insertions(+), 2757 deletions(-)

Changes: dotnet/linker@a07cab7...460dd6d

    % git diff --shortstat a07cab7b...460dd6dd
     84 files changed, 2403 insertions(+), 258 deletions(-)

Changes: dotnet/runtime@02f70d0...96ce6b3

    % git diff --shortstat 02f70d0b90...96ce6b3535
     2586 files changed, 123677 insertions(+), 34433 deletions(-)

Changes: dotnet/java-interop@a5ed891...4fb7c14

  * dotnet/java-interop@4fb7c147: [build] set $(DisableImplicitNamespaceImports) by default (#859)
  * dotnet/java-interop@855ecfa3: [generator] Don't generate unexpected NRT types like `void?` (#856)
  * dotnet/java-interop@4a02bc32: Revert "[Xamarin.Android.Tools.Bytecode] hide nested types (#827)" (#855)
  * dotnet/java-interop@95c9b79d: [generator] Avoid 'error (…):' construct in diagnostic messages (#851)
  * dotnet/java-interop@7c4f7db0: [build] Bump to Mono with MSBuild 16.10 (#848)
  * dotnet/java-interop@0227cdae: [generator] Gracefully handle BindingGeneratorException. (#845)
  * dotnet/java-interop@ce1750fd: Add SECURITY.md (#846)

Context: dotnet/runtime#55384
Context: dotnet/sdk#18639

Updates:

  * Microsoft.Dotnet.Sdk.Internal: from 6.0.100-preview.7.21327.2 to 6.0.100-rc.1.21369.3
  * Microsoft.NET.ILLink.Tasks: from 6.0.100-preview.6.21322.1 to 6.0.100-preview.6.21366.2
  * Microsoft.NETCore.App.Ref: from 6.0.0-preview.7.21326.8 to 6.0.0-rc.1.21368.1

dotnet/runtime#55384 broke how .NET 6 interacts with
`AndroidClientHandler`.  Fix this by introducing a new
`Xamarin.Android.Net.AndroidMessageHandler` type for use on .NET 6,
and update the .NET 6 `AndroidClientHandler` to delegate to
`AndroidMessageHandler`.

`AndroidMessageHandler` doesn't exist on Legacy.

Update `.apkdesc` files:

  * `BuildReleaseArm64SimpleDotNet` is ~37KB smaller
  * `BuildReleaseArm64XFormsDotNet` is ~62KB larger.

Update `tests/api-compatibility/api-compat-exclude-attributes.txt`
so that `T:System.Runtime.CompilerServices.CompilerGeneratedAttribute`
is ignored.  `[CompilerGeneratedAttribute]` is emitted as part of
C# 3 "auto-props":

	public T Property { get; set; }

Converting this into a "real" property:

	T value;
	public T Property {
	    get => value;
	    set => this.value = value;
	}

results in the `_CheckApiCompatibility` target complaining about an
API break.  We don't care; ignore `[CompilerGeneratedAttribute]`.

Remove `$(SelfContained)` property: early on in .NET 5 (yes 5)
development, the Xamarin.Android SDK needed to specify
`$(SelfContained)` by default in order to produce `.apk` files.

After we became a proper workload, setting the value became
unnecessary.  It also didn't actually do anything because
dotnet/sdk overwrote the value.

Starting in dotnet/sdk#18639,
`Microsoft.NET.RuntimeIdentifierInference.targets` is now being
imported *after* a workload, which meant that dotnet/sdk no longer
overwrote our `$(SelfContained)` value, which broke things:

	error NETSDK1031: It is not supported to build or publish a self-contained application without specifying a RuntimeIdentifier. You must either specify a RuntimeIdentifier or set SelfContained to false.

We can simply remove `$(SelfContained)` now, and rely on
dotnet/sdk to set this value.

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: Steve Pfister <steve.pfister@microsoft.com>
jonpryor pushed a commit to dotnet/android that referenced this issue Jul 21, 2021
@dotnet-maestro @jonathanpeppers
Bump to dotnet/installer@808795cc 6.0.100-preview.7.21369.5 (#6097)
b10fe09 
Fixes: dotnet/java-interop#854

Changes: dotnet/installer@e8b3b6b...808795c

    % git diff --shortstat e8b3b6be...808795cc
     102 files changed, 2218 insertions(+), 2674 deletions(-)

Changes: dotnet/linker@a07cab7...9ecf5bd

    % git diff --shortstat a07cab7b...9ecf5bd2
     81 files changed, 2122 insertions(+), 246 deletions(-)


Changes: dotnet/runtime@02f70d0...8d3afa3

    % git diff --shortstat 02f70d0b...8d3afa3a
     2518 files changed, 122843 insertions(+), 33676 deletions(-)

Changes: dotnet/java-interop@a5ed891...4fb7c14

  * dotnet/java-interop@4fb7c147: [build] set $(DisableImplicitNamespaceImports) by default (#859)
  * dotnet/java-interop@855ecfa3: [generator] Don't generate unexpected NRT types like `void?` (#856)
  * dotnet/java-interop@4a02bc32: Revert "[Xamarin.Android.Tools.Bytecode] hide nested types (#827)" (#855)
  * dotnet/java-interop@95c9b79d: [generator] Avoid 'error (…):' construct in diagnostic messages (#851)
  * dotnet/java-interop@7c4f7db0: [build] Bump to Mono with MSBuild 16.10 (#848)
  * dotnet/java-interop@0227cdae: [generator] Gracefully handle BindingGeneratorException. (#845)
  * dotnet/java-interop@ce1750fd: Add SECURITY.md (#846)

Context: dotnet/runtime#55384
Context: dotnet/sdk#18639

Updates:

  * Microsoft.Dotnet.Sdk.Internal: from 6.0.100-preview.7.21327.2 to 6.0.100-preview.7.21369.5
  * Microsoft.NET.ILLink.Tasks: from 6.0.100-preview.6.21322.1 to 6.0.100-preview.6.21365.1
  * Microsoft.NETCore.App.Ref: from 6.0.0-preview.7.21326.8 to 6.0.0-preview.7.21368.2

dotnet/runtime#55384 broke how .NET 6 interacts with
`AndroidClientHandler`.  Fix this by introducing a new
`Xamarin.Android.Net.AndroidMessageHandler` type for use on .NET 6,
and update the .NET 6 `AndroidClientHandler` to delegate to
`AndroidMessageHandler`.

`AndroidMessageHandler` doesn't exist on Legacy.

Update `.apkdesc` files:

  * `BuildReleaseArm64SimpleDotNet` is ~37KB smaller
  * `BuildReleaseArm64XFormsDotNet` is ~62KB larger.

Update `tests/api-compatibility/api-compat-exclude-attributes.txt`
so that `T:System.Runtime.CompilerServices.CompilerGeneratedAttribute`
is ignored.  `[CompilerGeneratedAttribute]` is emitted as part of
C# 3 "auto-props":

	public T Property { get; set; }

Converting this into a "real" property:

	T value;
	public T Property {
	    get => value;
	    set => this.value = value;
	}

results in the `_CheckApiCompatibility` target complaining about an
API break.  We don't care; ignore `[CompilerGeneratedAttribute]`.

Remove `$(SelfContained)` property: early on in .NET 5 (yes 5)
development, the Xamarin.Android SDK needed to specify
`$(SelfContained)` by default in order to produce `.apk` files.

After we became a proper workload, setting the value became
unnecessary.  It also didn't actually do anything because
dotnet/sdk overwrote the value.

Starting in dotnet/sdk#18639,
`Microsoft.NET.RuntimeIdentifierInference.targets` is now being
imported *after* a workload, which meant that dotnet/sdk no longer
overwrote our `$(SelfContained)` value, which broke things:

	error NETSDK1031: It is not supported to build or publish a self-contained application without specifying a RuntimeIdentifier. You must either specify a RuntimeIdentifier or set SelfContained to false.

We can simply remove `$(SelfContained)` now, and rely on
dotnet/sdk to set this value.

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: Steve Pfister <steve.pfister@microsoft.com>
@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.
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
1 participant
@jpobst