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

generator Symbol Table semantics #1268

Open
jonpryor opened this issue Oct 21, 2024 · 1 comment
Open

generator Symbol Table semantics #1268

jonpryor opened this issue Oct 21, 2024 · 1 comment
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

Context? #1266
Context: #1267 (comment)

Currently, generators Symbol Table uses Java names as the keys:

AddType (new SimpleSymbol ("IntPtr.Zero", "void", "void", "V"));

Keys will be strings such as Java builtin types like boolean and int, and "fully qualified dotted" Java names such as java.lang.Object and android.app.Activity.

There are (at least?) two issues with this setup:

  1. Nested type semantics
  2. Potential ambiguity with JNI names

For nested type semantics, consider #1267:

[Register ("com/mypackage/FieldClass")]
public class FieldClass : Java.Lang.Object {
    public NestedFieldClass field;

    public class NestedFieldClass : Java.Lang.Object {
    }
}

The above could be a hand-written C# snippet added to a binding assembly, and then referenced by generator when emitting a new set of bindings.

The (long-standing so not necessarily a "real") concern is that when generator processes the above, the symbol table will have:

  • an entry com.mypackage.FieldClass for FieldClass (as per [Register]), and
  • an entry Com.Mypackage.FieldClass.NestedFieldClass for FieldClass.NestedFieldClass`.

This nested name is wrong; it should instead be one of:

  • com.mypackage.FieldClass.NestedFieldClass (append nested name to declaring type name), or
  • Whatever jcw-gen would emit for this type, which could be com.mypackage.FieldClass_NestedFieldClass, or something like that. (Requires testing.)

Then there's #1266: somehow -- we're not quite sure we fully understand yet -- JniTypeSignatureAttribute.SimpleReference is being used as a key for the Symbol Table.

Given:

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

and a Java type:

// 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);
}

then the return value of WorkRequest.Builder.setId() is being detected as JavaSByteArray instead of WorkRequest.Builder.

Aside: how?!

internal static RegisterAttribute? RegisterFromJniTypeSignatureAttribute (CustomAttribute attr)
{
// attr.Resolve ();
RegisterAttribute? r = null;
if (attr.ConstructorArguments.Count == 1)
r = new RegisterAttribute ((string) attr.ConstructorArguments [0].Value, attr);
if (r != null) {
var v = attr.Properties.FirstOrDefault (p => p.Name == "GenerateJavaPeer");
if (v.Name == null) {
r.DoNotGenerateAcw = false;
} else if (v.Name == "GenerateJavaPeer") {
r.DoNotGenerateAcw = !(bool) v.Argument.Value;
}
var isKeyProp = attr.Properties.FirstOrDefault (p => p.Name == "IsKeyword");
var isKeyword = isKeyProp.Name != null && ((bool) isKeyProp.Argument.Value) == true;
var arrRankProp = attr.Properties.FirstOrDefault (p => p.Name == "ArrayRank");
if (arrRankProp.Name != null && arrRankProp.Argument.Value is int rank) {
r.Name = new string ('[', rank) + (isKeyword ? r.Name : "L" + r.Name + ";");
}
}
return r;
}

certainly looks like [JniTypeSignature("B", ArrayRank=1, IsKeyword=true)] should result in a Register attribute with a name of [B. How/where would this value be "cleaned up"?

The scenario described in #1266 should not happen. An idea to do that would be to alter the symbol table keys to instead be JNI type signatures. B would thus only be "byte", while LB; would be a class B{} in the global package, and Ljava/lang/Object; would be java.lang.Object.

jonpryor pushed a commit that referenced this issue Oct 21, 2024
Context: #1266
Context: #1268

While the issue described in #1266 is definitely
an issue, that example was merely setting up the scenario needed to
expose another bug:

When a `Java.Lang.Object` or `Java.Interop.JavaObject` subclass
has a `public` or `internal` field whose type is a nested type:

	// C#
	namespace Com.Mypackage;

	[Register ("com/mypackage/FieldClass")]
	public class FieldClass : Java.Lang.Object {
	    public NestedFieldClass field;
	
	    public class NestedFieldClass : Java.Lang.Object {
	    }
	}

We correctly import the `Field.TypeName` as
`Com.Mypackage.FieldClass.NestedFieldClass`, however we also create
`Field.SetterParameter` with the "same" type, but we do not replace
the `/` nested type separator with a period, resulting in
`Com.Mypackage.FieldClass/NestedFieldClass`.

Later, when validating the `Field`, we successfully find
`Field.TypeName` in the symbol table, but fail to find
`Field.SetterParameter` as the symbol table expects a period nested
type separator.  (Note this only happens because `NestedFieldClass`
does not have a `[Register]` attribute, thus it gets added to the
symbol table with its managed name rather than its Java name.)

This causes `generator` to crash with:

	System.NotSupportedException: Unable to generate setter parameter list in managed type Com.Mypackage.FieldClass
	   at MonoDroid.Generation.Field.Validate(CodeGenerationOptions opt, GenericParameterDefinitionList type_params, CodeGeneratorContext context)
	   at MonoDroid.Generation.GenBase.OnValidate(CodeGenerationOptions opt, GenericParameterDefinitionList type_params, CodeGeneratorContext context)
	   at MonoDroid.Generation.ClassGen.OnValidate(CodeGenerationOptions opt, GenericParameterDefinitionList type_params, CodeGeneratorContext context)
	   at MonoDroid.Generation.GenBase.Validate(CodeGenerationOptions opt, GenericParameterDefinitionList type_params, CodeGeneratorContext context)

Fix this by applying the same `FullNameCorrected()` logic that is
applied to `Field.TypeName`.
jonpryor pushed a commit that referenced this issue Oct 21, 2024
…#1266)

Context: #1268
Context: 78d5937

Commit 78d5937 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 78d5937, 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? #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 jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Oct 22, 2024
@jpobst
Copy link
Contributor

jpobst commented Oct 22, 2024

Aside: how?!

I think we're conflating different processes here that have different codebases. The linked CecilExtensions.cs code is used for jcw-gen, but not for generator. generator simply looks at the first constructor argument for [Register] and uses it:

public static GenBaseSupport CreateGenBaseSupport (TypeDefinition t, CodeGenerationOptions opt)
{
var obs_attr = GetObsoleteAttribute (t.CustomAttributes);
var reg_attr = GetRegisterAttribute (t.CustomAttributes, opt);
var jn = reg_attr != null ? ((string) reg_attr.ConstructorArguments [0].Value).Replace ('/', '.') : t.FullNameCorrected ();
var idx = jn.LastIndexOf ('.');

Thus something like [JniTypeSignature] is not going to work in generator without modification, as it uses the first constructor parameter unmodified which is B:

[JniTypeSignature ("B", ArrayRank=1, IsKeyword=true, GenerateJavaPeer=false)] 

Note for generator purposes we would not want a [JniTypeSignature] with byte to exist either, as byte is a built-in type and no class should be trying to register itself as byte:

// Also incorrect
[JniTypeSignature ("byte", ArrayRank=1, IsKeyword=true, GenerateJavaPeer=false)] 

AddType (new SimpleSymbol ("0", "byte", "sbyte", "B"));


There are (at least?) two issues with this setup:
2. Potential ambiguity with JNI names

I do not think this one is an issue. The generator symbol table is all Java names, all the time. If a JNI name key is added to the table or the table is queried for a JNI name, then the table is being used incorrectly and the caller needs to be fixed. (#1266 fixed a case of this.)

We do have a bug here that the symbol table also should not be queried for a generic type parameter name like B, but that tends to just get swept under the "we don't do generics well" rug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants