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

Allow metadata to work on Kotlin visibility #790

Closed
jpobst opened this issue Feb 3, 2021 · 0 comments · Fixed by #793 or dotnet/android#5757
Closed

Allow metadata to work on Kotlin visibility #790

jpobst opened this issue Feb 3, 2021 · 0 comments · Fixed by #793 or dotnet/android#5757
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jpobst
Copy link
Contributor

jpobst commented Feb 3, 2021

With our enhanced Kotlin support we now understand the way things look in Kotlin, and not just how they look in Java. This means we can do stuff like hide Kotlin internal types even though they appear as public when decompiled in Java.

internal class MyClass { }

This is correct and is automatically doing "The Right Thing". However, a user may not actually want "The Right Thing" done. Because Java exposes internal Kotlin classes and methods as public, there seems to be many cases in the wild where the types are used by consuming Java code despite the original intention.

This means there are cases where users do want to bind Kotlin internal things. The ideal scenario would be they could write a metadata transform to override visibility, just like they can do with Java types/members. But these Kotlin fixups are done in class-parse and not generator, and thus it cannot be fixed with metadata. In fact, there is currently no way to "fix" this scenario other than changing the Kotlin code and recompiling. (Users often do not have this ability and are simply using a 3rd party .jar.)

Proposal

Extend our api.xml format to allow visibility='kotlin-internal'.

Today, class-parse marks internal members as private, which prevents class-parse from emitting them. Instead, we should allow them to be emitted with visibility='kotlin-internal'. Once in the api.xml, this would give users a chance to change the visibility to public if desired:

<attr path="/api/package[@name='com.example']/class[@name='MyClass']" name="visibility">public</attr>

generator would be modified to ignore kotlin-internal members when importing the api.xml file. (This happens after the XPath transforms are performed on the metadata file so user modifications will be taken into account.)

@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Feb 3, 2021
@jonpryor jonpryor added this to the Under consideration milestone Feb 11, 2021
jonpryor pushed a commit that referenced this issue Mar 2, 2021
…793)

Fixes: #790

Kotlin compiled for Java Bytecode is an interesting beast, as it has
features which aren't directly supported by Java bytecode.

One such feature is visibility: Kotlin supports an [`internal`][0]
visibility on types and members:

	internal class Example

Java doesn't have a direct equivalent to `internal`; instead,
Java Bytecode uses a visibility of *`public`*:

	% javap Example.class 
	public final class Example {
	  public Example();
	}

