[class-parse, generator] Allow users to expose Kotlin internal types/members with metadata.

[jpobst]

Fixes #790.

Modify class-parse to emit Kotlin internal types/members so that users can use metadata to change them to public if they wish, giving them the same access they would have if consumed from Java. If visibility is not changed to public by the user, generator will ignore importing these types/members, resulting in the same bindings as today.

Format is the same as existing metadata visibility fixup:

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

class-parse now emits all private types as visibility="private" because the existing generator behavior will ignore any private types, which is what we want. Kotlin internal types were already being changed to private, so they fall into this category.

class-parse now emits Kotlin internal members as visibility="kotlin-internal" because the existing generator behavior is that members marked as internal or private are actually bound as internal/private members. Whether this was ever intentional is debatable, but it is a behavior that user code in the wild relies on that we do not wish to break.

There are a couple of issues with simply allowing members to be bound as internal/private:

• Though the linker would remove them, it is not very desirable to bloat our assemblies and build times with something that isn't intended to be used.
• In testing, there is at least 1 generator bug where attempting to bind a private member results in invalid C# code. This casts doubt that this was ever "supported", and thus there could be an unknown number of other bugs users would begin hitting if we started emitting a bunch of private members.

[jpobst]

Release note:

Kotlin `internal` types are now added to `api.xml` with `visibility='private'` and `internal` members 
are added with `visibility='kotlin-internal'`.  This provides an opportunity to use `metadata` to 
change the `visibility` to `public`, allowing them to be bound.  If `visibility` is not manually changed, 
these Kotlin `internal` types/members will be ignored, matching the existing behavior.

[jonpryor]

I don't understand this change and the related one for "interface": why do we need to check for ./@visibility='internal' here, but not visibility = "" (not set)/private/etc.?

How is non-public visibility handled elsewhere, and why is internal different?

[jpobst]

private members are not emitted by class-parse, so they should never exist in the api.xml. (This is how we hide Kotlin internal types today: we set them to private in class-parse so they do not appear in the api.xml.)

not-set members are a little different. We still import them so they can participate in Java type resolution, and there's some logic about copying package-private interface methods directly to types. However we do not emit them as managed types.

[jonpryor]

not-set members are a little different. … However we do not emit them as managed types.

How does that work? (The 'obvious-to-me' answer would be for e.g. XmlApiImporter.CreateClass() to return null when Visibility=="", but that isn't the case -- there are no early returns in XmlApiImporter.CreateClass().)

Would it make more sense to treat Visibility=="internal" the same way we treat Visibility==""?

Not understanding how "not-set" visibility works is confusing to me, actually; java.util.concurrent.ConcurrentHashMap.CollectionView has no visibility (useful example) and isn't overridden in metadata (useful example) and isn't bound -- all yay! -- but why isn't it bound? I've lost the plot, and can't readily see where/why this type isn't bound.

[jonpryor]

"not-set" visibility is handled by:

https://github.com/xamarin/java.interop/blob/ee7b6bbe382bf408cc1a991e6149819889ad8c6c/tools/generator/CodeGenerator.cs#L236-L283

[jonpryor]

Commit message for review:

Summary:

[class-parse, generator] Allow showing Kotlin internals via metadata (#793)

Body:

Fixes: https://github.com/xamarin/java.interop/issues/790

Context: https://github.com/xamarin/java.interop/issues/789

Commit 439bd839 bound Kotlin `internal` types as if they were Java
`private` types, which had the effect of *removing* them from
`api.xml` *before* `Metadata.xml` is processed, because `class-parse`
[doesn't write out `private` members][0].  Consequently, if you
wanted to mak an `internal` type *`public`* -- because that's what
*Java* sees, and thus may be needed for some interop scenarios --
you couldn't.

Update `class-parse` so that instead of emitting Kotlin `internal`
types and members as `private` members, they are instead emitted with
a `//@visibility` of `kotlin-internal`:

	// Kotlin
	internal class MyClass {
	}

	// api.xml
	<class name="MyClass" visibility="kotlin-internal" … />

This causes the types to be "preserved" within `api.xml`.

Update `generator` so that types and members with a visibility of
`kotlin-internal` are *skipped* by default.  These types shouldn't be
emitted as part of normal operation, as they're not *public* API.

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

[0]: https://github.com/xamarin/java.interop/blob/b46598a254c20060b107312564e0ec0aee9e33d6/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs#L32-L33

[jonpryor]

Alternative idea/question: should we instead update XmlClassDeclarationBuilder so that private members aren't excluded, i.e. remove these lines:

https://github.com/xamarin/java.interop/blob/b46598a254c20060b107312564e0ec0aee9e33d6/src/Xamarin.Android.Tools.Bytecode/XmlClassDeclarationBuilder.cs#L32-L33

which is kinda/sorta equivalent to "reverting" https://github.com/xamarin/monodroid/commit/2c8110e6e7.

@dellis1972 : do you remember why you wanted to skip private members, as done in xamarin/monodroid@2c8110e6e7?

[jpobst]

I am warming up to the idea of putting private things in api.xml. I will need to do some research to determine if that has any unintended consequences for generator.

[jonpryor]

I also prefer the idea of putting private things into api.xml. I look forward to the results of that research. :-)

[jpobst]

Research notes:

• Setting a type to private correctly prevents the type from being emitted and prevents it from interacting with generator because it is dropped immediately after importing:
  • https://github.com/xamarin/java.interop/blob/main/tools/generator/CodeGenerator.cs#L150
• A public type trying to extend a private type correctly issues a warning and is not emitted:
  • warning BG8102: Class 'AndroidX.TvProvider.Media.Tv.Channel2' has unknown base type 'androidx.tvprovider.media.tv.Channel'.
• Setting a type member to private results in the member being emitted with private visibility. While this is "correct" and the linker will remove these members, they will add to assembly size and assembly processing time (including linking) for any pre-link processing.
  • ❌ Setting a method as private will result in the disallowed C#: private virtual unsafe void MyMethod ().
• If a public member overrides a private member, the public member is correctly marked as public virtual instead of public override.

The existing behavior for private types seems like exactly what we want: they are discarded immediately after being imported. 👍

However the existing behavior for private members seems problematic. Ideally we would immediately discard these as well, however we do not. In some cases they generate uncompilable code (private virtual) but other cases likely work (private static).

It is conceivable that a user may be relying on the current private behavior:

• Set generated member to private using metadata.
• Create public member in Additions that chains to the private member.

Update: Verified with Matthew that people do indeed do ^^:

• https://github.com/mattleibow/Microsoft-Band-SDK-Bindings/blob/master/Microsoft.Band/Microsoft.Band.Android/Transforms/Metadata.xml#L71
• https://github.com/mattleibow/Microsoft-Band-SDK-Bindings/blob/master/Microsoft.Band/Microsoft.Band.Android/Tiles/BandTheme.cs

This example provides public Color versions of the bound int methods.

[jonpryor]

I don't think that there's a problem with relying on the linker to remove private members that aren't used. I don't think that assembly sizes will increase significantly.

[jpobst]

Updated code and PR description to reflect the approach we settled on:

• private types are now emitted from class-parse, including Kotlin internal types which have been changed to private.
• Kotlin internal members are emitted from class-parse as kotlin-internal.

[jonpryor]

New & Improved™ commit message?

Fixes: https://github.com/xamarin/java.interop/issues/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 439bd839 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