1031397 - Implement Tagged Templates as described in ES6 draft section 12.3.7

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[guptha]

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

Comment 1

[guptha]

Attachment: taggedtsv0.diff

function func(a,b){return a;}
c = func`a${4}b`

js> c
[["a", "b"], "a", "b"]
js> c.raw
["a", "b"]
js> c[0]
["a", "b"]

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8448338 [details] [diff] [review]
taggedtsv0.diff

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

We already discussed how we want to implement this on IRC, but I also want to show how to track down this kind of bug.

I applied the patch and ran the JS shell. Then I used the dis() function:

js> dis();   print`hello ${a} world`;
loc     op
-----   --
main:
00000:  getgname "dis"
00005:  undefined
00006:  call 0
00009:  setrval
00010:  getgname "print"
00015:  undefined
00016:  newarray 3
00020:  dup
00021:  object ["hello ", " world"]
00026:  setownprop "raw"
00031:  initelem_array 0
00035:  string "hello "
00040:  initelem_array 1
00044:  string " world"
00049:  initelem_array 2
00053:  endinit
00054:  getgname "a"
00059:  call 2
00062:  setrval
00063:  retrval
...

The code to create the call site object runs from offset 00016 to 00053 inclusive.

It looks like this bytecode will create exactly the kind of object you're observing:

newarray 3                            // arr
dup                                   // arr arr
object ["hello ", " world"]           // arr arr arr2
setownprop "raw"                      // arr arr2       (sets arr.raw = arr2)
initelem_array 0                      // arr            (defines arr[0] = arr2)

In other words, the .raw property isn't "indexable"; the bytecode is simply creating arr.raw and arr[0] with the same value.

Comment 3

[guptha]

Attachment: taggedtsv1.diff

I've changed the call site object as we wanted.

The raw strings are not what is needed. I'm at a loss about how to get at them from Parser.cpp without passing a modifier to getToken.

Comment 4

[guptha]

Attachment: Patch to implement tagged templates v2

I use JSOP_OBJECT and it works, but only because JS::CompartmentOptionsRef(cx).cloneSingletons(cx) is false. Should I use a different opcode for this. If not, how should I modify this?

Other than this, the feature is complete I think - will r?you once I update this.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8452394 [details] [diff] [review]
Patch to implement tagged templates v2

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

Make sure call site objects are handled correctly in js::XDRScript. XDR is a way of serializing scripts and functions. We use it for caching compiled code. Right now I think XDRScript would be able to serialize a call site object, but when it deserializes one, it won't create a frozen object. Write a test for this -- see js/src/jit-test/xdr/trivial.js for examples.

I guess it'll be best to use a new opcode for call site objects. The defined behavior of JSOP_OBJECT is "always clone". (As an optimization, we sometimes skip cloning, when it's impossible for JS code to detect that we did so.) But we want "never clone" behavior. Add the new opcode to vm/Opcodes.h; update XDR_BYTECODE_VERSION in vm/Xdr.h; and when it lands, don't forget to follow the instructions in Xdr.h to update the bytecode documentation.

Comment 6

[guptha]

Attachment: Patch to implement tagged templates v3

Comment 7

[André Bargull [:anba]]

Comment on attachment 8452819 [details] [diff] [review]
Patch to implement tagged templates v3

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

Some small comments from me:

::: js/src/frontend/Parser.cpp
@@ +7010,5 @@
>  
>              nextMember = handler.newPropertyByValue(lhs, propExpr, pos().end);
>              if (!nextMember)
>                  return null();
> +        } else if (allowCallSyntax &&

allowCallSyntax is only relevant for TOK_LP. That means this is valid syntax and is expected to return the array «[1, 2, 3]»:
---
new ((cs, sub) => function(){ return sub }) `${[1, 2, 3]}`
---

@@ +7031,5 @@
>              if (!nextMember)
>                  return null();
>  
>              if (JSAtom *atom = handler.isName(lhs)) {
>                  if (atom == context->names().eval) {

It is not necessary to special case "eval" in tagged template literals, because that form will never perform a proper direct eval call.

::: js/src/frontend/TokenStream.h
@@ +469,5 @@
>      // asm.js reporter
>      void reportAsmJSError(uint32_t offset, unsigned errorNumber, ...);
>  
> +    JSAtom *getRawAtom(uint32_t begin, uint32_t length) {
> +        return AtomizeChars(cx, userbuf.base() + begin, length);

Line endings are normalised for the raw string, too. So «eval("(cs => cs.raw) `\r`")» is supposed to return «["\n"]».

::: js/src/vm/Opcodes.h
@@ +895,5 @@
>       */ \
>      macro(JSOP_INITELEM_SETTER, 100, "initelem_setter",   NULL, 1,  3,  1, JOF_BYTE|JOF_ELEM|JOF_SET|JOF_DETECTING) \
>      \
> +    /*
> +     * Pushes the specified call site object on to the stack.

"on to" -> "onto"

Comment 8

[guptha]

Attachment: Patch to implement tagged templates v4

Addressed Andre's comments.

Comment 9

[guptha]

Attachment: Patch to implement tagged templates v5

Added guard for the XDR test case.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8453440 [details] [diff] [review]
Patch to implement tagged templates v5

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

guptha and I discussed on IRC and I think there are two bugs: one involving the eval cache, and one involving XDR.

Clearing r? to wait for another revision.

Comment 11

[guptha]

Attachment: Patch to implement tagged templates v6

Added tests to validate the eval cache and XDR frozen behavior; changed call from isFrozen to isExtensible in the XDR code

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8454164 [details] [diff] [review]
Patch to implement tagged templates v6

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

Getting closer...

Please file a follow-up bug to implement JSOP_CALLSITEOBJ in the JITs. It'll be easy, I think!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3837,3 @@
>  
>  bool
>  ParseNode::getConstantValue(ExclusiveContext *cx, bool strictChecks, MutableHandleValue vp)

While you're in here: strictChecks doesn't seem to be used anywhere, so please delete it.

A separate changeset for that (or even a separate bug) would be fine.

@@ +3869,5 @@
> +
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +        bool isArray = false;
> +        if (getKind() == PNK_ARRAY)
> +            isArray = true;

Style nit:
    bool isArray = (getKind() == PNK_ARRAY);

@@ +3900,3 @@
>  
>          RootedObject obj(cx,
> +                         NewDenseAllocatedArray(cx, count, nullptr, MaybeSingletonObject));

Style micro-nit: Now this fits on a single line (of up to 99 characters), right?

@@ +3905,5 @@
>  
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +        if (!isArray) {
> +            RootedObject valueObj(cx);
> +            valueObj = &value.toObject();

Style nit: you can put this on one line
    RootedObject valueObj(cx, &value.toObject());
but why not put this line immediately before the line where valueObj is used?

@@ +3908,5 @@
> +            RootedObject valueObj(cx);
> +            valueObj = &value.toObject();
> +            if (!JSObject::defineProperty(cx, obj, name->asPropertyName(), value,
> +                                          nullptr, nullptr, JSPROP_ENUMERATE))
> +                return false;

Style nit: When the condition is multiline, we add curly braces:

    if (!JSObject::defineProperty(...
                                  ...))
    {
        return false;
    }

@@ +3909,5 @@
> +            valueObj = &value.toObject();
> +            if (!JSObject::defineProperty(cx, obj, name->asPropertyName(), value,
> +                                          nullptr, nullptr, JSPROP_ENUMERATE))
> +                return false;
> +            if (!JSObject::freeze(cx->asJSContext(), valueObj))

See comment below about cx->asJSContext().

@@ +3927,5 @@
> +        JS_ASSERT(idx == count);
> +
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +        if (!isArray) {
> +            if (!JSObject::freeze(cx->asJSContext(), obj))

cx->asJSContext() is a downcast, and therefore it isn't safe unless you've done a type check. Is there some reason it's safe here?

The class hierarchy is

    - class js::ThreadSafeContext
        - class js::ExclusiveContext
            - class JSContext

See jscntxt.h.

I think you can probably just change JSObject::freeze() to take an ExclusiveContext as the first argument, and so on to whatever functions it calls. But I haven't tried it. In any case you can't call cx->asJSContext() without first ensuring that cx->isJSContext(). I was able to get an assertion failure by doing this in the shell:

    offThreadCompileScript("(x=>x)`abc`");
    runOffThreadScript();

::: js/src/frontend/FullParseHandler.h
@@ +125,5 @@
>          return new_<NullaryNode>(PNK_TEMPLATE_STRING, JSOP_NOP, pos, atom);
>      }
> +
> +    /* Call only if code flow ensures the array exists. No checks added. */
> +    ParseNode *getCallSiteObjRawArray(ParseNode *callSiteObj) {

I'll explain what I mean by this in the Parser.cpp review, but the new methods in the ParseHandler interface should be:

    Node newCallSiteObject(uint32_t begin);
    bool addToCallSiteObject(Node callSiteObj, JSAtom *raw, JSAtom *cooked);

Doing it this way means you can delete getCallSiteObjRawArray(), which is nice, since the comment is a little scary!

Never miss a chance to assert. Here, MOZ_ASSERT(callSiteObj->isKind(PNK_CALLSITEOBJ)) would be good.

@@ +132,5 @@
> +
> +    ParseNode *newCallSiteObject(uint32_t begin, unsigned blockid) {
> +        ParseNode *literal = new_<ListNode>(PNK_CALLSITEOBJ, TokenPos(begin, begin + 1));
> +        if (literal)
> +            literal->pn_blockid = blockid;

This method definitely should not take a blockid argument and must not assign to literal->pn_blockid.

ParseNode is a big ugly union; a ListNode uses the 'list' branch of the union and pn_blockid is part of a different branch, the 'name' branch.

I don't think 'literal' is the right name for this; callSite would be OK.

::: js/src/frontend/ParseNode.h
@@ +93,5 @@
>      F(STRING) \
>      F(TEMPLATE_STRING_LIST) \
>      F(TEMPLATE_STRING) \
> +    F(TAGGED_TEMPLATE) \
> +    F(CALLSITEOBJ) \

Don't forget to add entries for the new ParseNodeKinds to the long comment in this file.

::: js/src/frontend/Parser.cpp
@@ +1963,5 @@
> +
> +    while (tt == TOK_TEMPLATE_HEAD) {
> +        Node pn = expr();
> +        if (!pn) {
> +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);

The comments below about error handling in templateLiteral() also apply here.

I usually interpret duplicate bugs as a sign that there shouldn't be duplicate code. Perhaps it would be better to unify the two methods. I'm not sure. I'll leave this up to you.

@@ +1996,5 @@
>      Node pn = noSubstitutionTemplate();
>      if (!pn) {
> +        report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);
> +        return null();
> +    }

This is actually a bug. If noSubstitutionTemplate() returns null, that means it ran into an error, which it has already reported. Reporting again here just replaces the error with a (possibly less specific) error message. Please change it to

    if (!pn)
        return null();

@@ +2004,5 @@
>      TokenKind tt;
>      do {
>          pn = expr();
>          if (!pn) {
> +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);

This must not call report().

@@ +2010,5 @@
>          }
>          handler.addList(nodeList, pn);
>          tt = tokenStream.getToken();
>          if (tt != TOK_RC) {
> +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);

This happens in cases like |`${0)`| and |`${0@`|, where the token immediately after an expression is not }.

This code must call report() if and only if tt != TOK_ERROR.

@@ +2015,5 @@
>              return null();
>          }
>          tt = tokenStream.getToken(TokenStream::TemplateTail);
>          if (tt == TOK_ERROR) {
> +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);

This must not call report().

@@ +2021,5 @@
>          }
>  
>          pn = noSubstitutionTemplate();
>          if (!pn) {
> +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);

This must not call report().

@@ +2313,5 @@
>  
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +template <typename ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::addPropDefnToCallSiteObj(Node literal, const char *cbStr)

All of this is AST-building code. Move it into FullParseHandler::newCallSiteObject()!

@@ +2321,5 @@
> +        cb.append(cbStr[i]);
> +    RootedAtom atom(context);
> +    if (!(atom = AtomizeChars(context, cb.begin(), cb.length())))
> +        return null();
> +    Node propName = handler.newIdentifier(atom, pos());

Don't call AtomizeChars: cx->names().raw is the right JSAtom * to use here.

But it might be better to change the structure so that you don't need this string. Currently it's something like this:

    (CALLSITEOBJ [(COLON (NAME "raw") (ARRAY [(STRING "x") (STRING "y") (STRING "\\n")]))]
                 (TEMPLATE_STRING "x")
                 (TEMPLATE_STRING "y")
                 (TEMPLATE_STRING "\n"))

The string is always the same. It would be possible to eliminate the COLON node, since getConstantValue() is already aware that CALLSITEOBJ nodes are somewhat special. Again, your choice here.

@@ +2336,5 @@
> +
> +// If the call site object does not exist, is created. It is then updated.
> +template <typename ParseHandler>
> +bool
> +Parser<ParseHandler>::createOrUpdateCallSiteObj(Node *callSiteObj, TokenKind tt)

I don't think the "createOrUpdate"-ness here is necessary. Instead, add a call to handler.newCallSiteObject() before the first call to createOrUpdateCallSiteObj() in taggedTemplate(). That way the PNK_CALLSITEOBJ node always exists by the time this is called.

Then rename this method to appendToCallSiteObject and delete the special code that handles the case where callSiteObj is null. Make the argument type of callSiteObj |Node| rather than |Node *|.

Lastly, please move everything you can from this method to a FullParseHandler method:

    bool addToCallSiteObject(ParseNode *callSiteObj, JSAtom *raw, JSAtom *cooked) {
        MOZ_ASSERT(callSiteObj->isKind(PNK_CALLSITEOBJ));
        ...
    }

It should return false only on out-of-memory, like addPropertyDefinition.

@@ +2359,5 @@
> +    JSAtom *atom;
> +    if (tt == TOK_TEMPLATE_HEAD)
> +         atom = tokenStream.getRawTemplateStringAtom(pos().begin + 1, pos().end - pos().begin - 3);
> +    else
> +         atom = tokenStream.getRawTemplateStringAtom(pos().begin + 1, pos().end - pos().begin - 2);

Please consider moving the rather delicate code deciding of how much to add and subtract. It would fit better in TokenStream, which after all is responsible for parsing tokens. Plus I think that would get rid of *all* the arguments:

    JSAtom *atom = tokenStream.getRawTemplateStringAtom();

which looks nice to me. Is that right?

If not, the way you've written it is OK too. But *either* way, please add a comment pointing out that a TOK_TEMPLATE_HEAD looks like either |`...${| or |}...${|, and what the other possibility is, and what that looks like.

@@ +2368,5 @@
> +        return false;
> +
> +    /* We don't know when the last noSubstTemplate will come in, and we
> +     * don't want to deal with this outside this method
> +     */

Style nit: use either // comments:

    // We don't know ...
    // don't want ...

or use SpiderMonkey style for the /**/ comment:

    /*
     * We don't know ...
     * don't want ...
     */

@@ +7035,5 @@
> +                if (
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +                    tt == TOK_LP &&
> +#endif
> +                    atom == context->names().eval) {

Oh, interesting. Hmm.

Please revert this change.

According to the current specification,
    eval`x`
is a direct call to eval, and although that's a little crazy and currently has no observable effects, we might as well implement it that way.

::: js/src/frontend/TokenStream.h
@@ +470,5 @@
>      void reportAsmJSError(uint32_t offset, unsigned errorNumber, ...);
>  
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +    JSAtom *getRawTemplateStringAtom(uint32_t begin, uint32_t length) {
> +        uint32_t cur = begin;

In cases like these, we tend to use pointers:

    jschar *cur = userbuf.base() + begin;
    jschar *end = cur + length;

    while (cur < end) {
        int32_t ch = *cur;

and so on. But do it whichever way seems best.

@@ +477,5 @@
> +            int32_t ch = *(userbuf.base() + cur);
> +            if (ch == '\r') {
> +                ch = '\n';
> +                if (cur + 1 >= begin + length)
> +                    break;

This is a bug:

js> f=(a=>a)
a=>a
js> x=eval("f`\r`")
["\n"]
js> x.raw
[""]

Add a test!

::: js/src/jsreflect.cpp
@@ +1245,5 @@
> +        return false;
> +
> +    return newNode(AST_TAGGED_TEMPLATE, pos,
> +                   "tag", callee,
> +                   "template", array,

Please change these to be named "callee" and "arguments" and update the tests.

That way, Reflect.parse users can treat it just like a CallExpression.

@@ +2834,5 @@
> +            JS_ASSERT(pn->pn_pos.encloses(next->pn_pos));
> +
> +            RootedValue expr(cx);
> +            if (!expression(next, &expr))
> +                return false;

Please make .raw and .cooked arrays of strings, not arrays of Literal nodes.

::: js/src/tests/ecma_6/TemplateStrings/tagTemplTests.js
@@ +1,1 @@
> +// This Source Code Form is subject to the terms of the Mozilla Public

Please don't put "Tests" in test filenames -- it's already under a "tests" directory. "taggedTemplates.js" or "tagged.js" would be fine. Feel free to rename the other one if you like!

@@ +10,5 @@
> +// do the actual evaluation.
> +
> +function testCaseFn() {
> +/*
> +function assertNotEq (actual, expected) {

This is only used once, and there I would instead write something like
   assertEq(foo !== bar, true);

@@ +50,5 @@
> +assertEq(funcRetRawOne``, "");
> +assertEq(funcRetRawOne`${4}a`, "");
> +assertEq(funcRetRawOne`${4}`, "");
> +assertEq(funcRetRawTwo`${4}`, "");
> +assertEq(funcRetRawTwo`a${4}`, "");

I think all these would be clearer (and more thorough tests) if you wrote them like this:

    function check(actual, expected) {
        assertEq(actual.length, expected.length);
        for (var i = 0; i < expected.length; i++)
            assertEq(actual[i], expected[i]);
    }

    function cooked(cs) { return cs; }
    function raw(cs) { return cs.raw; }
    function args(cs, ...rest) { return rest; }

    check(raw``, [""]);
    check(raw`${4}a`, ["", "a"]);
    check(raw`${4}`, ["", ""]);
    check(raw`a${4}`, ["a", ""]);

This also avoids the One=0, Two=1 names which made these hard for me to read :)

@@ +132,5 @@
> +
> +syntaxError("funcRetFirstExpr`${}`");
> +syntaxError("funcRetFirstExpr`${`");
> +syntaxError("funcRetFirstExpr`${\\n}`");
> +syntaxError("funcRetFirstExpr`${yield 0}`");

also:
"funcRetFirstExpr`"
"funcRetFirstExpr`$"
"funcRetFirstExpr`${"
"funcRetFirstExpr.``"

@@ +135,5 @@
> +syntaxError("funcRetFirstExpr`${\\n}`");
> +syntaxError("funcRetFirstExpr`${yield 0}`");
> +
> +// Template substitution tests in the context of tagged templates
> +// Extra whitespace inside a template substitution is ignored.

Also test that whitespace, including comments and newlines, is permitted between the tag and the template.

@@ +202,5 @@
> +
> +var callSiteObj = [];
> +callSiteObj[0] = funcRetCallSiteObj`aa${4}bb`
> +for (var i = 1; i < 3; i++)
> +    callSiteObj[i] = funcRetCallSiteObj`aa${4}bb`

style nit: add semicolons on the two lines here that don't have them.

@@ +217,5 @@
> +// Frozen objects
> +assertEq(Object.isFrozen(callSiteObj[0]), true);
> +assertEq(Object.isFrozen(callSiteObj[0].raw), true);
> +
> +// Allow call syntax

Test that |obj.method`template`| works, and test that |obj| is passed as the this-value to the method.

Test that tagged templates can chain.
    tag `template1` `template2` `template3`
is a valid expression, just like f(arg1)(arg2)(arg3).

@@ +218,5 @@
> +assertEq(Object.isFrozen(callSiteObj[0]), true);
> +assertEq(Object.isFrozen(callSiteObj[0].raw), true);
> +
> +// Allow call syntax
> +var a = new ((cs, sub) => function(){ return sub }) `${[1, 2, 3]}`

Hmm. This is a good thing to be testing, but --- André, I think this should be a syntax error.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-left-hand-side-expressions

The grammar allows both |new MemberExpression Arguments| and |new NewExpression|, but not |new CallExpression|, and a tagged template is a CallExpression.

@@ +221,5 @@
> +// Allow call syntax
> +var a = new ((cs, sub) => function(){ return sub }) `${[1, 2, 3]}`
> +assertEq(a[0], 1)
> +assertEq(a[1], 2)
> +assertEq(a[2], 3)

Add semicolons please.

@@ +225,5 @@
> +assertEq(a[2], 3)
> +
> +var a = [];
> +function test() {
> +    var x = callSite => callSite; for (var i = 0; i < 2; i++) a[i] = eval("x``");

Style nit: Insert some newlines, please.

::: js/src/tests/ecma_6/TemplateStrings/templLitTests.js
@@ +18,5 @@
>          if (e.name === "SyntaxError") {
>              return;
>          }
>      }
> +    throw new TypeError('Expected syntax error: ' + script);

Hmm. It's not really a TypeError. How about just `throw new Error(...)`?

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +97,5 @@
> +                template: templatePart })
> +function templateNoExpr(rawArr, cookedArr) Pattern([{ type: "CallSiteObject", raw: rawArr, cooked: cookedArr }])
> +function templateOneExpr(rawArr, cookedArr, expr1) Pattern([{ type: "CallSiteObject", raw: rawArr,
> +                cooked: cookedArr }, expr1])
> +function templateTwoExpr(rawArr, cookedArr, expr1, expr2) Pattern([{ type: "CallSiteObject", raw: rawArr,

function template(raw, cooked, ...args) Pattern([{type: "CallSiteObject", raw, cooked}, ...args])

::: js/src/vm/Interpreter.cpp
@@ +2740,5 @@
> +CASE(JSOP_CALLSITEOBJ)
> +{
> +    RootedObject &ref = rootObject0;
> +    ref = script->getObject(REGS.pc);
> +    PUSH_OBJECT(*ref);

There's no need to store this in a local variable. You can just
    PUSH_OBJECT(*script->getObject(REGS.pc));

Comment 13

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Oh, interesting. Hmm.
> 
> Please revert this change.
> 
> According to the current specification,
>     eval`x`
> is a direct call to eval, and although that's a little crazy and currently
> has no observable effects, we might as well implement it that way.
> 

Hah, you almost convinced me that there's a spec bug! :-p
« eval`x` » is "MemberExpression :: MemberExpression TemplateLiteral", direct eval calls require a CallExpression (18.2.1.1 [1]). As soon as 12.3.7.1 [2] is finished, direct eval call sites should be more easy to spot in the spec.

[1] https://people.mozilla.org/~jorendorff/es6-draft.html#sec-direct-call-to-eval
[2] https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-calls-runtime-semantics-evaluation


> 
> @@ +218,5 @@
> > +assertEq(Object.isFrozen(callSiteObj[0]), true);
> > +assertEq(Object.isFrozen(callSiteObj[0].raw), true);
> > +
> > +// Allow call syntax
> > +var a = new ((cs, sub) => function(){ return sub }) `${[1, 2, 3]}`
> 
> Hmm. This is a good thing to be testing, but --- André, I think this should
> be a syntax error.
> 
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-left-hand-side-
> expressions
> 
> The grammar allows both |new MemberExpression Arguments| and |new
> NewExpression|, but not |new CallExpression|, and a tagged template is a
> CallExpression.
> 

Shouldn't the following derivation work to get that source string?

LeftHandSideExpression
=> NewExpression
=> new NewExpression
=> new MemberExpression
=> new MemberExpression TemplateLiteral
=> new PrimaryExpression TemplateLiteral
=> new ParenthesizedExpression TemplateLiteral

Comment 14

[guptha]

(In reply to Jason Orendorff [:jorendorff] from comment #12)

> ::: js/src/frontend/Parser.cpp
> @@ +1963,5 @@
> > +
> > +    while (tt == TOK_TEMPLATE_HEAD) {
> > +        Node pn = expr();
> > +        if (!pn) {
> > +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);
> 
> The comments below about error handling in templateLiteral() also apply here.
> 
> I usually interpret duplicate bugs as a sign that there shouldn't be
> duplicate code. Perhaps it would be better to unify the two methods. I'm not
> sure. I'll leave this up to you.
> 

I'm refactoring part of this method into a new method which has some of the common code. I'm reluctant to make this a single method because it's just too messy.

> @@ +2834,5 @@
> > +            JS_ASSERT(pn->pn_pos.encloses(next->pn_pos));
> > +
> > +            RootedValue expr(cx);
> > +            if (!expression(next, &expr))
> > +                return false;
> 
> Please make .raw and .cooked arrays of strings, not arrays of Literal nodes.

Can you explain please? For PNK_STRING, they use the literal call, which is the one I've used.

> 
> Test that tagged templates can chain.
>     tag `template1` `template2` `template3`
> is a valid expression, just like f(arg1)(arg2)(arg3).
> 

Can you explain how you'd want this tested? Should the tag just return a new function each time?

Comment 15

[guptha]

Attachment: Patch to implement tagged templates v7

Not requesting review for now. There are a few more things to be decided/done.

Also, Jason, should I remove AST_TAGGED_TEMPLATE from jsreflect and just use the callExpression method instead of the taggedTemplate method?

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to guptha from comment #14)
> I'm refactoring part of this method into a new method which has some of the
> common code. I'm reluctant to make this a single method because it's just
> too messy.

Good.

> > Please make .raw and .cooked arrays of strings, not arrays of Literal nodes.
> 
> Can you explain please? For PNK_STRING, they use the literal call, which is
> the one I've used.

Yes. I don't think we should do that. The individual parts of a template are not string literals.

I'm not saying you need to redo the whole ParseNode structure, just change the code in jsreflect.cpp to generate a more user-friendly AST. It should be straightforward.

> > Test that tagged templates can chain.
> >     tag `template1` `template2` `template3`
> > is a valid expression, just like f(arg1)(arg2)(arg3).
> 
> Can you explain how you'd want this tested? Should the tag just return a new
> function each time?

Yes. Because it's a possibility according to the spec, test it however you can manage. In reflect-parse.js too!

Comment 17

[guptha]

Attachment: Patch to remove unused strict check param of getConstantValue v1

Comment 18

[guptha]

Attachment: Patch to implement tagged templates v8

This does not contain the asJSContext stuff. I thought that should go into a separate patch/bug, especially if it involves invasive changes.

Also, I've removed AST_TAGGED_TEMPLATE and just used call expression's node in jsreflect. If that's not what you meant, I will revert it.

Comment 19

[Jason Orendorff [:jorendorff]]

Regarding
>var a = new ((cs, sub) => function(){ return sub }) `${[1, 2, 3]}`

(In reply to André Bargull from comment #13)
> Shouldn't the following derivation work to get that source string?
> 
> LeftHandSideExpression
> => NewExpression
> => new NewExpression
> => new MemberExpression
> => new MemberExpression TemplateLiteral
> => new PrimaryExpression TemplateLiteral
> => new ParenthesizedExpression TemplateLiteral

Yes, quite right.

However, the runtime semantics for evaluating this seem to say that this should throw a TypeError, because it's interpreted like

    var a = new (((cs, sub) => function(){ return sub }) `${[1, 2, 3]}`)

and the part in parentheses evaluates to [1, 2, 3], so:

    var a = new [1, 2, 3];

If so, it seems like a spec bug, because that's just perverse, right?

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 8456961 [details] [diff] [review]
Patch to implement tagged templates v8

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

OK, this is in good shape. Just a few minor comments.

I would mark it r+ with these comments addressed, but the ->asJSContext() problem will require some discussion.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3897,5 @@
> +            {
> +                return false;
> +            }
> +            RootedObject valueObj(cx, &value.toObject());
> +            if (!JSObject::freeze(cx->asJSContext(), valueObj))

This can't land without a fix for the cx->asJSContext() issue.

I agree the fix should go in a separate patch/bug, but it has to happen first. :(

::: js/src/frontend/Parser.cpp
@@ +1951,5 @@
>  
>  #ifdef JS_HAS_TEMPLATE_STRINGS
>  template <typename ParseHandler>
> +bool
> +Parser<ParseHandler>::addExprAndGetNextToken(Node nodeList, TokenKind &tt)

The name of this function should indicate that this is something to do with template strings.

@@ +1961,5 @@
> +
> +    tt = tokenStream.getToken();
> +    if (tt != TOK_RC) {
> +        if (tt != TOK_ERROR)
> +            report(ParseError, false, null(), JSMSG_SYNTAX_ERROR);

Improve the error message by adding an entry to js.msg and using it here.

The error message should be "missing } in template string".

@@ +1977,5 @@
> +Parser<ParseHandler>::taggedTemplate(Node nodeList, TokenKind tt)
> +{
> +    Node callSiteObjNode;
> +    if (!(callSiteObjNode = handler.newCallSiteObject(pos().begin, pc->blockidGen)))
> +        return false;

Please rearrange it like:

    Node callSiteObjNode = handler.newCallSiteObject(...);
    if (!callSiteObjNode)
        return false;

::: js/src/frontend/TokenStream.h
@@ +469,5 @@
>      // asm.js reporter
>      void reportAsmJSError(uint32_t offset, unsigned errorNumber, ...);
>  
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +    JSAtom *getRawTemplateStringAtom() {

This method should assert that currentToken().type is either TOK_TEMPLATE_HEAD or TOK_NO_SUBS_TEMPLATE.

@@ +477,5 @@
> +            // Of the form    |`...${|   or   |}...${|
> +            end = userbuf.base() + currentToken().pos.end - 2;
> +        else
> +            // NO_SUBS_TEMPLATE is of the form   |`...`|   or   |}...`|
> +            end = userbuf.base() + currentToken().pos.end - 1;

Style nit: Add curly braces around the multiline if and else statements (even if it's just because of comments).

::: js/src/jsreflect.cpp
@@ +2750,5 @@
>  
>        case PNK_NEW:
> +#ifdef JS_HAS_TEMPLATE_STRINGS
> +      case PNK_TAGGED_TEMPLATE:
> +#endif

You went a bit further than I intended... please keep AST_TAGGED_TEMPLATE and the line that said
>+ASTDEF(AST_TAGGED_TEMPLATE,       "TaggedTemplate",                 "taggedTemplate")
so that the .type of a tagged template node is "TaggedTemplate";
but in all other ways it should look like a CallExpression.

Comment 21

[guptha]

Attachment: Patch to implement tagged templates v9

Comment 22

[guptha]

Attachment: Patch to move raw property definition and freeze to run-time

I'd ideally like to test Baseline's handling of freeze, but am not sure how to do that without using magic numbers in the test. Without magic numbers, the Interpreter always freezes the call site object before the Baseline can get around to it

Comment 23

[Jason Orendorff [:jorendorff]]

(In reply to guptha from comment #22)
> I'd ideally like to test Baseline's handling of freeze, but am not sure how
> to do that without using magic numbers in the test. Without magic numbers,
> the Interpreter always freezes the call site object before the Baseline can
> get around to it

Any test you add under js/src/jit-test/tests will automatically be run (on our test servers) under several different configurations. One of the test configurations uses --baseline-eager, which causes all JS code to run under baseline compiler wherever possible. No magic numbers required!

You can run them that way on your machine using this command:

    python jit_test.py --tbpl -s -o $JS

Or just use --baseline-eagar when you run the js shell.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 8462008 [details] [diff] [review]
Patch to move raw property definition and freeze to run-time

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

r=me with these comments addressed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1059,5 @@
> +EmitObjectPairOp(ExclusiveContext *cx, ObjectBox *objbox1, ObjectBox *objbox2, JSOp op,
> +                 BytecodeEmitter *bce)
> +{
> +    return EmitInternedObjectPairOp(cx, bce->objectList.add(objbox1), bce->objectList.add(objbox2),
> +                                    op, bce);

Since the two indices are guaranteed to be in sequence (index2 == index1 + 1), can we store only one of them in the bytecode, eliminating the need for EmitIndex32Pair?

@@ +3878,5 @@
> +    if (!pn_head->getConstantValue(cx, vp))
> +        return false;
> +    return true;
> +}
> +#endif

Please don't add a new method on ParseNode that must only be used on a particular kind of ParseNode. We have many of those but we'd like to get rid of them.

One nice way to do this is to define a ParseNode subclass. It would live in ParseNode.h (alongside BooleanLiteral etc.) and it would look like this:

    /*
     * A CallSiteNode represents the implicit call site object argument in a TaggedTemplate.
     */
    struct CallSiteNode : public ListNode {
        explicit CallSiteNode(.....) : ListNode(PNK_CALLSITEOBJ, .....) {}

        static bool test(const ParseNode &node) {
            return node.isKind(PNK_CALLSITEOBJ);
        }

        bool getRawArrayValue(ExclusiveContext *cx, MutableHandleValue vp) {
            return pn_head->getConstantValue(cx, vp);
        }
    };

where you would have to figure out the '.....' parts yourself.

Calling it would require an explicit downcast, which would look like this:

    if (!pn->as<CallSiteNode>().getRawArrayValue(cx, &raw))
        return false;

We like this way because explicit downcasts seem like a good idea. pn->as<CallSiteNode>() asserts that CallSiteNode::test(pn) returns true; the assertion is *definitely* a good idea.

The code in FullParseHandler that creates a PNK_CALLSITEOBJ node could also be changed from:
    new_<ListNode>(PNK_CALLSITEOBJ, TokenPos(begin, begin + 1));
to perhaps:
    new_<CallSiteNode>(begin);

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 8458917 [details] [diff] [review]
Patch to implement tagged templates v9

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

r=me. Please merge it with the other patch to land, since ... well, I don't think they make sense separately, you can't build with just one and not the other and expect it to work properly.

It greatly helped reviewing to have two separate patches, though! Thanks.

Comment 26

[guptha]

Attachment: Patch to implement tagged templates v10

Addressed review comments and merged patches into one super-patch.

Comment 27

[guptha]

Attachment: Patch to implement tagged templates v11

Try run fixes.

Comment 28

[guptha]

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

Comment 29

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

That try run has a single canceled build. Clearing checkin-needed until we a finished try run is posted.

Comment 30

[guptha]

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


Aargh! Sorry. Copied the wrong try link.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/479cbe3d30cd

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/479cbe3d30cd

Comment 33

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

Is this fully covered by automatic tests?

Comment 34

[guptha]

Yes, it is.

Comment 35

[Florian Scholz (Open Web Docs)]

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