RFR 8189749: Devise strategy for making source level checks more uniform

[Maurizio Cimadamore]

Thanks - fixed and pushed!

Maurizio


On 04/12/17 17:15, Jonathan Gibbons wrote:
> Yes, that was my expectation.
>
> -- Jon
>
>
> On 12/4/17 4:22 AM, Maurizio Cimadamore wrote:
>> Thanks,
>> I will make the change ahead of the push (w/o submitting another 
>> round) if that's ok
>>
>> Maurizio
>>
>>
>> On 02/12/17 00:29, Jonathan Gibbons wrote:
>>> +1. Very nice.
>>>
>>> Nit: I would suggest changing multi-catch statements to the plural 
>>> form.
>>>
>>> tests/MulticatchNotSupported.java:39: error: multi-catch statement 
>>> is not supported in -source 6
>>>
>>> -- Jon
>>>
>>> On 11/28/2017 06:19 AM, Maurizio Cimadamore wrote:
>>>> Updated webrev here:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/8189749-v2/
>>>>
>>>> I've included a report of the diagnostics that have been tweaked by 
>>>> this webrev here to make sure that singular vs. plural is used 
>>>> effectively:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/8189749-v2/diags.html
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>>
>>>> On 20/10/17 19:07, Maurizio Cimadamore wrote:
>>>>> Hi,
>>>>> this biggie patch allows the treatment of source level checks to 
>>>>> be more uniform. The general problem is explained in details here:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8189749
>>>>>
>>>>> Webrev is here:
>>>>>
>>>>> http://cr.openjdk.java.net/~mcimadamore/8189749/
>>>>>
>>>>> The way I approached this, was to create a first class entity, an 
>>>>> enum called Feature. Each 'feature' has:
>>>>>
>>>>> * a min source level (e.g. what is the min source level required 
>>>>> to compile that feature, meaning that lower levels will result in 
>>>>> errors)
>>>>> * a max source level (e.g. what is the max source level required 
>>>>> to compile that feature, meaning that higher levels will result in 
>>>>> errors)
>>>>> * an optional resource key
>>>>>
>>>>> The first two properties are obviously used to decided if feature 
>>>>> XYZ can be used given source level N. The last property is used to 
>>>>> automate generation of source check diagnostics. Note that this 
>>>>> patch introduce a _single_ source check diagnostic:
>>>>>
>>>>> # 0: message segment (feature), 1: string (found version), 2: 
>>>>> string (expected version)
>>>>> compiler.misc.feature.not.supported.in.source=\
>>>>>    {0} not supported in -source {1}\n\
>>>>>     (use -source {2} or higher to enable {0})
>>>>>
>>>>> And then there's a bunch of feature fragments, example:
>>>>>
>>>>> compiler.misc.feature.modules=\
>>>>>     modules
>>>>>
>>>>> compiler.misc.feature.diamond.and.anon.class=\
>>>>>     ''<>'' with anonymous inner classes
>>>>>
>>>>> compiler.misc.feature.binary.lit=\
>>>>>     binary literals
>>>>>
>>>>> (and many more :-))
>>>>>
>>>>> Since each feature 'knows' its fragment, it is super easy for the 
>>>>> compiler to generate the source level check diagnostic for any 
>>>>> feature. And, if you need to add a new feature, you just need to 
>>>>> add an enum value, and wired it up with the corresponding resource 
>>>>> key - done :-)
>>>>>
>>>>> Since this change affects the way in which source check 
>>>>> diagnostics are generated, quite a lot of changes were required to 
>>>>> adjust expected output in golden files. Aside from those changes, 
>>>>> I also addressed the following issues:
>>>>>
>>>>> * JavaTokenizer was not using SOURCE_LEVEL flags for its 
>>>>> diagnostics, and it was not using the new diagnostic framework to 
>>>>> generate errors, which made the porting harder - so I've fixed that
>>>>> * CALog in Completeness analyzer required a new override for an 
>>>>> AbstractLog::error variant that was probably forgotten (this was 
>>>>> required otherwise CompletenessAnalyzerTest was failing after the 
>>>>> change above)
>>>>> * Log has a 'feature' so that diagnostics marked with the 
>>>>> SOURCE_LEVEL flag can be reported only once per source. But this 
>>>>> check was only looking at the 'head' of a diagnostic, ignoring all 
>>>>> arguments. Since now all source level check diagnostics share the 
>>>>> same head, I needed to tweak the logic to also compare arguments - 
>>>>> or else, one source level check error being reported would also 
>>>>> disable unrelated source level check diags.
>>>>>
>>>>> Then there were a bunch of mechanical translation, for instance 
>>>>> idioms such as:
>>>>>
>>>>> allowSimplifiedVarargs = source.allowSimplifiedVarargs();
>>>>>
>>>>> Were rewritten as:
>>>>>
>>>>> allowSimplifiedVarargs = source.allowed(Feature.SIMPLIFIED_VARARGS);
>>>>>
>>>>> Also, in JavacParser, this idiom:
>>>>>
>>>>> checkTypeAnnotations();
>>>>>
>>>>> has been replaced with
>>>>>
>>>>> checkSourceLevel(Feature.TYPE_ANNOTATIONS);
>>>>>
>>>>> Where checkSourceLevel is a new general routine that works on any 
>>>>> Feature.
>>>>>
>>>>> Finally, where possible, I also got rid of the various boolean 
>>>>> flags of the kind 'allowPrivateMethodInInterfaces' - if the class 
>>>>> already had a 'source' field, the boolean flag can be omitted, and 
>>>>> the check can be performed on the source object itself.
>>>>>
>>>>> Cheers
>>>>> Maurizio
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>