1024748 - Implement Template Literals as described in ES6 draft section 12.2.9

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[guptha]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Comment 1

[guptha]

Attachment: Original patch v1

Comment 2

[André Bargull [:anba]]

The JSOP_ADD instruction is not quite correct, because it does not apply the correct string conversion. Is there already a matching opcode for string conversion you can use here?

Test case: `${{valueOf: () => "<valueOf>", toString: () => "<toString>"}}`
Expected: Evaluates to "<toString>"

Comment 3

[guptha]

Attachment: The one where toString behavior is incorrect version 1

I'm adding this part again because I left some tests commented out last time

Comment 4

[guptha]

Attachment: Added JSOP_TOSTRING v1

Added the JSOP_TOSTRING bytecode

Comment 5

[guptha]

Attachment: Added JSOP_TOSTRING v2

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8440002 [details] [diff] [review]
The one where toString behavior is incorrect version 1

Good work. Lots of comments. Ping me when you get a revised patch posted.

Please generate patches for review using diff -U8. The extra context
really helps reviewers.

Clearing r? for now.

In js/src/frontend/BytecodeEmitter.cpp:
> /* A function, so that we avoid macro-bloating all the other callsites. */
>+/* This function updates the line number and column number information in the source notes */
> static bool
> UpdateSourceCoordNotes(ExclusiveContext *cx, BytecodeEmitter *bce, uint32_t offset)

Please don't write two adjacent comments like that. In this case, the
pre-existing comment can just be deleted. Your comment is a sentence, so
it should end with a period, but "This function" can be removed:

    /* Update the line number and column number information in the source notes. */

In EmitTemplateString:
>+            // There are two items on the stack. We should push in add now

This comment isn't quite grammatical. How about:

    // We've pushed two items onto the stack. Add them together, leaving just one.

In js/src/frontend/FullParseHandler.h:
>+    ParseNode *newTemplateStringLiteral(JSAtom *atom, const TokenPos &pos) {
>+        return new_<NullaryNode>(PNK_TEMPLATE_STRING, JSOP_STRING, pos, atom);
>+    }

Please use JSOP_NOP instead of JSOP_STRING.

The reason for this is a little embarrassing. The pn_op field of
ParseNodes is *mostly* unused and we'd like to remove it. So please
don't store any useful information there if you can help it. Thanks. :)

In js/src/frontend/Parser.cpp:
>+typename ParseHandler::Node
>+Parser<ParseHandler>::templateStringExpr()

Please rename this method to templateLiteral (the name ES6 uses for this).

This code is pretty nice! It can be simpler, though. Ideally the loop
should look something like this (pseudocode):

    do {
        parse an expression;
        parse a TemplateTail;
    } while (tt == TOK_TEMPLATE_HEAD);

That is: you should not need to call expr() both outside the loop and
inside the loop.

>+Parser<ParseHandler>::templateStringLiteral()

If you keep this method, please rename it to noSubstitutionTemplate (the
name the ES6 specification uses for this).

>+    // Large strings are fast to parse but slow to compress. Stop compression on
>+    // them, so we don't wait for a long time for compression to finish at the
>+    // end of compilation.
>+    const size_t HUGE_STRING = 50000;
>+    if (sct && sct->active() && atom->length() >= HUGE_STRING)
>+        sct->abort();

Please never copy and paste code if there is a reasonable alternative!
Common this up with stringLiteral(), either by factoring out a method
containing the shared code, or by merging them completely and adding an
extra argument to tell if it's a template literal or not.

In frontend/SyntaxParseHandler.h:
>+#ifdef JS_HAS_TEMPLATE_STRINGS
>+    Node newTemplateStringLiteral(JSAtom *atom, const TokenPos &pos) {
>+        lastAtom = atom;
>+        lastStringPos = pos;
>+        return NodeString;
>+    }
>+#endif

This will cause isStringExprStatement to return true for statements like

    `use strict`;

which we don't want, I think. See where this is called and what it is
used for, and find out if it's a bug. If so, figure out how to write a
test.

In frontend/TokenStream.h:
>+    TOK_TEMPLATE_STRING_SUBS,      // template string with substitutions
>+    TOK_TEMPLATE_STRING_NO_SUBS,   // template string without substitutions

Please rename these to
    TOK_TEMPLATE_HEAD,             // start of template literal with substitutions
    TOK_NO_SUBS_TEMPLATE,          // template literal without substitutions

In js/src/frontend/TokenStream.cpp:
>+    // Check if in the middle of a template string. Have to get this out of
>+    // the way first.
>+#ifdef JS_HAS_TEMPLATE_STRINGS
>+    if (MOZ_UNLIKELY(modifier == TemplateTail)) {
>+        c = '`';
>+        c1kind = TemplateString;
>+        goto template_string_cont;
>+    }
>+#endif

Instead of a goto, is it possible to factor that code out into a
separate function that we can call from here and from the string-parsing
code?

I'm not allergic to goto, so if it's even uglier to implement without
goto, just say so. :)

>+            if (c == '$' && nc == '{') {
>+                tp->type = TOK_TEMPLATE_STRING_SUBS;
>+            } else {
>+                tp->type = TOK_TEMPLATE_STRING_NO_SUBS;
>+            }

Style nit: Please remove the curly braces here, since the `if`,
then-substatement, and else-substatement all fit on single lines.

In js/src/jsreflect.cpp:
>+#ifdef JS_HAS_TEMPLATE_STRINGS
>+      case PNK_TEMPLATE_STRING:
>+#endif

OK, but what about substitutions? We need a test for Reflect.parse that
uses substitutions, and that means some code here to implement.

Let me know if I should explain this a little more.

Esprima's "harmony" branch is presumably ahead of us on this. Take a
look at what they've done, maybe?

In js/src/tests/ecma_6/TemplateStrings/templLitTests.js:
>+syntaxError("${}");

Please add these tests. (I haven't tested these tests, so there may be
bugs. Sorry!)

syntaxError("${");
syntaxError("${\\n}");
syntaxError("${yield 0}");

// Extra whitespace inside a template substitution is ignored.
assertEq(`${
0
}`, "0");

