Skip to content
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

[class-parse] support Module AttributeInfo #1097

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Apr 12, 2023

Fixes: #1096

Context: https://stackoverflow.com/questions/57358750/module-info-class-file-is-different-in-the-module-jar-file-and-compiled-module-i
Context: 678c4bd

JDK 9 adds support for modules, which are (kinda sorta) like
.NET Assemblies: modules can depend upon other modules, export
packages, etc.

In particular:

exports and exports…to. An exports module directive specifies
one of the module’s packages whose public types (and their nested
public and protected types) should be accessible to code in all
other modules.

This allows an equivalent to the C# internal access modifier:
public types in a non-exported package should be treated as
"internal", while public types in an exported package are
"fully public".

Update Xamarin.Android.Tools.Bytecode.dll to extract the module-
related information, the update XmlClassDeclarationBuilder so that
it updates all public types outside of the "exported" packages to
have a visibility of kotlin-internal.

Why a //*/@visibility value of kotlin-internal? From a
suggestion for the commit message of 678c4bd, which was sadly
overlooked in the final merge:

Note: we introduce and use a new //*/@visibility value of
kotlin-internal because internal is an existing value that may
be used in Metadata.xml files, e.g. making public API internal
so that it can still be used in the binding, but isn't public.

If we use internal, those types are still bound, it's just that
the bound types have C# internal visibility, while we want them
to be skipped entirely. A visibility value of kotlin-internal
allows us to skip them, which is desired.

tests/Xamarin.Android.Tools.Bytecode-Tests has been updated to:

  1. Contain a module-info.java, which declares a com.xamarin
    module.

  2. Add a new com.xamarin.internal.PublicClassNotInModuleExports
    type which is not in the com.xamarin package, but instead
    a nested package. The type is public.

  3. Build a xatb.jar artifact

This makes for a simple one-off test:

% dotnet build tests/Xamarin.Android.Tools.Bytecode-Tests/*.csproj
% dotnet build tools/class-parse/*.csproj
% dotnet bin/Debug-net7.0/class-parse.dll \
  tests/Xamarin.Android.Tools.Bytecode-Tests/obj/Debug-net7.0/xatb.jar
…
    <class
      name="PublicClassNotInModuleExports"
      …
      visibility="kotlin-internal" />

Note that com.xamarin.internal.PublicClassNotInModuleExports is now
shown as kotlin-internal instead of public.

Aside, a discovered oddity: jar cf … modifies module-info.class,
adding a ModulePackages attribute! (Specifically, if you compare
the "on-disk" module-info.class to the one within
tests/Xamarin.Android.Tools.Bytecode-Tests/obj/$(Configuration)/xatb.jar,
they differ in size!)

@jonpryor jonpryor requested a review from jpobst April 12, 2023 07:55
@jonpryor jonpryor force-pushed the jonp-class-parse-modules branch 2 times, most recently from ecc04de to bf9200c Compare April 20, 2023 15:52
@jonpryor jonpryor marked this pull request as ready for review April 20, 2023 15:58
@jonpryor
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Fixes: dotnet#1096

Context: https://stackoverflow.com/questions/57358750/module-info-class-file-is-different-in-the-module-jar-file-and-compiled-module-i
Context: 678c4bd

JDK 9 adds support for [modules][0], which are (kinda sorta) like
.NET Assemblies: modules can depend upon other modules, export
packages, etc.

In particular:

> **exports and exports…to.** An exports module directive specifies
> one of the module’s packages whose `public` types (and their nested
> `public` and `protected` types) should be accessible to code in all
> other modules.

This allows an equivalent to the [C# `internal` access modifier][1]:
`public` types in a *non-`export`ed package* should be treated as
"internal", while `public` types in an `export`ed package are
"fully public".

Update `Xamarin.Android.Tools.Bytecode.dll` to extract the module-
related information, the update `XmlClassDeclarationBuilder` so that
it updates all `public` types *outside* of the "exported" packages to
have a visibility of `kotlin-internal`.

Why a `//*/@visibility` value of `kotlin-internal`?  From a
[suggestion][2] for the commit message of 678c4bd, which was sadly
overlooked in the final merge:

> Note: we introduce and use a new `//*/@visibility` value of
> `kotlin-internal` because `internal` is an *existing* value that may
> be used in `Metadata.xml` files, e.g. making `public` API `internal`
> so that it can still be used in the binding, but isn't *public*.

