[generator] Only use [JniTypeSignatureAttribute] for JavaInterop1.

[jpobst]

Registering types that are annotated with [JniTypeSignatureAttribute] to add to the generator type symbol table causes issues, as the Java*Array types from Java.Interop get added:

[JniTypeSignature ("B", ArrayRank = 1, IsKeyword = true, GenerateJavaPeer = false)]
public class JavaSByteArray { ... }

This type is added to the type symbol table with a key of B for the JNI Byte type, but the symbol table doesn't deal in JNI types, thus this type will get chosen when a method parameter or return type has a generic type named "B" (or a global package type "B").

When binding AndroidX with .NET 9, this causes the error:

System.NotSupportedException: Unable to generate setter parameter list in method SetId in managed type Java.Interop.JavaSByteArray

Java Documentation

A workaround is to only use [JniTypeSignatureAttribute] for the JavaInterop1 generator target.

This commit plumbs CodeGenerationOptions deeper into generator and a new ApiImporterOptions into Java.Interop.Tools.JavaTypeSystem that is used to determine whether we should be importing [JniTypeSignatureAttribute] types.

dotnet/android test PR: dotnet/android#9406

[jonpryor]

Shouldn't this overload instead take ApiImporterOptions so that it can check ApiImporterOptions. SupportedRegisterAttributes, a'la GetRegisterAttribute() within ManagedApiImporter.cs?

[jpobst]

The issue is that we're working in 2 different assemblies with both of these seemingly identical changes.

In generator, we have plumbed CodeGenerationOptions through most of the assembly, including the main public entry points to CecilApiImporter: CreateClass and CreateInterface. In this case it makes the most sense to continue using our exist "options" class and plumb it a little deeper.

ApiImporterOptions is in Java.Interop.Tools.JavaTypeSystem, which did not previously have an "options" class so one had to be added. As JavaTypeSystem is a more general assembly it should not have the concept of JavaInterop1/XAJavaInterop1, so the shape of the "options" class is a little different.

[jonpryor]

Should this be SupportedTypeMapAttributes? So that we could have a SupportedMethodMapAttributes (should we need one)?

[jonpryor]

@jpobst: This should also add a unit test of some form which demonstrates the problem we're trying to avoid here, to reduce the chance of future AndroidX breakage.

[jpobst]

This should also add a unit test of some form which demonstrates the problem we're trying to avoid here, to reduce the chance of future AndroidX breakage.

This would be ideal, but I'm not sure exactly what we can test. The issue is "B" meaning byte (and all other primitive types) shouldn't be added to the symbol table, but "B" is valid to be added to the table as a global type named "B".

I suspect we're going to end up revisiting this later. We have fixed this for XAJavaInterop1 but it seems like JavaInterop1 is still broken by the same issue.

[jonpryor]

@jpobst: we'll be able to add an end-to-end test to Java.Base-Tests once #1261 is merged, allowing us to add <JavaCompile Include="B.java" />. We'll fix JavaInterop1 for .NET 10.

[jonpryor]

@jpobst: one thing confuses me with the described scenario: B is a generic type parameter! From WorkRequest.Builder:

public abstract /* partial */ class androidx.work.WorkRequest.Builder<
    B extends androidx.work.WorkRequest$Builder<B, ?>,
    W extends androidx.work.WorkRequest>
{
    public final B setId(java.util.UUID);
}

Yes, if someone had public class B{} in the global package, I can see how that would be a problem, but here? Generic type parameters shouldn't be in the symbol table…should they?

[jonpryor]

@jpobst: another point of confusion: the error message:

Unable to generate setter parameter list in method SetId in managed type Java.Interop.JavaSByteArray

implies it's trying to emit a property. However, there is no WorkRequest.Builder.getId() method, so why would we be trying to turn SetId() into a property?

When I git grep 'Unable to generate setter', the only match is for Fields:

java-interop/tools/generator/Java.Interop.Tools.Generator.ObjectModel/Field.cs

Line 63 in b656f7f

