1021368 - Implement NoSubstitutionTemplate as described in ES6 draft 11.8.6

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: 20140527133511

Comment 1

[guptha]

Attachment: Patch to implement NoSubstitutionTemplate

Comment 2

[guptha]

Attachment: Patch to implement NoSubstitutionTemplate

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8435401 [details] [diff] [review]
Patch to implement NoSubstitutionTemplate

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

Great work on the patch. I have a lot of *very picky* comments but they are all easily fixed. Whenever you have a chance to upload a revised patch, I will give it the r+ bit and we'll land it.

This is exciting stuff. I can't wait to see it in Nightly.

::: js/src/frontend/TokenStream.cpp
@@ +1300,5 @@
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +                                if (c1kind == TemplateString) {
> +                                    reportError(JSMSG_DEPRECATED_OCTAL);
> +                                    goto error;
> +                                } else 

We have this peculiar rule across the whole Mozilla codebase: No 'else' right after a return/break/continue/goto statement, or a block that ends with one of those.

So please remove this 'else' (and the trailing whitespace on this line), and re-indent the lines after it:

#ifdef FOO
            if (x == y) {
                reportError(PROBLEMS);
                goto fail;
            }
#endif
            if (!reportStrictModeError(PROBLEMS))
                goto fail;

(I actually agree with the rule in this case, as this will make the code a little nicer and the #ifdef a little less disruptive.)

@@ +1357,5 @@
> +                } else if ((
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +                c1kind == String && 
> +#endif
> +                TokenBuf::isRawEOLChar(c)) || c == EOF) {

The style rule in SpiderMonkey is to put the '{' on its own line, outdented, whenever the condition is multiple lines. Like this:

    } else if ((
#ifdef JS_HAS_TEMPLATE_STRINGS
        c1kind == String &&
#endif
        TokenBuf::isRawEOLChar(c)) || c == EOF)
    {
        ...
        ...
        ...
    }

Also, please remove the space at the end of the line that says 'c1kind == String &&'.

::: js/src/frontend/TokenStream.h
@@ +278,4 @@
>      }
>  
>      void setAtom(JSAtom *atom) {
> +        JS_ASSERT(type == TOK_STRING 

Please also remove the extra space at the end of this line.

You might want to configure your code editor to highlight trailing spaces.

::: js/src/tests/ecma_6/TemplateStrings/noSubstTests.js
@@ +1,1 @@
> +// |reftest| skip-if(!xulRuntime.shell)

Oh. Please remove this line. This test *should* run in the browser just fine... the only problem is that quit() I told you to put in. So please figure out how to take it out again! This test should run and pass in both shell and browser, whether template strings are enabled or not. I'm confident it can be done. Hint: since our whole test is now in a comment, there's no reason the try/catch/quit has to be at the top of the file anymore.

You'll want to test the test on your machine. Bug 889897 contains a patch that you can apply in your repository to make it easy to run js tests in the browser. With that patch, the command is like:

    ./mach jstestbrowser --filter=TemplateStrings

Of course you'll need to './mach build' first, if you haven't already built a browser.

@@ +4,5 @@
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +// This test case is weird in the sense the actual work happens at the eval 
> +// at the end. If template strings are not enabled, the test cases would throw 
> +// a syntax error and we'd have failure reported. To avoid that, the entire 

Please delete trailing spaces on these three lines.

@@ +7,5 @@
> +// at the end. If template strings are not enabled, the test cases would throw 
> +// a syntax error and we'd have failure reported. To avoid that, the entire 
> +// test case is commented out and is part of a function. We use toString to
> +// get the string version, obtain the actual lines to run, and then use eval to
> +// do the actual evaluation.

Very nice comment. Thank you!

@@ +9,5 @@
> +// test case is commented out and is part of a function. We use toString to
> +// get the string version, obtain the actual lines to run, and then use eval to
> +// do the actual evaluation.
> +
> +try { eval("``"); 

This line too.

@@ +14,5 @@
> +} catch (exc) { reportCompare(0, 0, "ok"); quit(); }
> +function testCaseFn() {
> +/*
> +function syntaxError(script) {
> +  try {

Please indent 4 spaces if you can bring yourself to do it. Two spaces is better than none, but only half as good as four. We don't enforce the rules as strictly in tests, but we do everywhere else, including js/src/builtin/*.js, so it's a good idea to get your editor configured for 4 spaces now...

Tabs are right out.

@@ +15,5 @@
> +function testCaseFn() {
> +/*
> +function syntaxError(script) {
> +  try {
> +    eval(script);

For the purpose of detecting a SyntaxError, Function(script); is even better, since it additionally checks that the error is thrown even if the code in question is never called! This specification requires this---might as well test it.

@@ +29,5 @@
> +
> +
> +// unterminated quasi literal
> +syntaxError("`");
> +syntaxError("`${");

These are awesome. Please also test "`$".

Which is excessive, right? But seriously this kind of thing is *such* a common source of bugs.

@@ +63,5 @@
> +syntaxError("`\\3`");
> +syntaxError("`\\4`");
> +syntaxError("`\\5`");
> +syntaxError("`\\6`");
> +syntaxError("`\\7`");

also:

syntaxError("`\\01`");
syntaxError("`\\001`");
syntaxError("`\\00`");

@@ +100,5 @@
> +assertEq("", eval("`\\\u2029`"))
> +
> +
> +// source character
> +for (var i = 0; i < 0xFFFF; ++i) {

This takes 5 seconds to run --- which is too much. These tests run very often.

So test 0-255 and whatever other particular points you're interested in. Definitely test \u2028 (LINE SEPARATOR) and \u2029 (PARAGRAPH SEPARATOR); those are special characters in regular strings (though of course not template strings).

@@ +116,5 @@
> +// multi-line
> +assertEq(`hey
> +there`, "hey\nthere");
> +
> +// differences between strings and template strings 

And here.

@@ +117,5 @@
> +assertEq(`hey
> +there`, "hey\nthere");
> +
> +// differences between strings and template strings 
> +syntaxError("Function(\"var obj = {`x`: 0};\")");

I would just delete this one. (It will no longer throw a syntax error after you switch syntaxError from using eval() to using Function().)

@@ +118,5 @@
> +there`, "hey\nthere");
> +
> +// differences between strings and template strings 
> +syntaxError("Function(\"var obj = {`x`: 0};\")");
> +syntaxError("var obj = { `illegal`: 1}");

Good; also

1.  Check that JSON.parse does not accept template strings anywhere.

2.  Check that ES5 getter syntax:
        {get "name"() { return 10; }}
    does not work with a template string.

3.  Check that in

        function f() {
            `ignored string`;
            "use strict";
            ...
        }

    the "use strict"; is *not* treated as a Use Strict Directive.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +53,5 @@
>  function breakStmt(lab) Pattern({ type: "BreakStatement", label: lab })
>  function continueStmt(lab) Pattern({ type: "ContinueStatement", label: lab })
>  function blockStmt(body) Pattern({ type: "BlockStatement", body: body })
> +function stringProgram(val) Pattern({type: "Program", body: val})
> +function stringExpr(val) Pattern({type: "ExpressionStatement", expression: val})

Delete these two lines...

@@ +54,5 @@
>  function continueStmt(lab) Pattern({ type: "ContinueStatement", label: lab })
>  function blockStmt(body) Pattern({ type: "BlockStatement", body: body })
> +function stringProgram(val) Pattern({type: "Program", body: val})
> +function stringExpr(val) Pattern({type: "ExpressionStatement", expression: val})
> +function stringLit(val) Pattern({value: val, type: "Literal" })

Rename to "literal" and rearrange the two fields so that "type" is first...

@@ +140,5 @@
>      program([patt]).assert(Reflect.parse(src, {builder: builder}));
>  }
>  
> +function assertStringExpr(src, patt) {
> +    stringProgram([stringExpr(patt)]).assert(Reflect.parse(src));

change this to

    program([exprStmt(patt)]).assert(Reflect.parse(src));

@@ +403,5 @@
>                    blockStmt([throwStmt(lit(1)), throwStmt(lit(2)), throwStmt(lit(3))]),
>                    exprStmt(lit(true))));
> +var hasTemplateStrings = false;  try { eval("``"); hasTemplateStrings = true; } catch (exc) { }
> +if (hasTemplateStrings == true) {
> +    assertStringExpr("`hey there`", stringLit("hey there"));

Also test one with a \n in it to make sure the result contains a string. Thanks.

Comment 4

[guptha]

Attachment: Patch to implement NoSubstitutionTemplate addressing review comments

Comment 5

[Jason Orendorff [:jorendorff]]

One more round.

The main thing missing is a test I requested in comment 3, that
    ({get `name`() { return 10; }});
is a SyntaxError.



In TokenStream.cpp
>+                } else if ((
>+#ifdef JS_HAS_TEMPLATE_STRINGS
>+                c1kind == String &&
>+#endif
>+                TokenBuf::isRawEOLChar(c)) || c == EOF)
>+                {

Please fix the indentation one more time. The lines starting with `c1kind` and
`TokenBuf` should be indented 4 more spaces.

In noSubstTests.js:
>+assertEq("", eval("`\\\u2028`"))
>+assertEq("", eval("`\\\u2029`"))

Also test these two without the backslash.

And as a matter of good style, please add a semicolon on these lines.

>+// test for JSON.parse
>+try {
>+    testVar = '[1, `false`]';
>+    JSON.parse(testVar);
>+    throw "Expected syntax error: JSON.parse(" + testVar + ")";
>+} catch (exc) {
>+    if (exc.name !== "SyntaxError") {
>+        throw "Expected syntax error: JSON.parse(" + testVar + ")";
>+    }
>+}

Please replace all this with:

    assertThrowsInstanceOf(() => JSON.parse('[1, `false`]'), SyntaxError);

>+try {
>+    eval("``");
>+    try {
>+        eval(str);
>+    } catch (exc) {
>+        throw "Test case failed" + exc;
>+    }
>+} catch (exc) {
>+    if (exc.toString().indexOf("Test case failed") > -1) {
>+        throw exc.replace(/^Test case failed/, '');
>+    }
>+}

Ehhh, all this string manipulation is kind of icky. Let's do this the way you did it in reflect-parse.js:

var hasTemplateStrings = false;  try { eval("``"); hasTemplateStrings = true; } catch (exc) { }
if (hasTemplateStrings)
    eval(str);

In reflect-parse.js:
>+function stringLiteral(val) Pattern({ type: "Literal",  value: val })

Please rename this to just `literal`.

Comment 6

[André Bargull [:anba]]

I haven't tested the actual patch, so maybe I'm wrong here, but line ending normalisation does not seem to be implemented. (Cf. TV of TemplateCharacter :: LineTerminatorSequence is its TRV and TRV of LineTerminatorSequence :: <LF> and LineTerminatorSequence :: <CR><LF> is \n.)
Two simple tests for this feature:
---
eval("`a\rb`") === "a\nb"
eval("`a\r\nb`") === "a\nb"
---

Comment 7

[guptha]

Attachment: Patch v3 to address comments

jorendorff:
I've addressed all your comments - sorry for missing comment 3 previously

André:
You are correct - modified the code and added the suggested test cases. Also, thank you for your exhaustive set of tests - have appropriated plenty of them.

Comment 8

[Jason Orendorff [:jorendorff]]

Mmh. This hasn't been run by the try server, removing checkin-needed.

Comment 9

[Jason Orendorff [:jorendorff]]

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

Comment 10

[guptha]

Attachment: Patch v4 to work around XP debug compiler bug

Comment 11

[Jason Orendorff [:jorendorff]]

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

Comment 12

[Jason Orendorff [:jorendorff]]

The Try Server thought that patch was just great. So I landed it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81893e0553da

This feature is Nightly-only for now. More to come.

Congratulations, Guptha!

Comment 13

[Chris Peterson [:cpeterson]]

Thanks, Guptha! :D

If you're looking for your next bug to fix, just drop by the #jsapi IRC channel on irc.mozilla.org or browse through mentor bugs on "Bugs Ahoy": http://www.joshmatthews.net/bugsahoy

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/81893e0553da

Comment 15

[guptha]

Chris: Thanks - I am already working with Jason on my next bug.

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> 3.  Check that in
> 
>         function f() {
>             `ignored string`;
>             "use strict";
>             ...
>         }
> 
>     the "use strict"; is *not* treated as a Use Strict Directive.

This seems not to have happened.  (Deferred for later, maybe?)  Also test

  Function("`use strict`; return 05;"); // should work

Comment 17

[guptha]

Yes Waldo - this will happen with my next patch (or the one after that) - right now, we pass a PNK_STRING to BytecodeEmitter. It will have to be a PNK_TEMPLATE_STRING to get that functionality.

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is there a need for manual testing on this feature? The set of tests seems rather exhaustive.

Comment 19

[Jason Orendorff [:jorendorff]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #18)
> Is there a need for manual testing on this feature? The set of tests seems
> rather exhaustive.

No manual testing is required.

Comment 20

[Adam Katz]

As a note for posterity, NoSubstitutionTemplate strings were written into the final ECMAScript 2015 spec as "template strings" and were renamed to "Template literals" in ECMAScript 2016.  They are documented on MDN here:

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

(This comment mostly serves to ensure that a web search for "template literal" will also show this bug.  I had written my documentation to use "NoSubstitutionTemplate" and then failed to find any resources on that when later reviewing my work.)