If we use `internal`, *those types are still bound*, it's just that
the bound types have C# `internal` visibility, while we *want* them
to be *skipped entirely*.  A visibility value of `kotlin-internal`
allows us to skip them, which is desired.

`tests/Xamarin.Android.Tools.Bytecode-Tests` has been updated to:

 1. Contain a `module-info.java`, which declares a `com.xamarin`
    module.

 2. Add a new `com.xamarin.internal.PublicClassNotInModuleExports`
    type which is *not* in the `com.xamarin` package, but instead
    a *nested* package.  The type is `public`.

 3. Build a `xatb.jar` artifact

This makes for a simple one-off test:

	% dotnet build tests/Xamarin.Android.Tools.Bytecode-Tests/*.csproj
	% dotnet build tools/class-parse/*.csproj
	% dotnet bin/Debug-net7.0/class-parse.dll \
	  tests/Xamarin.Android.Tools.Bytecode-Tests/obj/Debug-net7.0/xatb.jar
	…
	    <class
	      name="PublicClassNotInModuleExports"
	      …
	      visibility="kotlin-internal" />

Note that `com.xamarin.internal.PublicClassNotInModuleExports` is now
shown as `kotlin-internal` instead of `public`.

Aside, a discovered oddity: `jar cf …` *modifies* `module-info.class`,
adding a `ModulePackages` attribute!  (Specifically, if you compare
the "on-disk" `module-info.class` to the one within
`tests/Xamarin.Android.Tools.Bytecode-Tests/obj/$(Configuration)/xatb.jar`,
they differ in size!)

[0]: https://www.oracle.com/corporate/features/understanding-java-9-modules.html
[1]: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/internal
[2]: dotnet#793 (comment)
@jonpryor jonpryor force-pushed the jonp-class-parse-modules branch from bf9200c to b18bfdf Compare April 20, 2023 18:16
@@ -88,9 +88,6 @@ static bool ShouldLoadEntry (ZipArchiveEntry entry)
if (entry.Length == 0)
return false;

if (entry.Name == "module-info.class")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to undo #1093?

That is, will this write module-info.class into api.xml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it will "undo" #1093, and No, it will not write module-info.class into api.xml. (I should explicitly call this out in the commit message; doh!)

See FixupModuleVisibility(), in particular https://github.com/xamarin/java.interop/pull/1097/files#diff-cd606380b5886cb27bfcf4f255f042d4cb739a6a33545920a305cba1113dac74R386

We do two things in FixupModuleVisibility():

  1. We collect all of the ClassFile entries of type ClassAccessFlags.Module, and remove them from classFiles. This prevents them from being written into api.xml.
  2. For the ClassFile entries in (1), we find the set of exported packages (in publicPackages), and (2) iterate over every entry in classFiles. Any classFiles entry with a package name outside of publicPackages has its visibility updated to ClassAccessFlags.Internal.

One problem with this approach, not considered until just now, is that if class-parse is parsing multiple .jar files and writing out a single api.xml, then the module-info.class information would be applied to entries from a different .jar. This may result in "weird" behavior, and I'll need to think about this.

@jonpryor jonpryor requested a review from jpobst April 20, 2023 20:39
@jpobst
Copy link
Contributor

jpobst commented Apr 20, 2023

It occurs to us that properly supporting Modules may potentially remove API's that AndroidX/GPS (and external bindings) have already shipped as "public".

jonpryor added a commit to jonpryor/java.interop that referenced this pull request Apr 20, 2023
Context: dotnet#1097 (comment)
Context: dotnet#1097 (comment)
Context: b274a67

During code review, two questions came to light:

 1. Will this cause API breakage in AndroidX/etc., because
    `module-info.class` will make types "disappear"?
    (See also b274a67).

 2. `class-parse` can accept *multiple* `.jar` files and `.class`
    files on the command-line.  Should `module-info.class` processing
    apply to *everything*?

Address the first concern by keeping `//*/@visibility` as `public`,
and instead if we think the type should be "internal" we instead add
an `//*/@annotated-visibility` attribute value of `module-info`.
If there is *already* an `//*/@annotated-visibility` value, then we
*append* ` module-info` to the attribute value.