assertEq(`${ // Comments work in template substitutions.
// Even comments that look like code:
// 0}`, "FAIL");   /* NOTE: This whole line is a comment.
0}`, "0");

// Template substitutions are expressions, not statements.
syntaxError("${0;}");
assertEq(`${{}}`, "[object Object]");
assertEq(`${
    function f() {
        return "ok";
    }()
}`, "ok");

// Template substitutions can have side effects.
var x = 0;
assertEq(`= ${x += 1}`, "= 1");
assertEq(x, 1);

// The production for a template substitution is Expression, not
// AssignmentExpression.
x = 0;
assertEq(`${++x, "o"}k`, "ok");
assertEq(x, 1);

// --> is not a comment inside a template.
assertEq(`
--> this is text
`, "\n--> this is text\n");

// reentrancy
function f(n) {
    if (n === 0)
        return "";
    return `${n}${f(n - 1)}`;
}
assertEq(f(9), "987654321");

// Template string substitutions in generator functions can yield.
function* g() {
    return `${yield 1} ${yield 2}`;
}
var it = g();
var next = it.next();
assertEq(next.done, false);
assertEq(next.value, 1);
next = it.next("hello");
assertEq(next.done, false);
assertEq(next.value, 2);
next = it.next("world");
assertEq(next.done, true);
assertEq(next.value, "hello world");

// undefined
assertEq(`${void 0}`, "undefined");
assertEq(`${Object.doesNotHaveThisProperty}`, "undefined");

// Template string substitution values are converted to strings using ToString.
assertEq(`${{toString: () => "PASS"}}`, "PASS");
assertEq(`${{toString: () => "PASS", valueOf: () => "FAIL"}}`, "PASS");

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8440012 [details] [diff] [review]
Added JSOP_TOSTRING v2

Review of attachment 8440012 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice. r=me with the one comment addressed.

Until you have level 3 access, this means you need to post an updated patch. But you can just set 'review+' on the new patch. There's no need for another full review.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3452,2 @@
>          if (pn2 != pn->pn_head) {
>              // There are two items on the stack. We should push in add now

Now this comment can be changed to say "two strings" rather than "two items".

::: js/src/tests/ecma_6/TemplateStrings/templLitTests.js
@@ +45,5 @@
>  // inception
>  assertEq("abcdef", `a${`b${"cd"}e${"f"}`}`);
>  
> +// proper toString behavior
> +assertEq("<toString>", `${{valueOf: () => "<valueOf>", toString: () => "<toString>"}}`);

Ah! Never mind the very similar test I wrote for this, then (in the other review).

::: js/src/vm/Interpreter.cpp
@@ +2735,5 @@
> +        operString = ToString<CanGC>(cx, oper);
> +        if (!operString)
> +            return false;
> +    }
> +    oper.setString(operString);

Shorter:

    if (!oper.isString()) {
        JSString *operString = ToString<CanGC>(cx, oper);
        if (!operString)
            goto error;
        oper.setString(operString);
    }

Note that `return false` instead of `goto error` would bypass the code that looks to see if we're in a `try` block. Add a test that would catch this bug.

Comment 8

[guptha]

Attachment: The one where toString behavior is incorrect version 2 (addressed review comments)

Comment 9

[guptha]

Attachment: Added JSOP_TOSTRING v3 (addressed review comments)

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8442258 [details] [diff] [review]
The one where toString behavior is incorrect version 2 (addressed review comments)

Review of attachment 8442258 [details] [diff] [review]:
-----------------------------------------------------------------

OK, r=me with these comments addressed. Please update your patch, post it here, and push it to the Try Server.

When the Try Server likes it, you can post the link here and add checkin-needed to the Keywords field.

::: js/src/frontend/SyntaxParseHandler.h
@@ +78,5 @@
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +    Node newTemplateStringLiteral(JSAtom *atom, const TokenPos &pos) {
> +        lastAtom = atom;
> +        lastStringPos = pos;
> +        return NodeGeneric;

Don't set lastAtom or lastStringPos, since you're not telling the caller that this is a string.

::: js/src/frontend/TokenStream.cpp
@@ +1056,5 @@
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +    if (MOZ_UNLIKELY(modifier == TemplateTail)) {
> +        if (getStringOrTemplateToken('`', TemplateString, &tp) == true)
> +            goto out;
> +        goto error;

Style nit: Instead, write:

    if (!getStringOrTemplateToken('`', TemplateString, &tp))
        goto error;
    goto out;

This is more like how every other error path in the codebase is written, and if we ever need to add more code between the if statement and 'goto out', it can be done without rearranging anything.

@@ +1250,5 @@
>  #endif
>           ) {
> +        if (getStringOrTemplateToken(c, c1kind, &tp) == true)
> +            goto out;
> +        goto error;

Same here.

@@ +1583,5 @@
>      onError();
>      return TOK_ERROR;
>  }
>  
> +bool TokenStream::getStringOrTemplateToken(int c, FirstCharKind c1kind, Token **tp)

Try replacing
    c1kind == TemplateString        with     qc == '`'
    c1kind == String                with     qc != '`'

That way the c1kind argument can be deleted, and FirstCharKind does not need to be moved to the header. Also I think you can get rid of TemplateString and just use String for '`'.

@@ +1586,5 @@
>  
> +bool TokenStream::getStringOrTemplateToken(int c, FirstCharKind c1kind, Token **tp)
> +{
> +    *tp = newToken(-1);
> +    int qc = c;

The argument should be named qc, and just declare `int c;` here.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +404,5 @@
>  var hasTemplateStrings = false;  try { eval("``"); hasTemplateStrings = true; } catch (exc) { }
>  if (hasTemplateStrings == true) {
>      assertStringExpr("`hey there`", literal("hey there"));
>      assertStringExpr("`hey\nthere`", literal("hey\nthere"));
> +    assertExpr("`hey${\"there\"}`", templateLit([lit("hey"), lit("there"), lit("")]));

OK! This will be fine for now. Before we ship it, we need to settle on a final form of Reflect.parse output that also supports tagged templates and that we can share with Esprima. Here's what Esprima does right now:

js> load("esprima.js");
js> esprima.parse("`hey${\"there\"}`").body[0].expression
({
    type: "TemplateLiteral",
    quasis: [
        {type: "TemplateElement", value: {raw: "hey", cooked: "hey"}, tail: false},
        {type: "TemplateElement", value: {raw: "", cooked: ""}, tail: true}
    ],
    expressions: [
        {type: "Literal", value: "there", raw: "\"there\""}
    ]
})

Somewhere between what you've got and what they've got is where we need to end up...

Comment 11

[guptha]

Attachment: The one where toString behavior is incorrect version 3 (addressed review comments)

Comment 12

[guptha]

https://tbpl.mozilla.org/?tree=Try&rev=84c89a820a72

Comment 13

[guptha]

https://tbpl.mozilla.org/?tree=Try&rev=164e11289372

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Can you please suggest some commit messages for these patches per the link below? I'm not sure I understand them well enough to do so myself. Also note that your patches should contain the other commit information as well. Thanks!

https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 16

[guptha]

Attachment: The one where toString behavior is incorrect version 4 (added commit message to patch)

Comment 17

[guptha]

Attachment: Added JSOP_TOSTRING v4 (added commit message to patch)

Comment 18

[guptha]

Ryan,

I've updated the patches with commit messages. Please let me know if anything else is needed, thanks!

Comment 19

[guptha]

Attachment: The one where toString behavior is incorrect version 5 (added user info to patch)

Comment 20

[guptha]

Attachment: Added JSOP_TOSTRING v4 (added user information to patch)

Comment 21

[guptha]

Attachment: The one where toString behavior is incorrect version 6 (updated commit message)

Comment 22

[guptha]

Attachment: Added JSOP_TOSTRING v6 (updated commit message)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fca18fabdbbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c487660d38f

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fca18fabdbbd
https://hg.mozilla.org/mozilla-central/rev/3c487660d38f

Comment 25

[Florian Bender]

(Set flag on meta bug instead, sorry for bugspam.)

Comment 26

[Florian Scholz (Open Web Docs)]

Docs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings