RFR: JDK-8211148: javac -source 10 in JDK11 is inconsistent with javac in JDK10

[Maurizio Cimadamore]

The changes look generally good, but I have some questions/comments.

* (cosmetic, optional) This change:

if (Feature.VAR_SYNTAX_IMPLICIT_LAMBDAS.allowedInSource(source)) {
+                        log.error(DiagnosticFlag.SYNTAX, param.pos, Errors.VarNotAllowedArray);
+                    } else {
+                        log.error(DiagnosticFlag.SYNTAX, param.pos, Errors.VarNotAllowedHere);
+                    }

Could be slightly simplified as:

log.error(DiagnosticFlag.SYNTAX, param.pos, Feature.VAR_SYNTAX_IMPLICIT_LAMBDAS.allowedInSource(source)
  ? Errors.VarNotAllowedArray : Errors.VarNotAllowedHere);


* A more serious issue is, I think that the code here:

if (Feature.VAR_SYNTAX_IMPLICIT_LAMBDAS.allowedInSource(source)) {
+                        param.startPos = TreeInfo.getStartPos(param.vartype);
+                        param.vartype = null;
+                    } else {
+                        log.error(DiagnosticFlag.SYNTAX, param.pos, Errors.VarNotAllowedHere);
+                    }

Should really call the parser method 'checkSourceLevel' which will issue 
the correct error if the feature is not allowed (e.g. 'var in lambda not 
allowed, use source 11 to enable'). Which brings to:

* You should define a compiler key for the new feature (e.g. 'var in 
lambda') so that the framework will pick that up when generating a 
diagnostic using the above method

* In the classification logic, I believe the goal of the changes is to 
disable the diagnostic generation for illegal combinations of lambda 
parameter declarations when one or more of these involve a 'var' which 
would have already been flagged in a separate diagnostic. That is, these 
changes are to avoid to report duplicate and spurious diagnostics, right?

* In the LambdaParserTest I see no relevant changes, but still the patch 
shows something - is there some trailing space or indent different at play?

Maurizio

On 03/10/18 01:40, Vicente Romero wrote:
> Hi,
>
> Please review the fix for [1] at [2]. There were a number of places 
> where the compiler was not checking if var syntax for implicit lambdas 
> was allowed or not. This implied that the JDK11 compiler was accepting 
> code with this feature even if the -source 10 option was passed, this 
> shouldn't happen. This patch fixes this and issues the: "var not 
> allowed here" error message as expected.
>
> Thanks,
> Vicente
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8211148
> [2] http://cr.openjdk.java.net/~vromero/8211148/webrev.00/jdk.dev.patch