Address the second concern by re-working `class-parse` command-line
parsing.  There is now a "global `ClassPath`", which will be used
to hold `.class` files provided on the command-line.  `.jar` and
`.jmod` files provided on the command-line will be given their own
`ClassPath` instances, and `module-info.class`-based visibility
fixups are specific to each `ClassPath` instance.  Global files are
processed together.  There is thus no way for `module-info.class`
visibility changes from `a.jar` to impact `b.jar`.  After
visibilities are fixed up, we then merge everything into the "global"
`ClassPath` instance before writing transforming to XML.

Additionally, `class-parse --dump` can now accept `.jar` files,
and will dump out *all* `.class` filers within the `.jar` file.
To make this output easier, each "entry" starts with a "header" of
`-- Begin {ClassFile.FullJniName}`, and a blank link will be printed
between each entry.
@jonpryor
Copy link
Member Author

@jpobst: latest commit attempts to resolve the two open issues:

Regarding:

It occurs to us that properly supporting Modules may potentially remove API's that AndroidX/GPS (and external bindings) have already shipped as "public".

We now keep visibility as public, and use the annotated-visibility XML attribute added as part of @RestrictTo processing to store module-info if a type should be hidden. This should maintain API compatibility in AndroidX/etc.

To address my concern about .jar "pollution":

if class-parse is parsing multiple .jar files and writing out a single api.xml, then the module-info.class information would be applied to entries from a different .jar.

This is addressed by altering class-parse command-line handling. If a .jar or .jmod file is encountered, then module-info.class-based visibility fixups are done local to that .jar file. The module-info.class files from one .jar won't be used to process other .jar files.

Context: dotnet#1097 (comment)
Context: dotnet#1097 (comment)
Context: b274a67

During code review, two questions came to light:

 1. Will this cause API breakage in AndroidX/etc., because
    `module-info.class` will make types "disappear"?
    (See also b274a67).

 2. `class-parse` can accept *multiple* `.jar` files and `.class`
    files on the command-line.  Should `module-info.class` processing
    apply to *everything*?

Address the first concern by keeping `//*/@visibility` as `public`,
and instead if we think the type should be "internal" we instead add
an `//*/@annotated-visibility` attribute value of `module-info`.
If there is *already* an `//*/@annotated-visibility` value, then we
*append* ` module-info` to the attribute value.

Address the second concern by re-working `class-parse` command-line
parsing.  There is now a "global `ClassPath`", which will be used
to hold `.class` files provided on the command-line.  `.jar` and
`.jmod` files provided on the command-line will be given their own
`ClassPath` instances, and `module-info.class`-based visibility
fixups are specific to each `ClassPath` instance.  Global files are
processed together.  There is thus no way for `module-info.class`
visibility changes from `a.jar` to impact `b.jar`.  After
visibilities are fixed up, we then merge everything into the "global"
`ClassPath` instance before writing transforming to XML.

Additionally, `class-parse --dump` can now accept `.jar` files,
and will dump out *all* `.class` filers within the `.jar` file.
To make this output easier, each "entry" starts with a "header" of
`-- Begin {ClassFile.FullJniName}`, and a blank link will be printed
between each entry.
@jonpryor jonpryor force-pushed the jonp-class-parse-modules branch from cd829ae to ba8877e Compare April 21, 2023 16:45
@jonpryor jonpryor merged commit 07d5595 into dotnet:main Apr 24, 2023
jonpryor pushed a commit to dotnet/android that referenced this pull request Apr 25, 2023
Fixes: dotnet/java-interop#1096

Changes: dotnet/java-interop@f0e3300...07d5595

 * dotnet/java-interop@07d5595b: [class-parse] support Module AttributeInfo (dotnet/java-interop#1097)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support module-info.class
2 participants