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.

Reply via email to