Clarifying record reflective support

[Maurizio Cimadamore]

Hi Chris,
I like your proposal; first, it makes isRecord/getRecordComponents in 
sync with other similar pair of methods like isArray/getComponentType. 
Secondly, it allows tighter tests to be written e.g. by the JCK team, 
which is, I think, where the value really is.

In your updated sped we say:

"Returns {@code true} if and only if this class is a record class."

Which is fine, but it doesn't define precisely how to establish as to 
whether a class _is_ a record class.

That is fine, I think, from a spec perspective, but I think that, moving 
forward, the implementation should be more deeply tested to make sure 
that the definition of what's a record and what's not remain stable over 
time. More specifically, while establishing as to whether a class 
extends j.l.Record is a straightforward binary check, ensuring that the 
record attribute is present leads to problems - because the attribute 
could be there, but parts of it might not be well-formed (e.g. one of 
the signature attribute point to an invalid CP entry). In such cases we 
need (at least) an internal agreement of what should happen - both at 
JVM classfile parse time _and_ at javac classfile parse time (although I 
realize these might implementation details).

In other words, I'm a bit concerned because, e.g. in enums we could 
piggy back on ACC_ENUM, which is another straightforward binary check. 
Here we're relying on a classfile attribute and, more specifically, on 
what happens when wrong bits of such attributes are discovered by the 
JVM; this in principle opens up issues where different VM 
implementations could have different recovery strategy, which might in 
turn lead to different answer to Class::isRecord.

The only way I see to tighten this up a bit is for the VM to reject a 
class whose record attribute contains wrong bits, rather than throw the 
attribute on the floor (like it happens with annotations).

Maurizio


On 03/12/2019 11:26, Chris Hegarty wrote:
> A number of issues and/or concerns have been come up recently relating
> to the reflective support for records. These arose when finalizing and
> completing the runtime CSR. Taken together they seem to lead back to a
> few small, but significant, omissions in the spec that would be good to
> tighten up. It is important that it be possible to reflectively reason
> about record classes in a way that is unambiguous and provides
> certainty that the record class is well-formed.
>
> For a class to be a record class, then:
>
>    1) It's direct superclass must be java.lang.Record, and
>    2) It must have a Record attribute.
>
> The entry point to the reflective API for records is the two methods:
> Class::isRecord and Class::getRecordComponents.
>
> The isRecord method should, in it's specification, guaranteed both of
> point 1 and point 2 above. That is to say, for a class to be considered
> a record class, then its direct superclass must be java.lang.Record, and
> it must have a Record attribute. The implementation already behaves
> this way, but the specification should require it.
>
> The getRecordComponents method currently returns an empty array for both
> a record class with no components and a non-record class. We thought it
> kinda nice to be able to avoid returning null, but with hindsight I
> think that it would be better to remove this potential ambiguity. The
> getRecordComponents method should only return a non-null value if both
> point 1 ( the class is a record class ) and point 2 above are true.
> There are many other null returning methods in Class, so this is not
> unusual or out of place. The implementation only requires a minor change
> to support this ( return null for non-record classes ).
>
> The most significant part of the changes proposed are to the
> specification, so I've included that here inline. The proposed changes
> tightly couple the pair of methods as part of their specification,
> something that will hopefully be cleaner to do (or even unnecessary)
> when we have full pattern matching.
>
>
> /src/java.base/share/classes/java/lang/Class.java
>
>       /**
>        * {@preview Associated with records, a preview feature of the Java language.
>        *
>        *           This method is associated with <i>records</i>, a preview
>        *           feature of the Java language. Preview features
>        *           may be removed in a future release, or upgraded to permanent
>        *           features of the Java language.}
>        *
>        * Returns {@code true} if and only if this class is a record class.
> -     * It returns {@code false} otherwise. Note that class {@link Record} is not a
> -     * record type and thus invoking this method on class {@link java.lang.Record}
> -     * returns {@code false}.
> -     *
> -     * @return true if and only if this class is a record class
> +     *
> +     * <p> The {@linkplain #getSuperclass() direct superclass} of a record
> +     * class is {@code java.lang.Record}. A record class has (possibly empty)
> +     * record components, that is, {@link #getRecordComponents()} returns a
> +     * non-null value.
> +     *
> +     * <p> Note that class {@link Record} is not a record type and thus invoking
> +     * this method on class {@code Record} returns {@code false}.
> +     *
> +     * @return true if and only if this class is a record class, otherwise false
>        * @jls 8.10 Record Types
>        * @since 14
>        */
>       public boolean isRecord() { ... }
>
>       /**
>        * {@preview Associated with records, a preview feature of the Java language.
>        *
>        *           This method is associated with <i>records</i>, a preview
>        *           feature of the Java language. Preview features
>        *           may be removed in a future release, or upgraded to permanent
>        *           features of the Java language.}
>        *
> -     * Returns an array containing {@code RecordComponent} objects reflecting all the
> -     * declared record components of the record represented by this {@code Class} object.
> -     * The components are returned in the same order that they are declared in the
> -     * record header.
> -     *
> -     * @return  The array of {@code RecordComponent} objects representing all the
> -     *          record components of this record. The array is empty if this class
> -     *          is not a record, or if this class is a record with no components.
> +     * Returns an array of {@code RecordComponent} objects representing all the
> +     * record components of this record class, or {@code null} if this class is
> +     * not a record class.
> +     *
> +     * <p> The components are returned in the same order that they are declared
> +     * in the record header. The array is empty if this record class has no
> +     * components. If the class is not a record class, that is {@link
> +     * #isRecord()} returns false, then this method returns null. Conversely, if
> +     * {@link #isRecord()} returns true, then this method returns a non-null
> +     * value.
> +     *
> +     * @return  An array of {@code RecordComponent} objects representing all the
> +     *          record components of this record class, or {@code null} if this
> +     *          class is not a record class
>        * @throws  SecurityException
>        *          ...
>        *
>        * @jls 8.10 Record Types
>        * @since 14
>        */
>       public RecordComponent[] getRecordComponents() { … }
>
>
> -Chris.
>