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

[Jonathan Gibbons]

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
>>>>
>>>>
>>>>
>>>
>>
>