Clarifying record reflective support

[Alex Buckley]

On 12/3/2019 8:35 AM, John Rose wrote:
> I have two concerns concerning JVM behavior:
> 
> 1. Keep class file loading fast and simple.  Don’t go beyond precedent in structure checking.
> The current implementation is good from this POV; it just ensures basic referential
> integrity at the constant pool level, plus shallow syntax checking of names and descriptors.

Yes, integrity+shallow_syntax checks are the essence of format checking 
at class load time. 
http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jvms.html#jvms-4.7 
currently specifies that Record undergoes format checking at class load 
time.

> Attributes are bundled up for later, as usual.

Checking which sub-attributes appear in `attributes` tables is also part 
of format checking. In keeping with the "shallow" mantra, only a name 
check of the attribute is needed at load time. ("This record component 
has an attribute which might be total junk but is called Signature, so 
PASS, but also has an attribute which might be well-formed but is called 
Code, so FAIL.")

> 2. Perform deeper checks only when reflection is performed.  This is when things get
> sticky.  There are many things which can go wrong during reflection on a class file
> that has passed all the (relative shallow) load-time checks.  If a field or method descriptor
> mentions a type which cannot be loaded, reflecting that field will fail, even though the
> bytecodes are perfectly serviceable (as long as the unloaded type is only used to pass
> nulls in bytecode execution).  The same is true for non-loadable types in InnerClasses.
> If a generic signature syntax is wrong, you find out during reflection.  We could try to
> test for such things earlier, but that would slow down application startup, which is a
> weak spot for us, that we don’t want to weaken further.

I agree with the above (setting aside anything to do with Signature 
because that attribute is a mess). Deep syntax checking and deep 
sub-attribute checking is for reflection time, not class load time.

> The application of point #2 to records is that a record component which has a
> non-loadable type descriptor should fail (with a low-level error) on reflection,
> even though the record can be used for normal bytecode execution.

Yes.

> Likewise,
> if a bogus record component mentions a field that doesn’t exist, this should
> (I think) fail at reflection time; there’s no reason to check for this particular
> error at earlier class load time.  And so on for any other structural problems
> that can happen with records.

Yes.

(Chris can probably stop reading here.)

> The upshot of this is that reflective APIs should be allowed to throw low-level
> errors if the class file has a deep error in it.  Such errors cannot *all* be ruled
> out at class load time (in principle, not just practically; details on request),
> and so reflection *must* be allowed to be an incomplete operation.
> 
> This subtly affects the reflective API points which return information that
> depends on classfile attributes (when, as is often the case, such attributes cannot
> be fully validated at class load time).  Such API points as getInnerClasses and
> getRecordComponents must be allowed to throw errors for “partially broken”
> class files.
> 
> (Do we need a specific term for “partially broken” here? We might say
> “reflectively invalid” I suppose.)
> 
> The javadoc is relatively silent about reflectively invalid class files,
> but don’t take that as evidence that failure is impossible.

The four paragraphs above suggest that the API spec should document 
"low-level errors" about invalid class files (that is, invalid according 
to deep, reflection-time, checks). The next paragraph, however, suggests 
that the API spec doesn't need to document such errors/failure modes:

> Should getRC document failure modes?  Maybe, but if they are just the
> same as those affecting getMethods, getInnerClasses, etc., there’s no need
> to.  New failure modes, such as “record component not found”, might be
> documented, maybe.  (I don’t see this happening in the JVM code, which
> is a good sign.)  But *all* such errors are artifacts introduced by
> broken tools, and it appears that they are sufficiently rare that they
> can be swept under the rug, in the javadoc.  After all, errors need not
> be documented, especially endemic ones like OOME and SOE, and
> the reflection doc creates CNFE with a similar level of silence.

Alex