RFR: 8210742: Compound var declaration splitting seems broken

[Maurizio Cimadamore]

Looks good. Seems like Netbeans needs to type check erroneous tree to 
later suggest possible refactoring that would be able to get rid of the 
error, which is why they are sensitive to what is the result of type 
analysis.

Cheers
Maurizio

On 19/11/2018 17:26, srinivas wrote:
>
> Hi Maurizio,
>
> After checking with NB team about the expected behaviour for array 
> types, below are the conclusions.
>
> 1. They provide some series hints for the error cases for the user.
>
> for ex: var v = {1,2}, w = 2;  will have hint as convert to var 
> v={1,2}; var w=2;
>
> For this they will reconstruct the tree structure with inferred types 
> from error messages .
>
> After having debugging session with them, I found the expected 
> behavior of the tree structure from javac is same for both jdk10 and 
> jdk11(for which
>
> they reported issue for var x; types earlier)
>
> So In summary: As discussed we don't need to alter array types 
> behavior from javac.
>
> Please review revised patch with code refactored with error message 
> order and array AST behaviour preserved.
>
> webrev: http://cr.openjdk.java.net/~sdama/8210742/webrev.01/
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210742
>
> Regards,
>
> Srinivas
>
> On 17/11/18 1:18 AM, Maurizio Cimadamore wrote:
>>
>> I understand.
>>
>> One thing that I don't 100% get is what is the set of requirements we 
>> need to preserve; in this case we have a parser error, meaning that 
>> the input source file is wrong according to the grammar; javac ends 
>> up creating a variable tree node anyway, and then this node gets 
>> eventually type-checked, and what we are talking about here is that, 
>> when this type-checking happens (on this tree that is the result of a 
>> javac error recovery step), we are observing a type checking 
>> difference from 10 to 11.
>>
>> Now, it seems like NB would like to have 'var' be applied in all 
>> branches of the compound assignment, and have all types be inferred.
>>
>> But, as you said, for a variable type such as 'var x[]' we can't do 
>> much, so we will report an error and we will, I think, create an AST 
>> of type 'var[]' which will be left untouched, I believe, by 
>> type-checking.
>>
>> So, I guess what I'm really asking for is some explanation of what 
>> the desired behavior should be, and why - so that we can make sure 
>> that, moving forward, we create the right kind of synthetic recovery 
>> trees in all cases.
>>
>> (of course none of this is an issue for javac which will shut down 
>> immediately after discovering a parser error w/o even attempting 
>> attribution).
>>
>> Maurizio
>>
>> On 15/11/2018 15:22, srinivas wrote:
>>>
>>> Hi Maurizio,
>>>
>>> Thanks for the review.
>>>
>>> Yes - Initially I thought of making this code common to all branches.
>>>
>>> Below are the reasons for not doing that.
>>>
>>> 1) the reported issue from NB team is for simple 'var x' types but 
>>> not for arrays.
>>>
>>> 2) I don't have enough data to preserve the type for arrays or not 
>>> and I want to avoid breaking any assumption related to arrays error 
>>> cases
>>>
>>> both from NB team and javac tests.
>>>
>>> Please let me know if I should include array case as well and 
>>> refactor the fix.
>>>
>>> Regards,
>>>
>>> Srinivas
>>>
>>>
>>>
>>> On 15/11/18 5:13 PM, Maurizio Cimadamore wrote:
>>>>
>>>> Hi,
>>>> I think what you are doing is ok, but I have some comments:
>>>>
>>>> 1) it seems like this code:
>>>>
>>>>
>>>> startPos = TreeInfo.getStartPos(mods);
>>>> if (startPos == Position.NOPOS)
>>>>    startPos = TreeInfo.getStartPos(type);
>>>>
>>>> should probably be lifted outside the 'if' - e.g. should apply to 
>>>> all cases after the check for "isRestrictedLocalVarTypeName"
>>>>
>>>> 2) A similar argument applies, kind of, to setting type to 'null'. 
>>>> With your patch we do that in 2/3 branches; the branch that is left 
>>>> out is when you have a var declaration like this:
>>>>
>>>> var foo[] = ...
>>>>
>>>> In this case the parser will still attempt to create a var tree 
>>>> with 'var' as its type. If we are worried about how the AST would 
>>>> look in erroneous cases, then I think this is a problem too.
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>> On 15/11/2018 10:31, srinivas wrote:
>>>>> Hi,
>>>>>
>>>>> Please review http://cr.openjdk.java.net/~sdama/8210742/webrev.00/
>>>>>
>>>>> for https://bugs.openjdk.java.net/browse/JDK-8210742.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Srinivas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20181120/c7a361a7/attachment.html>