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

[Jonathan Gibbons]

Mostly excellent.

Some nitpicks for your consideration:

Source.java: Do we really want to support 1.x aliases for 9, 10?

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.

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.

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.

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