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.

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

Support module-info.class #1096

Closed
3 tasks
jonpryor opened this issue Apr 10, 2023 · 1 comment · Fixed by #1097
Closed
3 tasks

Support module-info.class #1096

jonpryor opened this issue Apr 10, 2023 · 1 comment · Fixed by #1097
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

Context: #1093
Context: https://mvnrepository.com/artifact/org.jetbrains/annotations/24.0.1
Context: https://www.oracle.com/corporate/features/understanding-java-9-modules.html
Context: https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.25

JDK 9 adds support for modules, which is (kinda sorta) like .NET Assemblies: modules can depend upon other modules, export types, 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. An exports…to directive enables you to specify in a comma-separated list precisely which module’s or modules’ code can access the exported package—this is known as a qualified export.

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 a "fully public".

TODO:

  • Implement support in Xamarin.Android.Tools.Bytecode for the Module Attribute, the CONSTANT_Module_info structure, and any other relevant module-related structures.
  • Figure out what //@visibility value should be used for "public types in a non-exported package"; should it be internal? kotlin-internal? Other? See also: 678c4bd, which added the kotlin-internal visibility
  • generator changes to deal with the new visibility value, if necessary.
jonpryor pushed a commit that referenced this issue Apr 11, 2023
Context: https://repo1.maven.org/maven2/org/jetbrains/annotations/24.0.1/annotations-24.0.1.jar
Context: #1096

Some AndroidX packages contain a file called `module-info.class` that
uses unsupported `.class` constructs.  It contains metadata for
[Java Modules][0].

![image](https://user-images.githubusercontent.com/179295/229861846-cbd04239-9d7b-470b-ae6c-844713009104.png)

`class-parse` emits this XML fragment for `module-info.class`:

	<package
	    name=""
	    jni-name="">
	  <class
	      abstract="false"
	      deprecated="not deprecated"
	      final="false"
	      name="module-info"
	      jni-signature="Lmodule-info;"
	      source-file-name="module-info.java"
	      static="false"
	      visibility="" />
	</package>

When we try to process this `<class/>`, `generator` emits the warning:

	warning BG8605: The Java type '' could not be found (are you missing a Java reference jar/aar or a Java binding library NuGet?)

This is neither useful nor actionable.

Ignore this file in `class-parse`, until we can properly parse it.

TODO: Issue #1096

[0]: https://www.oracle.com/corporate/features/understanding-java-9-modules.html
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Apr 11, 2023
jonpryor added a commit to jonpryor/java.interop that referenced this issue Apr 12, 2023
Fixes: dotnet#1096

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

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 `internal`.

`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="internal" />

Note that `com.xamarin.internal.PublicClassNotInModuleExports` is now
shown as `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
@jonpryor
Copy link
Member Author

After discussion, the //*/@visibility value should be kotlin-internal, as that's just easier. See also this suggested commit message for PR #793, which sadly didn't make it into the final commit:

Note: we introduce and use a new `//*/@visibility` value of
`kotline-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*.

jonpryor added a commit to jonpryor/java.interop that referenced this issue Apr 20, 2023
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 an `//*/@visibility` value of `kotlin-internal`?  From a
[suggestion][2] for the commit message of 678c4bd, which was sadly
overlooked in the final draft:

> Note: we introduce and use a new `//*/@visibility` value of
> `kotline-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="internal" />

Note that `com.xamarin.internal.PublicClassNotInModuleExports` is now
shown as `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 added a commit to jonpryor/java.interop that referenced this issue Apr 20, 2023
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 added a commit to jonpryor/java.interop that referenced this issue Apr 20, 2023
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 added a commit that referenced this issue Apr 24, 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
Context: b274a67

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, then update `ClassPath` so that it updates all
`public` types *outside* of the "exported" packages to have an
`//*/@annotated-visibility` attribute value of `module-info`.
(See also commit b274a67, which added `//*/@@annotated-visibility`.)
If there is *already* an `//*/@annotated-visibility` value, then we
*append* ` module-info` to the attribute value.

We use `//*/@annotated-visibility` because we are concerned about
introducing an ABI break into AndroidX-related bindings because of
type visibility changes.  If this isn't a concern, it should be
possible to use Metadata to remove those types:

	<attr path="//class[@annotated-visibility]"
	    name="visibility">kotlin-internal</attr>
	<attr path="//interface[@annotated-visibility]"
	    name="visibility">kotlin-internal</attr>

`class-parse` command-line parsing has been altered.  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 annotated-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
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.

`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"
	      …
	      annotated-visibility="module-info" />

Note that `com.xamarin.internal.PublicClassNotInModuleExports` now
has an XML attribute `annotated-visibility="module-info"`.

Aside: the commit message of 678c4bd sadly overlooked this
[clarification][2] for why `kotlin-internal` was introduced:

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

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]: #793 (comment)
jonpryor pushed a commit to dotnet/android that referenced this issue 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants