[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[Paul Sandoz]

Hi David,

Thank you for your patience, i struggled to explain my point.

How about we proceed as is, and as you suggest, i can discuss more with John when he is back from vacation. I think we will have time to revisit if necessary.

My preference would be to store the classes from the class file bytes in a Set, cache in ReflectionData, then obtain the “cloned” array from that cached Set. We can use Set.of, which is efficient for small sizes, for the case of one element we could even erase the cached value to Object.

Thanks,
Paul.

> On May 21, 2018, at 10:50 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Paul,
> 
> On 22/05/2018 2:15 PM, Paul Sandoz wrote:
>> Hi David,
>>> On May 21, 2018, at 5:05 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> Hi Paul,
>>> 
>>> On 22/05/2018 2:39 AM, Paul Sandoz wrote:
>>>>> On May 20, 2018, at 11:32 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>> 
>>>>>> 3984     public Class<?>[] getNestMembers() {
>>>>>> I still think not removing dups is a mistake as it could be a source of subtle bugs. But i doubt at this point i can persuade you or others to change it :-)
>>>>> 
>>>>> Unlikely. :) Again well-formed programs just won't encounter this and it would penalize all well-formed programs.
>>>>> 
>>>> Although those well-formed programs may need to check for dups themselves because they don’t want to rely in implementation details (and they are not aware of the probability of implementations deviating).
>>> 
>>> I'm quite concerned about your level of concern with "dups". This just shouldn't be an issue. While the spec allows for dups javac will never produce them - and file a bug on it if it ever does! Similarly for other compilers - there is no reason to generate duplicate entries.
>>> 
>> Perhaps i am obsessing a little too much, i thought there might be a slight window of opportunity while other related reviews are progressing :-) but i don’t want to block things for 11.
>> My concern, placing my library/API designer hat on, is the specification is saying something very clear and yet on the other hand it's as if we are saying “oh you can ignore that, the specification does not matter, it will never happen in practice”. It feels like the JVM world is intruding too much into the reflection world (see below).
>>> Looking through the JVMS and the defined classfile attributes it seems to me that the annotations[] of RuntimeVisibleAnnotations (et al) doesn't preclude duplicates either. And the bootstrap_methods[] of the BootstrapMethodsAttribute. Also look at the parameters[] of the MethodParametersAttribute**. Do you agree?
>>> 
>>> ** Which even has an explicit note this is not something a JVM implementation has to check.
>>> 
>> I don’t dispute there may be duplicate information in class file bytes, nor am i suggesting the class file bytes for nestmates be changed, nor that the verifier get involved. However, the reflection API is not a direct reflection of those class file bytes. It provides a runtime view of a class and often performs its own computation from and validation of information in the class file bytes.
>> Not all information in the class file bytes is directly accessible via the reflection API, such as BootstrapMethodsAttribute, or MethodParametersAttribute, the latter of which AFAICT is exposed via the Reflection API indirectly via a Parameter[] array returned by Method.getParameters().
>> I am less sure about RuntimeVisibleAnnotations but there is quite a lot of processing performed by the Java reflection code before annotations reach the hands of the developer, and a quick look at some code shows the use of maps keyed by annotation class to the annotation value. And see for example here in AnnotationParser:
>>     if (AnnotationType.getInstance(klass).retention() == RetentionPolicy.RUNTIME &&
>>         result.put(klass, a) != null) {
>>             throw new AnnotationFormatError(
>>                 "Duplicate annotation for class: "+klass+": " + a);
>> http://hg.openjdk.java.net/jdk/jdk/file/95ba3a1dc2b2/src/java.base/share/classes/sun/reflect/annotation/AnnotationParser.java#l110
> 
> Okay I acknowledge your point here about VM view versus Reflection view. It would be easier if we were returning the members in a duplicate-detecting data structure rather than an array (though placing the host at zeroth element complicates that). The sheer effort involved in detecting and removing duplicates from an array is what made me shy away from pushing for that. You can see the EG discussion from here:
> 
> http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2017-December/000464.html
> 
> If you really think this is worth re-opening it would probably be expedient to discuss with John so you both end up on the same page, then let me know the outcome.
> 
> Though I'll also note that we can strengthen the current implementation at any time and just update the @implNote.
> 
> Thanks,
> David
> -----
> 
>>>> Here’s a thought: did you consider caching the nest members in the ReflectionData class? that may be worth doing regardless of dups.
>>> 
>>> No that was not considered. Caching, as you know, is a space-time trade off and we have no data to use to determine whether caching would be of any benefit here. To me that would be a future RFE regardless. (And I don't expect these introspective nest methods to be used much in any case.)
>>> 
>> Yes, agreed, caching can be possible future work.
>> Paul,