RFR: JDK-8209865: Incorrect 'multiple elements' notes with Elements#getTypeElement and --release & JDK-8209058: Cannot find annotation method 'value()' in type 'Profile+Annotation'

[Jonathan Gibbons]

2 +1's inline.

-- Jon

On 09/20/2018 07:35 AM, Jan Lahoda wrote:
> On 20.9.2018 02:21, Jonathan Gibbons wrote:
>>
>>
>> On 09/19/2018 08:54 AM, Jan Lahoda wrote:
>>> Hi,
>>>
>>> I'd like to ask for a review of two related --release related bugs:
>>>
>>> The first bug is:
>>> https://bugs.openjdk.java.net/browse/JDK-8209865
>>>
>>> Incorrect 'multiple elements' notes with Elements#getTypeElement and
>>> --release
>>>
>>> The issue here is that
>>> Elements.getTypeElement("javax.annotation.processing.Generated")
>>> returns null. The cause is the structure of ct.sym: to share classfile
>>> used for --release 8 and --release 9, the --release 9 is implemented
>>> in a way that every module gets a plain directory with all classfiles
>>> (whether they belong to the given module or not) on its path, and when
>>> compiling, javac picks the correct packages/classes from this
>>> directory based on module exports. But for the "unbound" search using
>>> getTypeElement, all modules are searched, ignoring module exports, so
>>> the Generated class is found in multiple modules.
>>>
>>> The proposed patch drops this ct.sym structure, and creates a simple
>>> module-path-like structure for --release 9+ data:
>>> 9/java.base
>>>  /java.compiler
>>> ...
>>>
>>> 9A/java.base
>>>   /java.compiler
>>> ...
>>>
>>> The paths for a module M for --release N are all directories in the
>>> form ".*N.*/M". These contain only classes that belong to the given
>>> module.
>>>
>>> For --release <= 8, the original approach remains. As a consequence,
>>> there are no shared classfiles between JDK 8 and JDK 9 data, but the
>>> size increase seems acceptable compared to the much cleaner structure
>>> of ct.sym (originally about 6.2MB with JDK 11 data, about 7.9MB with
>>> this patch and JDK 11 data).
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8209865
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8209865/webrev.00/
>>
>> This email is just about the first of your two bugs.
>>
>> So, this works because the data files for the releases remain unchanged,
>> and all you're doing here is changing the way those text files are
>> "compiled" into ct.sym at build time, right, and subsequently read by
>> JDKPlatformProvider at user-compile-time?
>
> Yes.
>
>>
>> Separately, we should really have an (internal) spec for the format of
>> files like ct.cym.   Yes, you're changing code, but it is surprising
>> that there are no comments or any other documentation being updated
>> along with the code changes to modify the format of the file.
>>
>> I understand that you want to move away from the old way of handling
>> modules by introducing a new layer of module name. How hard would it  be
>> to share the data with versions <=8 by ignoring the enclosing module?
>
> Not that hard, a patch doing that is here:
> http://cr.openjdk.java.net/~jlahoda/8209865/webrev.01/
>
> A delta from webrev.00:
> http://cr.openjdk.java.net/~jlahoda/8209865/webrev.delta.00.01/

+1

>
> And a patch fixing the second bug in a different way that does allow 
> sharing the classfiles is here:
> http://cr.openjdk.java.net/~jlahoda/8209058/webrev.01/

+1

>
> The main drawback is that the bootclasspath for --release <= 8 will be 
> longer (as it needs to include "879/java.base", "879/java.desktop", etc.)

I don't see that as a big drawback, since it is now conceptually the 
same across all versions.

>
> Thanks for the comments,
>     Jan

-- Jon


>
>>
>> -- Jon