string.Format ("Unable to generate setter parameter list {0}", context.ContextString));

which makes a bit more sense -- this isn't dealing with "method properties", so there doesn't need to be a getId() method -- and significantly less sense: how are we in Field.cs for the method setId(UUID)?

Which leads me to believe that I don't understand what "in method SetId in managed type Java.Interop.JavaSByteArray" is actually saying, which makes me feel like we don't fully understand this scenario.

[jonpryor]

Draft commit message:

Context: https://github.com/dotnet/java-interop/issues/1268
Context: 78d59371a9e27aa601bc32a07a529d09e2f7c796

Commit 78d59371 included this snippet:

> Update `generator` to support using `Java.Base.dll` as a referenced
> assembly for binding purposes, in particular by supporting the use
> of `[JniTypeSignatureAttribute]` on already bound types.

Aside: the symbol table used by `generator` uses the "full Java name"
as the key.  Which means that after 78d59371, types with
`JniTypeSignatureAttribute.SimpleReference` values are now added to
the symbol table, which in turn means that when given:

	// C#
	[JniTypeSignature ("B", ArrayRank=1, IsKeyword=true, GenerateJavaPeer=false)]
	public sealed partial class JavaSByteArray : JavaPrimitiveArray<SByte> {
	}

when `generator` needs to find type information corresponding to the
Java name of `B`, it will find `JavaSByteArray`!

This apparently breaks binding the [`androidx.work:work-runtime`][0]
Maven package, as [`androidx.work.WorkRequest.Builder.setId(UUID)`][1]
has a return type of `B`, a generic type parameter:

	// Java
	public abstract /* partial */ class androidx.work.WorkRequest.Builder<
	    B extends androidx.work.WorkRequest$Builder<B, ?>,
	    W extends androidx.work.WorkRequest>
	{
	  public final B setId(java.util.UUID);
	}

As a workaround, only use `[JniTypeSignature]` when using
`generator --codegen-target=JavaInterop1`.
`generator --codegen-target=XAJavaInterop1` will only look for
`[RegisterAttribute]` when constructing the symbol table.

This commit plumbs `CodeGenerationOptions` deeper into `generator`
and a new `ApiImporterOptions` into `Java.Interop.Tools.JavaTypeSystem`
that is used to determine whether we should be importing
`[JniTypeSignatureAttribute]` types.

TODO? dotnet/java-interop#1268

[0]: https://maven.google.com/web/index.html#androidx.work:work-runtime
[1]: https://developer.android.com/reference/androidx/work/WorkRequest.Builder?hl=en#setId(java.util.UUID)

[jpobst]

Yes, if someone had public class B{} in the global package, I can see how that would be a problem, but here? Generic type parameters shouldn't be in the symbol table…should they?

No, that part is still wrong (yay generics!) and this API likely won't bind because it won't find a "B" in the table. But at least it will fail in the expected way, get invalidated, and removed from the final output rather than crash generator.

Which leads me to believe that I don't understand what "in method SetId in managed type Java.Interop.JavaSByteArray" is actually saying, which makes me feel like we don't fully understand this scenario.

Referencing Java.Interop.JavaSByteArray causes us to attempt to validate that type, so the actual exception happens while trying to validate the Java.Interop.JavaSByteArray.ArrayMarshaler field.

This leads to the question "why is generator even aware of this field since it isn't marked with [Register]?".

The answer surprises me, as it appears we only import fields that do not have a [Register] attribute:

java-interop/tools/generator/Java.Interop.Tools.Generator.Importers/CecilApiImporter.cs

Lines 44 to 46 in b656f7f

	foreach (var f in t.Fields)
	if (!f.IsPrivate && GetRegisterAttribute (f.CustomAttributes) == null)
	klass.AddField (CreateField (f));

This feels wrong, and I assumed I must have broken something in my past generator refactorings, but this logic still exists since the beginning of OSS Xamarin:

java-interop/tools/generator/ClassGen.cs

