RFR: 8177466: Add compiler support for local variable type-inference

[Vicente Romero]

On 09/20/2017 05:14 PM, Maurizio Cimadamore wrote:
> Thanks for the comments - some responses below
>
>
> On 20/09/17 20:36, Vicente Romero wrote:
>> Hi,
>>
>> Another great piece of work! some high level comments:
>>
>> at Types:
>> * new visitors TypeProjection and CaptureScanner would benefit of 
>> having some documentation added, it should be made clear what 
>> (!upward) means as there is no explicit reference to the downward 
>> projection in the code. Instead of passing a boolean, using an enum 
>> could be beneficial for self documentation. Like enum Projection 
>> {UPWARD, DOWNWARD}, the enum could have a getInverseProjection() or 
>> getComplementProjection() method.
> Good idea, I'll play with it a bit
>>     - also probably examples of expected output for a given input 
>> could be added, but I'm not too strong about this though
>>     - also could it be beneficial to include some unit tests for 
>> them? I realize that they need to be public for a jtreg test to be 
>> able to access them but I think it will be important to check that 
>> the upward projection is yielding the
>>       expected result. I'm mostly interested in this code path: "Type 
>> lower = t.map(this, !upward);" at TypeProjection.mapTypeArgument()
> I think a TypeHarness test would be a natural way to unit test this. 
> Not sure though if this needs to be done now or as a follow up

right it doesn't have to be right now, it can wait.

> - after all, the test harness for LVTI is already providing some sort 
> of unit testing of this part, look at this:
>
> http://cr.openjdk.java.net/~mcimadamore/8177466_v2/test/langtools/tools/javac/lvti/harness/NonDenotableTest.java.html 
>
>>
>> Attr:
>> why at attribLazyConstantValue you do:
>>     - variable.sym.type = chk.checkLocalVarType(variable, itype, 
>> variable.name); and at visitVarDef:
>>    - v.type = chk.checkLocalVarType(tree, tree.init.type.baseType(), 
>> tree.name);  // baseType() is invoked in this case
>>
> As to why there's a baseType in visitVarDef, I believe it is because 
> we can get into that code path if a 'var' is not final but its 
> initializer is a constant - e.g. :
>
> var s = "Hello!"
>
> In this case we don't want the type of 's' to be constant.
>
> Regarding attribLazyConstantValue, I think it's probably better to use 
> baseType() there too. But I will have to double check if lack of 
> baseType is creating issues.
ok thanks for the clarification, yes I think that we need to investigate 
if you need to use baseType() for lazy evaluation too which should be 
triggered by:

final var s = "Hello!" right?
>>
>> JavacParser:
>> general question: can't the parser do an early detection of cases 
>> like "var s = () -> {};"?
> I think it's maybe doable, but it gets convoluted if you have casts 
> and other things before the lambda.

right but if there is a cast then there is no issue as there is a 
target, we need to discard only the cases when a "raw" lambda or method 
reference is used to initialize a variable with no explicit type. Again 
if you consider that code won't benefit from this then let's let it as 
it is. But it could be worthy trying to discard ill formed expressions 
as soon as possible

>>
>> Thanks,
>> Vicente

Vicente


