Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Oct 8, 2019

(Copied from #467)

Context: #466

While investigating issue #466, I ran across an "oddity" within the
type kotlin/collections/AbstractCollection\$toString\$1.class:

 <api api-source="class-parse">
   <package name="kotlin.collections" jni-name="kotlin/collections">
     <class
       name="AbstractCollection.toString.1"
       jni-signature="Lkotlin/collections/AbstractCollection$toString$1;">
       <implements
         name="kotlin.jvm.functions.Function1"
         name-generic-aware="kotlin.jvm.functions.Function1&lt;E, java.lang.CharSequence&gt;"
         jni-type="Lkotlin/jvm/functions/Function1&lt;TE;Ljava/lang/CharSequence;&gt;;" />
       <method
         name="invoke"
         jni-signature="(Ljava/lang/Object;)Ljava/lang/CharSequence;">
         <parameter
           name="it"
           type="E"
           jni-type="TE;" />
       </method>
     </class>
   </package>
 </api>

(Abbreviated output).

What's bizarre is that the invoke() method references the type
E, but there is no declaration of what E is.

Normally, there'd be a <typeParameters/> element, which is
provided by the Signature annotation.

Note that the above does not contain <typeParameters/>!

On the vague thought that maybe RuntimeVisibleAnnotations were being
used to store generic type information, add support to class-parse
to extract RuntimeVisibleAnnotations and
RuntimeInvisibleAnnotations annotation blobs.

@jpobst jpobst force-pushed the class-parse-annotations branch from 93a8db4 to 4760982 Compare October 8, 2019 19:29
name-generic-aware="java.lang.annotation.Annotation"
jni-type="Ljava/lang/annotation/Annotation;" />
<runtimeVisibleAnnotations>
<annotation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the question here is twofold:

  1. What type of XML output do we want to emit, and
  2. What type of XML output will be easiest for generator to consume?

While <annotation type="..."><value key=="..." /></annotation> is easy to emit, I doubt that this is at all easy to consume. It's useful for human reading of output, but I doubt it's useful for machine reading of output.

I suspect what we might instead want is:

<runtimeVisibleAnnotations>
  <annotation jni-type="Ljava/lang/annotation/Target;">
    <array>
      <enum jni-type="Ljava/lang/annotation/ElementType;" member="TYPE" />
      <enum jni-type="Ljava/lang/annotation/ElementType;" member="FIELD" />
      <enum jni-type="Ljava/lang/annotation/ElementType;" member="CONSTRUCTOR" />
      <enum jni-type="Ljava/lang/annotation/ElementType;" member="METHOD" />
      <enum jni-type="Ljava/lang/annotation/ElementType;" member="PARAMETER" />
      <enum jni-type="Ljava/lang/annotation/ElementType;" member="LOCAL_VARIABLE" />
    </array>
  </annotation>
  <annotation jni-type="Ljava/lang/annotation/Retention;">
    <enum jni-type="Ljava/lang/annotation/RetentionPolicy;" member="SOURCE" />
  </annotation>
</runtimeVisibleAnnotations>

I can imagine this being far more understandable to generator -- and maybe even humans? -- than the mishmash of what I came up with in PR #467, though it also means an associated increase in "bloat"/XML size as well as implementation complexity. (Which is why I didn't do it in #467, but also why I didn't want to merge #467.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am beginning to think we should stick to Agile principles here and not write code that we don't need yet, since we don't know if we'll ever need it or what shape would be best if we do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an excellent point. In that case, do we actually need a separate <value/> element when the //value/@key attribute is always value? Why not just merge into <annotation/>?

<annotation
  type="Ljava/lang/annotation/Target;"
  elements="[Enum(Ljava/lang/annotation/ElementType;.TYPE), Enum(Ljava/lang/annotation/ElementType;.FIELD), Enum(Ljava/lang/annotation/ElementType;.CONSTRUCTOR), Enum(Ljava/lang/annotation/ElementType;.METHOD), Enum(Ljava/lang/annotation/ElementType;.PARAMETER), Enum(Ljava/lang/annotation/ElementType;.LOCAL_VARIABLE)]"
/>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand, the keys have different names which should be preserved:

<annotation type="Lkotlin/Metadata;">
    <value key="mv">[1, 1, 15]</value>
    <value key="bv">[1, 0, 3]</value>
</annotation>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm an idiot and misread this line: https://github.com/xamarin/java.interop/pull/502/files#diff-759d8653993a49e7976b261c1ef45a72R335

return new XElement ("value",

I misread that as the attribute value. Doh!

@jpobst jpobst force-pushed the class-parse-annotations branch from 4760982 to bd25ffe Compare October 9, 2019 18:44
Context: #466

While investigating issue #466, I ran across an "oddity" within the
type `kotlin/collections/AbstractCollection\$toString\$1.class`:

 <api api-source="class-parse">
   <package name="kotlin.collections" jni-name="kotlin/collections">
     <class
       name="AbstractCollection.toString.1"
       jni-signature="Lkotlin/collections/AbstractCollection$toString$1;">
       <implements
         name="kotlin.jvm.functions.Function1"
         name-generic-aware="kotlin.jvm.functions.Function1&lt;E, java.lang.CharSequence&gt;"
         jni-type="Lkotlin/jvm/functions/Function1&lt;TE;Ljava/lang/CharSequence;&gt;;" />
       <method
         name="invoke"
         jni-signature="(Ljava/lang/Object;)Ljava/lang/CharSequence;">
         <parameter
           name="it"
           type="E"
           jni-type="TE;" />
       </method>
     </class>
   </package>
 </api>

(Abbreviated output).

What's *bizarre* is that the `invoke()` method references the type
`E`, but there is no declaration of what `E` *is*.

*Normally*, there'd be a `<typeParameters/>` element, which is
provided by the `Signature` annotation.

Note that the above does *not* contain `<typeParameters/>`!

On the vague thought that maybe `RuntimeVisibleAnnotations` were being
used to store generic type information, add support to `class-parse`
to extract `RuntimeVisibleAnnotations` and
`RuntimeInvisibleAnnotations` annotation blobs.
@jpobst jpobst force-pushed the class-parse-annotations branch from bd25ffe to 1127477 Compare October 9, 2019 18:45
@jonpryor
Copy link
Contributor

jonpryor commented Oct 9, 2019

Proposed commit message:

Context: https://github.com/xamarin/java.interop/issues/466
Context: https://github.com/xamarin/java.interop/pull/467
Context: https://github.com/xamarin/java.interop/pull/499

Add support for parsing the [`RuntimeVisibleAnnotations`][0] and
[`RuntimeInvisibleAnnotations`][1] attributes in Java bytecode.
These attributes store Java programming language [annotations][2]
placed on language constructs such as types, fields, and methods, and
are analogous with C# custom attributes.

Kotlin stores various bits of information in these attributes, and it
will be necessary to parse these attributes to read that information;
see also PR #499.

[0]: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.16
[1]: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.17
[2]: https://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.7

@jonpryor jonpryor merged commit d4d173b into master Oct 9, 2019
@jonpryor jonpryor deleted the class-parse-annotations branch October 9, 2019 19:00
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants