RFR:JDK-8198749 Translation of value constructors in classic constructor notation

[Srikanth]

Here is the push URL: 
http://hg.openjdk.java.net/valhalla/valhalla/rev/5d5382d7200c

See below for action taken report.

Thanks a lot for the review comments Maurizio. Appreciate it!
Srikanth

On Wednesday 11 July 2018 06:38 PM, Maurizio Cimadamore wrote:
> Hi Srikanth,
> very good piece of work. Some comments below:
>
> * JCTree: factory product symbol - I think what you did ( stash the 
> symbol in the tree) is an ok approach. If we were trying to reduce 
> changes to the AST (and we do not at this stage), we could probably 
> always assume that this symbol is the first local var slot of the 
> method (e.g. vload_0), so no symbol is strictly needed in Gen. And, as 
> for TransValues, I think we could have some visitor variable holding 
> the temporary symbol. Again, not needed - just giving some suggestion 
> in case you want to clean up the changes in the method AST.

As documented in previous mail, I undertook this noble pursuit in 
earnest and it took me to some interesting corners that negated the 
initial promising simplification that appeared possible. So
at least for now, I will stay with the present implementation. IMO it is 
not unreasonable that a
parse tree node would store a computed/derived attribute as opposed to 
what was primarily present
in source.

> * In LambdaToMethod, I believe your idea was that, for Foo::new, where 
> Foo is a value, is better to generate a lambda, given that the 'new 
> expression' will need to be altered by the TransValues rewriter. I 
> think this is a good, foward-looking call. +1

Thanks
>
> * TreeMaker not sure whether you need the new method to create a 
> method AST with a symbol in it - can't we just create the AST and then 
> set the symbol? Of course I don't object much to the new factory, 
> which is innocuous, it's just that it is only used in one place, and 
> it doesn't seem to improve the code all that much to justify the 
> addition.

I have withdrawn this change.

>
> * TransValues...
>
> - the name 'translatingValueConstructor' seems a bit off - I would 
> probably call it "insideValueConstructor". Also, this predicate kind 
> of relies on the 'currentMethod' field to be set - I wonder if having 
> a true predicate on the method symbol wouldn't be better and more 
> reusable.
>
Name simplified to constructingValue()

> - I don't fully get the dance around JCNewClass when you want to 
> create a default value. I think a JCNewClass tree is too heavy for a 
> default value creation. What you need is an AST node with argument 
> expression and a type (or a symbol, if you want). You don't need 
> constructor symbols, I think - I get that this code has probably 
> changed over time from a world with slightly different invariants, but 
> looking at it now feels overly complex.

Agreed. I have raised https://bugs.openjdk.java.net/browse/JDK-8207168.

> - on whether you need a return or not, is the problem that you don't 
> know whether the code is alive or not after the last statement of the 
> constructor has been evaluated? If that's the case, you *could* in 
> principle feed the new method to Flow.aliveAnalyzer, and see whether 
> it completes normally or not. That's the same logic we also use for 
> lambda checking. E.g. you could create a new AliveAnalyzer visitor, 
> run it on the body you have, and then check whether its 'alive' 
> parameter is true or not. That will allow you to generate the body in 
> full, w/o splitting logic between TransValues and Gen (which is of 
> course fine as an interim workaround)

This again I have deferred after taking a look.

> - shouldn't we add the new factory to the scope of the class? How is 
> even ClassWriter picking it up if it's not in the scope? Ah, I see, 
> you add it into the scope in getValueFactory, and you do it only once 
> because of the init2factory map. I suggest to move the scope entering 
> near to where the factory method is created so that, longer term, we 
> can be guarateed that we won't attempt to add it to the same scope twice.

Done.
> - replacing old constructor body with super call is odd. Maybe we 
> should throw, to make sure nobody is calling that?

As discussed elsewhere, a throw is not done because verifier anyways 
rejects any attempts to reach <init> view new. new and VT don't go 
together. So ATM constructors are stubbed out with just a supercall 
which is required by the verifier.
>
> - don't you need also to override visitApply in case the constructor 
> is calling  an instance method on the value being constructed?

This will be covered as part of JDK-8205910
>
> * tests - mayeb for later, and depending on whether we support inner 
> values (seems to, looking at InnerValueNew) - one kind of super 
> constructor call that might get you in trouble is the qualified super 
> constructor call - e.g., with regular ordinary classes you can have this:

I think this is a non-issue, anyway I have added the following as a test 
FWIW.

>
> class A {
>
>     class Inner { }
> }
>
>
> class B extends A.Inner {
>
>        B(A encl) {
>            enc.super();
>        }
> }
>
>
> It would be interesting to verify at some point that this kind of 
> idiom works with your value desugaring too.
>
>
> That's all I can think of.
>
> Cheers
> Maurizio
>
>
>
> On 11/07/18 11:56, Srikanth wrote:
>>
>> Hi Maurizio,
>>
>> Please review the following patch that implements the changes 
>> required for
>> https://bugs.openjdk.java.net/browse/JDK-8198749 ([lworld] 
>> Translation of value constructors in classic constructor notation)
>>
>> Webrev: http://cr.openjdk.java.net/~sadayapalam/JDK-8198749/webrev.00/
>>
>> Part of the work in supporting value construction in the classic 
>> constructor notation
>> will happen independently on behalf of 
>> https://bugs.openjdk.java.net/browse/JDK-8205910(diagnose use of 
>> 'this' with DU fields (for VTs and VBCs)).
>>
>> Normally I would have requested the review after a fair bit more of 
>> testing and self-review, but given your proposed long absence, I am 
>> requesting the review now. However, I am happy to note that the code 
>> is in pretty decent shape for another person to study (known issues 
>> at the bottom)
>>
>> I will continue with the testing and self review in parallel.
>>
>> If time is an issue, you can limit the review to the following source 
>> files:
>>
>>     Gen.java
>>     TransValues.java
>>     LambdaToMethod.java
>>
>> and these test files: (these give an idea of what works already and 
>> allows you to experiment by tweaking)
>>
>> test/langtools/tools/javac/valhalla/lworld-values/InnerValueNew.java
>> test/langtools/tools/javac/valhalla/lworld-values/LocalValueNew.java
>> test/langtools/tools/javac/valhalla/lworld-values/QualifiedThisTest.java
>> test/langtools/tools/javac/valhalla/lworld-values/ValueConstructorRef.java 
>>
>> test/langtools/tools/javac/valhalla/lworld-values/ValueNewReadWrite.java
>>
>> Known issues:
>>
>> (1) make.at() calls may not be updated consistently/uniformly.
>> (2) I need to double check that some subtree translations are not 
>> skipped inadvertently.
>> (3) Some of the other *older* modified tests need rewriting - they 
>> compare javap output which is flaky due to constant pool offsets 
>> changing anytime there is a change in code generation scheme.
>> (4) Some code duplication can be avoided by creating utility routines.
>>
>> langtools tests are green (except for one non-material failure).
>>
>> Thanks!
>> Srikanth
>
>