>>
>>
>> On 09/19/2017 11:04 AM, Maurizio Cimadamore wrote:
>>> Hi,
>>> I have put together a slightly updated webrev:
>>>
>>> * as pointed out, one diagnostic used to say '9' instead of '10' 
>>> (this will likely need to change again because of new release 
>>> cadence, but good enough for now)
>>> * the previous webrev contained a spurious added file (TypeHarness) 
>>> which was caused by a glitch in the lvti branch in the amber repo.
>>>
>>> New webrev:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/8177466_v2/
>>>
>>> New diags:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/8177466_v2/diags.html
>>>
>>> Maurizio
>>>
>>>
>>> On 18/09/17 17:14, Maurizio Cimadamore wrote:
>>>> Hi,
>>>> this change adds support for local variable type inference (JEP 286 
>>>> [1]). A webrev of the change is available here:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/8177466
>>>>
>>>> The patch is relatively straightforward: implicitly typed locals 
>>>> are modeled in a similar fashion to implicit lambda parameters: 
>>>> their AST node is a JCVariableDecl whose 'vartype' field is not set 
>>>> (e.g. null).
>>>>
>>>> There are few tricky parts to this changeset:
>>>>
>>>> 1) tweak the parser to give 'var' special meaning depending on the 
>>>> version number and context
>>>>
>>>> 2) Add logic in name resolution to catch bad reference to types 
>>>> named 'var'
>>>>
>>>> 3) add logic to map initializer type back to a suitable variable 
>>>> declared type
>>>>
>>>> As for (1), the parser has been extended so as to special case 
>>>> local variables with special name 'var', so that the type will be 
>>>> left out of the corresponding AST representing the variable 
>>>> declaration. This behavior will only affect latest source version.
>>>>
>>>> The parser has a number of extra checks to prevent 'var to be used 
>>>> in places where it does not belong (according to the spec draft 
>>>> [2]); for instance, declaring a class whose name is 'var' is 
>>>> rejected in the parser. As a general rule, I tried to implement all 
>>>> such checks in the parser, as that gives very early and precise 
>>>> feedback about what's wrong with the code. The changes are 
>>>> implemented in Parser.java.
>>>>
>>>> There are however errors which cannot be caught in the parser, and 
>>>> that's why (2) is needed. Basically, whenever 'var' is used in a 
>>>> position where it could be either a type or a package name, the 
>>>> parser can't simply rule that out, so we have to accept the code, 
>>>> and give an error if, later on, we discover that 'var' was really 
>>>> used in a type position (see changes in Resolve.java).
>>>>
>>>> As far as (3) is concerned, we need to 'uncapture' captured types 
>>>> from initializers. That means that if we have a 'var' like this:
>>>>
>>>> class Foo {
>>>>     void test() {
>>>>         var x = getClass().getSuperClass();
>>>>     }
>>>> }
>>>>
>>>> The initializer type will be something like Class<? super #CAP>, 
>>>> where #CAP <: Foo
>>>>
>>>> In this case, the compiler will project this type back to the less 
>>>> specific type Class<?>, and use that as the declared type for 'x'. 
>>>> This logic is defined in Types.java. As this logic is the same 
>>>> logic needed by jshell to render type of standalone expressions, 
>>>> jshell class VarTypePrinter has been removed and jshell has been 
>>>> rewired to point at the (now) official routine in Types. Jshell 
>>>> also needed several other tweaks to (i) accept 'var' and (ii) to 
>>>> deal with non-denotable types (intersection types and anonymous 
>>>> class types) that can be produced by the LVTI machinery (many 
>>>> thanks to Jan for doing those changes!)
>>>>
>>>>
>>>> As far as testing is concerned, I wrote several tests to check that 
>>>> the parser was behaving as expected; to check the behavior of the 
>>>> LVTI inference machinery, I wrote a test harness which leverages 
>>>> annotation on 'var' so that we can write down assertions such as:
>>>>
>>>> @InferredType("java.util.List<? extends java.lang.String>")
>>>> var s = extString();
>>>>
>>>>
>>>> Regarding compiler diagnostics, for those interested, a 
>>>> comprehensive list of examples of new diagnostics triggered by the 
>>>> LVTI compiler can be found here:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/8177466/lvti-diags.html
>>>>
>>>> Finally, a finder has been added to detect local variable decls 
>>>> whose declared type can be replaced by 'var' - to enable it, the 
>>>> hidden option -XDfind=local should be used.
>>>>
>>>>
>>>> Thanks
>>>> Maurizio
>>>>
>>>> [1] - http://openjdk.java.net/jeps/286
>>>> [2] - http://cr.openjdk.java.net/~dlsmith/local-var-inference.html
>>>>
>>>>
>>>
>>
>