Re: RFR 8134941: Implement ES6 template literal support

[Andreas Woess]

Hannes, Attila,

thanks for your reviews and sorry for the late reply.

Template literals are always scanned as a whole, quote-to-quote (as 
with EditStringLexer). This turned out to be a problem with embedded 
functions in expressions and lazy/optimistic compilation on: 
Parser#skipFunctionBody would skip over the body and restart lexing 
at the RBRACE, forgetting about the embedding string context. I've 
worked around this in skipFunctionBody by skipping over to the RBRACE 
directly if it is already in the token stream (which it is because 
we've already scanned to the end quote).


It took me some time to figure this out. Maybe some more explanatory 
comments would be good. Does this also apply in other cases?
From the top of my head I'd say no, because we don't start the lexer at 
arbitrary positions, but only at a brace, and no other case comes to 
mind where we could end up with different tokens here. Yes, some more 
comments wouldn't have hurt. :)
It would probably be good to use the same code for scripting-mode `exec 
strings` which still have this and the quoting issue I mentioned.


We can probably file the issues as separate bugs and fix them later. 
One thing I want to do is add some more tests. Although your test 
script covers a lot of stuff (for its size) I would like to add a bit 
to it.

Please do!
Regarding caching of template objects: yes, we should follow the spec 
here, I deliberately left that out because I wanted to focus on the 
parser changes.



I’m sometimes in two minds about RuntimeNode (especially the fact that it 
currently can’t receive primitive parameters), but I have to admit that in this 
particular case it made the integration of the feature into the runtime fairly 
easy; it was really the parser parts that were tricky.
Yes, I like how simple it was to add runtime support with this approach, 
but you can certainly do better. Originally, I thought about adding 
another LiteralNode, but it would've been unnecessary complexity.


- Andreas

Re: RFR 8134941: Implement ES6 template literal support

[Michael Haupt]

Hi all,

excellent - I'll sponsor the change.

Best,

Michael

> Am 23.10.2015 um 11:28 schrieb Attila Szegedi :
> 
> +1 from me as well. Another apology from me for having to wait even more for 
> the second review.
> 
> Very nice work. I’m sometimes in two minds about RuntimeNode (especially the 
> fact that it currently can’t receive primitive parameters), but I have to 
> admit that in this particular case it made the integration of the feature 
> into the runtime fairly easy; it was really the parser parts that were tricky.
> 
> Attila.
> 
>> On Sep 24, 2015, at 4:44 PM, Hannes Wallnoefer 
>>  wrote:
>> 
>> Hi Andreas,
>> 
>> Thanks for the contribution, and sorry for the long time it took to get back 
>> to you.
>> 
>> I like the way you implemented this feature, the code looks very good.  
>> Comments inlined below.
>> 
>> Am 2015-09-03 um 18:49 schrieb Andreas Woess:
>>> Template literals are always scanned as a whole, quote-to-quote (as with 
>>> EditStringLexer). This turned out to be a problem with embedded functions 
>>> in expressions and lazy/optimistic compilation on: Parser#skipFunctionBody 
>>> would skip over the body and restart lexing at the RBRACE, forgetting about 
>>> the embedding string context. I've worked around this in skipFunctionBody 
>>> by skipping over to the RBRACE directly if it is already in the token 
>>> stream (which it is because we've already scanned to the end quote).
>> 
>> It took me some time to figure this out. Maybe some more explanatory 
>> comments would be good. Does this also apply in other cases?
>> 
>>> 
>>> Outstanding correctness issues not dealt with:
>>> * 12.2.9.3 GetTemplateObject stipulates that the returned template object 
>>> be cached and unique. I don't know why you'd want the spec to demand 
>>> caching rather than allow it (functionally it does not make a difference, 
>>> but you could observe it not being cached in a test).
>> 
>> Actually object identity can be observed by the tag function, and that was 
>> the reason for the committee to specify this. They chose caching of objects 
>> for performance reasons. The discussion can be found here:
>> 
>> https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-18.md#48-template-literal-call-site-object-caching
>> 
>> Even though it's a minor issue we should follow the spec here.
>> 
>>> * 12.2.9.5 Evaluation: string concatenation using + is slightly off-spec. 
>>> There are two way to solve this: (a) wrap the expressions in a ToString 
>>> UnaryExpression (or RuntimeCall) or (b) generate a call to concat with 
>>> interleaved string and expression arguments.
>>> 
>> 
>> The difference is between ToPrimitive being called with Number hint vs. 
>> String hint. This should not make a difference for the vast share of objects 
>> (all except those having a custom valueOf function I think), but it's 
>> something we should also get right. I've started experimenting with the 
>> solutions you suggested, I think adding a ToString conversion wrapper of 
>> some kind would be best.
>> 
>> All in all I think your patch looks good. We can probably file the issues as 
>> separate bugs and fix them later. One thing I want to do is add some more 
>> tests. Although your test script covers a lot of stuff (for its size) I 
>> would like to add a bit to it.
>> 
>> Thanks,
>> Hannes


-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment

Re: RFR 8134941: Implement ES6 template literal support

[Attila Szegedi]

+1 from me as well. Another apology from me for having to wait even more for 
the second review.

Very nice work. I’m sometimes in two minds about RuntimeNode (especially the 
fact that it currently can’t receive primitive parameters), but I have to admit 
that in this particular case it made the integration of the feature into the 
runtime fairly easy; it was really the parser parts that were tricky.

Attila.

> On Sep 24, 2015, at 4:44 PM, Hannes Wallnoefer  
> wrote:
> 
> Hi Andreas,
> 
> Thanks for the contribution, and sorry for the long time it took to get back 
> to you.
> 
> I like the way you implemented this feature, the code looks very good.  
> Comments inlined below.
> 
> Am 2015-09-03 um 18:49 schrieb Andreas Woess:
>> Template literals are always scanned as a whole, quote-to-quote (as with 
>> EditStringLexer). This turned out to be a problem with embedded functions in 
>> expressions and lazy/optimistic compilation on: Parser#skipFunctionBody 
>> would skip over the body and restart lexing at the RBRACE, forgetting about 
>> the embedding string context. I've worked around this in skipFunctionBody by 
>> skipping over to the RBRACE directly if it is already in the token stream 
>> (which it is because we've already scanned to the end quote).
> 
> It took me some time to figure this out. Maybe some more explanatory comments 
> would be good. Does this also apply in other cases?
> 
>> 
>> Outstanding correctness issues not dealt with:
>> * 12.2.9.3 GetTemplateObject stipulates that the returned template object be 
>> cached and unique. I don't know why you'd want the spec to demand caching 
>> rather than allow it (functionally it does not make a difference, but you 
>> could observe it not being cached in a test).
> 
> Actually object identity can be observed by the tag function, and that was 
> the reason for the committee to specify this. They chose caching of objects 
> for performance reasons. The discussion can be found here:
> 
> https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-18.md#48-template-literal-call-site-object-caching
> 
> Even though it's a minor issue we should follow the spec here.
> 
>> * 12.2.9.5 Evaluation: string concatenation using + is slightly off-spec. 
>> There are two way to solve this: (a) wrap the expressions in a ToString 
>> UnaryExpression (or RuntimeCall) or (b) generate a call to concat with 
>> interleaved string and expression arguments.
>> 
> 
> The difference is between ToPrimitive being called with Number hint vs. 
> String hint. This should not make a difference for the vast share of objects 
> (all except those having a custom valueOf function I think), but it's 
> something we should also get right. I've started experimenting with the 
> solutions you suggested, I think adding a ToString conversion wrapper of some 
> kind would be best.
> 
> All in all I think your patch looks good. We can probably file the issues as 
> separate bugs and fix them later. One thing I want to do is add some more 
> tests. Although your test script covers a lot of stuff (for its size) I would 
> like to add a bit to it.
> 
> Thanks,
> Hannes
> 
> 
> 
> 
> 

Re: RFR 8134941: Implement ES6 template literal support

[Hannes Wallnoefer]

Hi Andreas,

Thanks for the contribution, and sorry for the long time it took to get 
back to you.


I like the way you implemented this feature, the code looks very good.  
Comments inlined below.


Am 2015-09-03 um 18:49 schrieb Andreas Woess:
Template literals are always scanned as a whole, quote-to-quote (as 
with EditStringLexer). This turned out to be a problem with embedded 
functions in expressions and lazy/optimistic compilation on: 
Parser#skipFunctionBody would skip over the body and restart lexing at 
the RBRACE, forgetting about the embedding string context. I've worked 
around this in skipFunctionBody by skipping over to the RBRACE 
directly if it is already in the token stream (which it is because 
we've already scanned to the end quote).


It took me some time to figure this out. Maybe some more explanatory 
comments would be good. Does this also apply in other cases?




Outstanding correctness issues not dealt with:
* 12.2.9.3 GetTemplateObject stipulates that the returned template 
object be cached and unique. I don't know why you'd want the spec to 
demand caching rather than allow it (functionally it does not make a 
difference, but you could observe it not being cached in a test).


Actually object identity can be observed by the tag function, and that 
was the reason for the committee to specify this. They chose caching of 
objects for performance reasons. The discussion can be found here:


https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-18.md#48-template-literal-call-site-object-caching

Even though it's a minor issue we should follow the spec here.

* 12.2.9.5 Evaluation: string concatenation using + is slightly 
off-spec. There are two way to solve this: (a) wrap the expressions in 
a ToString UnaryExpression (or RuntimeCall) or (b) generate a call to 
concat with interleaved string and expression arguments.




The difference is between ToPrimitive being called with Number hint vs. 
String hint. This should not make a difference for the vast share of 
objects (all except those having a custom valueOf function I think), but 
it's something we should also get right. I've started experimenting with 
the solutions you suggested, I think adding a ToString conversion 
wrapper of some kind would be best.


All in all I think your patch looks good. We can probably file the 
issues as separate bugs and fix them later. One thing I want to do is 
add some more tests. Although your test script covers a lot of stuff 
(for its size) I would like to add a bit to it.


Thanks,
Hannes

RFR 8134941: Implement ES6 template literal support

[Andreas Woess]

Please review http://cr.openjdk.java.net/~aw/8134941/ for 
https://bugs.openjdk.java.net/browse/JDK-8134941 .


This change adds support for ES6 template literals (both untagged and 
tagged). This is rather heavy patch, so let me walk you through it.


Instead of using the existing EditStringLexer, I took a somewhat 
different approach. I've added 4 lexer tokens for the different template 
string portions cf. 11.8.6 and let the parser decide what to do with it. 
These are then treated different depending on whether the literal is tagged:

* untagged: desugared to a string concatenation using +
* tagged: desugared to function call (cf. 12.2.9.2) tag( 
RuntimeCall(GET_TEMPLATE_OBJECT, [...rawStrings], [...cookedStrings]), 
...evaluatedExpressions ), where rawStrings and cookedStrings are the 
unescaped and escaped string portions, respectively. CR and CRLF in the 
strings are always converted to LF.


The EditStringLexer did not correctly count quoted braces in embedded 
expressions (e.g., `${ "}" }` would error), so I've hooked an open brace 
counter into Lexer#lexify() directly that is context-aware. It's purpose 
is to break out of the nested Lexer at the closing RBRACE, and then 
continue with scanning the rest of the literal.
Template literals are always scanned as a whole, quote-to-quote (as with 
EditStringLexer). This turned out to be a problem with embedded 
functions in expressions and lazy/optimistic compilation on: 
Parser#skipFunctionBody would skip over the body and restart lexing at 
the RBRACE, forgetting about the embedding string context. I've worked 
around this in skipFunctionBody by skipping over to the RBRACE directly 
if it is already in the token stream (which it is because we've already 
scanned to the end quote).


Template strings use the same quotes (`) as exec strings in -scripting 
mode; if mixed, --language=es6 takes precedence over -scripting, 
ES5+scripting is unaffected.


Outstanding correctness issues not dealt with:
* 12.2.9.3 GetTemplateObject stipulates that the returned template 
object be cached and unique. I don't know why you'd want the spec to 
demand caching rather than allow it (functionally it does not make a 
difference, but you could observe it not being cached in a test).
* 12.2.9.5 Evaluation: string concatenation using + is slightly 
off-spec. There are two way to solve this: (a) wrap the expressions in a 
ToString UnaryExpression (or RuntimeCall) or (b) generate a call to 
concat with interleaved string and expression arguments.


Testing: ant -f make/build.xml test-pessimistic test-optimistic

Thanks,
Andreas