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

[Maurizio Cimadamore]

On 04/10/18 13:40, Vicente Romero wrote:
>
>
> On 10/04/2018 06:53 AM, Maurizio Cimadamore wrote:
>> Thanks, looks good. As for the test comment, I was referring to this 
>> hunk of the patch:
>>
>> -        EXPLIICT_SIMPLE("A", ExplicitKind.EXPLICIT),
>> -        EXPLIICT_SIMPLE_ARR1("A[]", ExplicitKind.EXPLICIT),
>> -        EXPLIICT_SIMPLE_ARR2("A[][]", ExplicitKind.EXPLICIT),
>> +        EXPLICIT_SIMPLE("A", ExplicitKind.EXPLICIT),
>> +        EXPLICIT_SIMPLE_ARR1("A[]", ExplicitKind.EXPLICIT),
>> +        EXPLICIT_SIMPLE_ARR2("A[][]", ExplicitKind.EXPLICIT),
>>
>>
>> Looks like the contents are the same, what's up?
>
> I see, I was just fixing a typo: EXPLIICT for EXPLICIT
Ah! thanks :-)

Maurizio
>>
>> Maurizio
>
> Vicente
>>
>> On 03/10/18 17:39, Vicente Romero wrote:
>>> 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
>>>>
>>>
>>
>