Final nestmates spec

[David Holmes]

Hi John,

On 13/12/2017 7:22 AM, John Rose wrote:
> On Dec 11, 2017, at 11:52 PM, David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
>>
>> I have one query with regards to the processing of the NestMembers 
>> attribute. Originally when I wrote the draft I copied the InnerClasses 
>> text and that included prohibiting duplicate entries in NestMembers. 
>> You dropped that restriction from the spec (though I haven't yet 
>> removed it from the VM). How does that interact with the 
>> Class.getNestMembers() API? Should it return the raw NestMembers 
>> contents (which may include duplicates - including additional 
>> references to "self") or should it return the set of members (ie no 
>> duplicates, including self)?
> 
> My take:
> 
> We have landed on laissez-faire for this kind of format check
> at class load time.  The access checking rules are cleverly
> written so that garbage in the list is ignored.  This is noted
> by the non-normative language in 4.7.29:
> 
>> This array is consulted during access checking (5.4.4). It should 
>> consist of references to other classes and interfaces that belong to 
>> the same run-time package and have NestHost attributes referencing 
>> this class or interface. Items that do not meet this description are 
>> discouraged and will be ignored by access checking.
> 
> There are four kinds of garbage:
> 
> 1. [self] the nest host class itself (the one containing NestMs)
> 2. [outsider] valid classes whose package prefix differs form that of 
> the nest host class
> 3. [duplicate] duplicate entries (either the same CONSTANT_Class or a 
> homonym)
> 4. [not-found] references to classes which will not exist when resolved
> 
> The spec. says these are *allowed* and *ignored*.
> 
> (A fifth kind of garbage, syntax errors in class names, is caught
> by routine processing of CONSTANT_Class items.  Maybe this is worth
> a unit test.  The other four cases are worth a unit test, for that matter.)

A sixth kind of garbage, or an expansion of #2, are classes that don't 
list the current class as their nest host.

> Each of these four cases poses a question for reflection:  Do we hide
> it or expose it as an anomalous entry in the reflected list?
> 
> In an earlier conversation, I think we agree that case 4 should just
> throw an error when reflection is performed, following a similar
> precedent with Class.getDefinedClasses.
> 
> I don't remember our resolution of what to do about 1/2/3.  The overall

I recall no discussion of #1 and #3. For #2, #4 and #6 - ie any listed 
member that is not validated as being a member - we throw exceptions.

   * <p>Each listed nest member must be validated by checking its own
   * declared {@linkplain #getNestHost() nest host}. Any exceptions that 
occur
   * as part of this process will be thrown.

    * @throws LinkageError if there is any problem loading or validating
    * a nest member or its nest host

In practice these will be IncompatibleClassChangeError for #2 and #6, 
and NoClassDefFoundError for #4.

For #5, depending on exactly what you mean by "syntax error", it could 
be rejected at classfile parsing time; or it simply forms the wrong name 
and will then come under #2, #4 or #6.

> choice is to sanitize on reflection or just pass through the garbage.
> Given the action on 4 (just let it hit the fan), I suggest that we should
> also let self, outsider, and duplicate entries show through on reflection.
> 
> Counterargument:  We could sanitize the reflected data, and also sort
> or randomize its order for good measure, to prevent buggy reflection users
> from accidentally depending on irrelevant specifics of classfile input.
> Remember how we got into trouble with reflected method lists, which
> when they were reordered caused some buggy reflectors to break.
> See also iteration order of maps and ImmutableCollections.SALT.
> 
> My recommendation:  Just let the bad data through reflection, and
> rely on javac to avoid putting bad data there in the first place.
> 
> The javadoc could be adorned with weasel words saying that response
> to bad data (of forms 1..4 above) and sequence order are undefined
> and may change without notice.

Recommendation noted. :)

Personally I'd prefer it if the array elements did form a set, but I 
don't want every correct use of this API to pay the cost of ensuring 
that is the case. So some additional words will be needed I think. I'll 
file a RFE to track that change and propose it here.

Thanks,
David

> — John