Lines 45 to 47 in 4e364ba

	foreach (var f in t.Fields)
	if (!f.IsPrivate && !f.CustomAttributes.Any (ca => ca.AttributeType.FullNameCorrected () == "Android.Runtime.RegisterAttribute"))
	AddField (new ManagedField (f));

So either it is correct, or it's wrong but doesn't really matter, as we don't really need to know about inherited fields since they don't play a part in binding a subclass. 🤷

[jonpryor]

@jpobst: then re-upping a previous comment: can we unit test this?

I thought (wrongly?) that a generic type named B would trigger it, so I tried adding java.util.Iterator<E>, renaming E to B, to tests/generator-Tests/expected.xaji/GenericArguments/GenericArguments.xml:

diff --git a/tests/generator-Tests/expected.ji/GenericArguments/GenericArguments.xml b/tests/generator-Tests/expected.ji/GenericArguments/GenericArguments.xml
index f9387a08..04cf4838 100644
--- a/tests/generator-Tests/expected.ji/GenericArguments/GenericArguments.xml
+++ b/tests/generator-Tests/expected.ji/GenericArguments/GenericArguments.xml
@@ -4,6 +4,18 @@
     <class abstract="false" deprecated="not deprecated" final="false" name="Object" static="false" visibility="public">
     </class>
   </package>
+  <package name="my.java.util">
+    <interface abstract="true" deprecated="not deprecated" final="false" name="Iterator" static="false" visibility="public">
+      <typeParameters>
+        <typeParameter name="B">
+          <genericConstraints>
+            <genericConstraint type="java.lang.Object" />
+          </genericConstraints>
+        </typeParameter>
+      </typeParameters>
+      <method abstract="true" deprecated="not deprecated" final="false" name="next" native="false" return="B" jni-return="TB;" static="false" synchronized="false" visibility="public" />
+    </interface>
+  </package>
   <package name="com.google.android.exoplayer.drm">
     <interface abstract="true" deprecated="not deprecated" final="false" name="ExoMediaCrypto" static="false" visibility="public">
       <method abstract="true" deprecated="not deprecated" final="false" name="requiresSecureDecoderComponent" native="false" return="boolean" static="false" synchronized="false" visibility="public">
diff --git a/tests/generator-Tests/expected.xaji/GenericArguments/GenericArguments.xml b/tests/generator-Tests/expected.xaji/GenericArguments/GenericArguments.xml
index f9387a08..04cf4838 100644
--- a/tests/generator-Tests/expected.xaji/GenericArguments/GenericArguments.xml
+++ b/tests/generator-Tests/expected.xaji/GenericArguments/GenericArguments.xml
@@ -4,6 +4,18 @@
     <class abstract="false" deprecated="not deprecated" final="false" name="Object" static="false" visibility="public">
     </class>
   </package>
+  <package name="my.java.util">
+    <interface abstract="true" deprecated="not deprecated" final="false" name="Iterator" static="false" visibility="public">
+      <typeParameters>
+        <typeParameter name="B">
+          <genericConstraints>
+            <genericConstraint type="java.lang.Object" />
+          </genericConstraints>
+        </typeParameter>
+      </typeParameters>
+      <method abstract="true" deprecated="not deprecated" final="false" name="next" native="false" return="B" jni-return="TB;" static="false" synchronized="false" visibility="public" />
+    </interface>
+  </package>
   <package name="com.google.android.exoplayer.drm">
     <interface abstract="true" deprecated="not deprecated" final="false" name="ExoMediaCrypto" static="false" visibility="public">
       <method abstract="true" deprecated="not deprecated" final="false" name="requiresSecureDecoderComponent" native="false" return="boolean" static="false" synchronized="false" visibility="public">

This didn't trigger/generate System.NotSupportedException: Unable to generate setter parameter list.

I think the problem is that GenericArguments.cs builds bindings strictly from API.xml, and doesn't involve Cecil imports.

Do we have any existing tests which involve generator against an existing Mono.Android.dll + Java.Interop.dll?

It doesn't look like we do until #1261…