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

[Maurizio Cimadamore]

Thanks for the review - comments inline


On 27/11/17 20:31, Jonathan Gibbons wrote:
> Mostly excellent.
>
> Some nitpicks for your consideration:
>
> Source.java: Do we really want to support 1.x aliases for 9, 10?
Not fond of the aliases - but though they were needed (at least in the 
short term) as a compatibility step.
>
> source.allowed(feature)  feels clunky; it would be better to name the 
> method "allows" or "isAllowed". By itself, "allowed" feels too much 
> like past tense, which may be OK for earlier releases but doesn't work 
> for the current release.
ok
>
> The overall code is inconsistent with respect to using 
> source.allowed(feature) and feature.allowedInSource(source). Of the 
> two, my personal preference is for the latter; while reading the 
> webrev, I was wondering whether the source.allowed(feature) calls 
> would be better reversed, and was then mildly surprised when I saw 
> later files in the webrev using that pattern.  But I think consistency 
> should win out, whichever form you prefer to use.
Ok - I will make another pass on the code to iron out these consistency 
issues - I agree we should use same convention across the codebase. I 
believe source.allowXYZ has been translated to source.allowed(FEATURE) 
but the new uses in Javac parser have been translated using the 
different idiom. I agree that Feature.allowedInSource is more direct and 
more readable than the counterpart.
>
> Diagnostics.  javac messages are not a shining example of eloquence, 
> but I do worry about "{0} not supported in -source {1}\n\", line 2608.
> I think it would be better to revert to the earlier phrasing of "... 
> are not supported..." and then ensure that all the feature names are 
> plural, which is already mostly the case. The ones that stand out as 
> needing to be fixed are: "diamond operator" and "multi-catch 
> statement".  That leaves "''<>'' with anonymous inner classes" . So 
> maybe we end up with just one clunky phrasing. My best suggestion 
> would be to allow selected specific messages to use a custom message 
> when the template form doesn't work well.
Ok, I'll think of something - perhaps an enum which has PLURAL and 
SINGULAR as values which point to different errors, and each feature has 
to decide which is which.

Maurizio
>
> -- Jon
>
>
> On 10/20/2017 11:07 AM, 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
>>
>>
>>
>