Commit 439bd83 added support to `Xamarin.Android.Tools.Bytecode.dll`
for parsing Kotlin metadata.  The result is that Kotlin `internal`
are marked as *`private`*, which causes them to be *skipped* and not
present within `api.xml`, because `class-parse`
[doesn't write out `private` members][1].

The result was that `Metadata.xml` could not be used to mark Kotlin
`internal` types as `public`, as they didn't exist within `api.xml`,
and thus couldn't serve as a "target" for `Metadata.xml`.

Improve support for using `Metadata.xml` to update Kotlin `internal`
types by making the following changes:

  * `XmlClassDeclarationBuilder` now emits *all* types, even
    `private` types.  This includes Kotlin `internal` types which
    were changed to have `private` visibility.

  * Kotlin `internal` members are emitted into `api.xml` with a new
    `//*/@visibility` value of `kotlin-internal`.

These changes allow the Kotlin declaration:

	internal class Example {
	    public fun pubFun() {}
	    internal fun intFun() {}
	}

to be emitted into `api.xml` a'la:

	<class name="Example" visibility="private" …>
	  <method name="pubFun" visibility="public" …/>
	  <method name="intFun$main" visibility="kotlin-internal" …/>
	</class>

[0]: https://kotlinlang.org/docs/visibility-modifiers.html#modules
[0]: https://github.com/xamarin/java.interop/blob/b46598a254c20060b107312564e0ec0aee9e33d6/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs#L32-L33
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Mar 18, 2021
Fixes: dotnet/java-interop#790

Changes: dotnet/java-interop@bba1f07...3824b97

  * dotnet/java-interop@3824b974: [java-interop] Windows build system support (dotnet#816)
  * dotnet/java-interop@94c0c709: Bump to xamarin/xamarin-android-tools/main@554d45a (dotnet#813)
  * dotnet/java-interop@5c756b14: [Java.Interop-PerformanceTests] Support .NET Core 3.1 (dotnet#808)
  * dotnet/java-interop@daec07b6: [build] Fix various warnings (dotnet#812)
  * dotnet/java-interop@678c4bd2: [class-parse, generator] Allow showing Kotlin internals via metadata (dotnet#793)
  * dotnet/java-interop@cd4c8f80: [jnienv-gen] Generate a header file for the native functions (dotnet#809)
  * dotnet/java-interop@69767c1a: [param-name-importer] Fix NSE when updating JavaApi.AllPackages (dotnet#807)
  * dotnet/java-interop@a666a6f9: [Java.Runtime.Environment] Partial support for .NET Core (dotnet#804)
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Mar 18, 2021
Fixes: dotnet/java-interop#790

Changes: dotnet/java-interop@bba1f07...3824b97

  * dotnet/java-interop@3824b974: [java-interop] Windows build system support (dotnet#816)
  * dotnet/java-interop@94c0c709: Bump to xamarin/xamarin-android-tools/main@554d45a (dotnet#813)
  * dotnet/java-interop@5c756b14: [Java.Interop-PerformanceTests] Support .NET Core 3.1 (dotnet#808)
  * dotnet/java-interop@daec07b6: [build] Fix various warnings (dotnet#812)
  * dotnet/java-interop@678c4bd2: [class-parse, generator] Allow showing Kotlin internals via metadata (dotnet#793)
  * dotnet/java-interop@cd4c8f80: [jnienv-gen] Generate a header file for the native functions (dotnet#809)
  * dotnet/java-interop@69767c1a: [param-name-importer] Fix NSE when updating JavaApi.AllPackages (dotnet#807)
  * dotnet/java-interop@a666a6f9: [Java.Runtime.Environment] Partial support for .NET Core (dotnet#804)

Note: dotnet/java-interop@3824b974 updated
Java.Interop/src/java-interop to use `_WINDOWS`, not `WINDOWS`.
Define `_WINDOWS` when building for Windows as well.
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Mar 18, 2021
Fixes: dotnet/java-interop#790

Changes: dotnet/java-interop@bba1f07...3824b97

  * dotnet/java-interop@3824b974: [java-interop] Windows build system support (dotnet#816)
  * dotnet/java-interop@94c0c709: Bump to xamarin/xamarin-android-tools/main@554d45a (dotnet#813)
  * dotnet/java-interop@5c756b14: [Java.Interop-PerformanceTests] Support .NET Core 3.1 (dotnet#808)
  * dotnet/java-interop@daec07b6: [build] Fix various warnings (dotnet#812)
  * dotnet/java-interop@678c4bd2: [class-parse, generator] Allow showing Kotlin internals via metadata (dotnet#793)
  * dotnet/java-interop@cd4c8f80: [jnienv-gen] Generate a header file for the native functions (dotnet#809)
  * dotnet/java-interop@69767c1a: [param-name-importer] Fix NSE when updating JavaApi.AllPackages (dotnet#807)
  * dotnet/java-interop@a666a6f9: [Java.Runtime.Environment] Partial support for .NET Core (dotnet#804)

Note: dotnet/java-interop@3824b974 updated
Java.Interop/src/java-interop to use `_WINDOWS`, not `WINDOWS`.
Define `_WINDOWS` when building for Windows as well.
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Mar 18, 2021
Fixes: dotnet/java-interop#790

Changes: dotnet/java-interop@bba1f07...3824b97

  * dotnet/java-interop@3824b974: [java-interop] Windows build system support (dotnet#816)
  * dotnet/java-interop@94c0c709: Bump to xamarin/xamarin-android-tools/main@554d45a (dotnet#813)
  * dotnet/java-interop@5c756b14: [Java.Interop-PerformanceTests] Support .NET Core 3.1 (dotnet#808)
  * dotnet/java-interop@daec07b6: [build] Fix various warnings (dotnet#812)
  * dotnet/java-interop@678c4bd2: [class-parse, generator] Allow showing Kotlin internals via metadata (dotnet#793)
  * dotnet/java-interop@cd4c8f80: [jnienv-gen] Generate a header file for the native functions (dotnet#809)
  * dotnet/java-interop@69767c1a: [param-name-importer] Fix NSE when updating JavaApi.AllPackages (dotnet#807)
  * dotnet/java-interop@a666a6f9: [Java.Runtime.Environment] Partial support for .NET Core (dotnet#804)

Note: dotnet/java-interop@3824b974 updated
Java.Interop/src/java-interop to use `_WINDOWS`, not `WINDOWS`.
Define `_WINDOWS` when building for Windows as well.
jonpryor added a commit to dotnet/android that referenced this issue Mar 23, 2021
Fixes: dotnet/java-interop#790

Changes: dotnet/java-interop@bba1f07...a3de91e

  * dotnet/java-interop@a3de91ef: [ci] Make VC++ toolchain optional (#820)
  * dotnet/java-interop@3824b974: [java-interop] Windows build system support (#816)
  * dotnet/java-interop@94c0c709: Bump to xamarin/xamarin-android-tools/main@554d45a (#813)
  * dotnet/java-interop@5c756b14: [Java.Interop-PerformanceTests] Support .NET Core 3.1 (#808)
  * dotnet/java-interop@daec07b6: [build] Fix various warnings (#812)
  * dotnet/java-interop@678c4bd2: [class-parse, generator] Allow showing Kotlin internals via metadata (#793)
  * dotnet/java-interop@cd4c8f80: [jnienv-gen] Generate a header file for the native functions (#809)
  * dotnet/java-interop@69767c1a: [param-name-importer] Fix NSE when updating JavaApi.AllPackages (#807)
  * dotnet/java-interop@a666a6f9: [Java.Runtime.Environment] Partial support for .NET Core (#804)

Note: dotnet/java-interop@3824b974 updated
Java.Interop/src/java-interop to use `_WINDOWS`, not `WINDOWS`.
Define `_WINDOWS` when building for Windows as well.
@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
2 participants