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

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

At first sight, this idea seems to come bundled with wonderful 
possibilities. Hmm. How about slot 0 reserved for factory product *AND* 
call the factory product `this' ;-) This would mean that *we could 
simply band-pass many of the trees without touching them at all* - i.e a 
field access such as this.x can be passed along to the code generator.

However, while prototyping it, I am hitting some interesting situations: 
SelfItem does not allow stores,
since `this' is usually not writeable. OK, we can subclass SelfItem for 
value factories. No problem.

But After doing that it turns out the class files produced would be bad 
from the verifier's standpoint because the initial frame shape is 
derived from the method descriptor and we have now stuck a VT ahead of 
the types in the descriptor :-(

Not a problem, I can force the generation of a full frame at bci 0 that 
properly describes what we have done, but I doubt if there isn't more to 
it ...

:)

Appending the product as the first local post argument vector does not 
confer any opportunities for elegance I fear.

Srikanth




> 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.
>
> * 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
>
> * 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.
>
> * 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.
>
> - 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.
>
> - 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)
>
> - 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.
>
> - replacing old constructor body with super call is odd. Maybe we 
> should throw, to make sure nobody is calling that?
>
> - don't you need also to override visitApply in case the constructor 
> is calling  an instance method on the value being constructed?
>
> * 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:
>
> 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
>
>