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

[Vicente Romero]

Hi Maurizio,

Thanks for the review. I have updated the webrev [1]

[1] http://cr.openjdk.java.net/~vromero/8211148/webrev.01/jdk.dev.patch

some comments below.

On 10/03/2018 06:18 AM, Maurizio Cimadamore wrote:
> 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);

done it
>
>
> * 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

done too,

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

yep, I reorganized the matrix putting the var rows and columns to the 
top and to the left, there wasn't a real reason to to this, but I 
thought that it could help reading the code in the future. Probably very 
subjective but still :) then yes if the source doesn't allow var in 
implicit lambdas a `mask` is applied to the value read from the matrix 
effectively changing it to a null to avoid duplicate reports.
>
> * In the LambdaParserTest I see no relevant changes, but still the 
> patch shows something - is there some trailing space or indent 
> different at play?

the change here was to add a case for which errors are expected when var 
is used and source is 10
>
> Maurizio

Vicente

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