Bug 142691 – [ES6] Implement ES6 template literals

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[ES6] Implement ES6 template literals

[Ryosuke Niwa]

See examples on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings.

I think this will be mostly a parser change with an additional node for tagged template.

[Yusuke Suzuki]

I'm working on this :)

[Yusuke Suzuki]

Created attachment 249514 [details]
WIP

WIP. Not supporting tagged template literals yet

[Yusuke Suzuki]

tagged templates require some global states and it makes the patch larger.
So in this bug, I'll implement template literals. And later, in subsequent patches, I'll implement tagged templates.

[Yusuke Suzuki]

Created attachment 249671 [details]
Patch

[WebKit Commit Bot]

Attachment 249671 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 249672 [details]
Patch

[WebKit Commit Bot]

Attachment 249672 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 249672 [details]
Patch

Can we guard this feature inside an if-def or runtime flag so that we can disable if we need to just in the case?

[Yusuke Suzuki]

(In reply to comment #9)
> Comment on attachment 249672 [details]
> Patch
> 
> Can we guard this feature inside an if-def or runtime flag so that we can
> disable if we need to just in the case?

Sure! So I'll make it behind the runtime flag, exposing only to inspector, JSC shell and test runner.

[Yusuke Suzuki]

(In reply to comment #10)
> Sure! So I'll make it behind the runtime flag, exposing only to inspector,
> JSC shell and test runner.

After investigating about this, I've found that runtime flags belong to JSGlobalObject, but parser belongs to VM.
For example,
1. To create JSScriptRef in JSC API, we pass JSContextGroupRef, that can be casted to VM.
2. CodeCache belongs to VM

So we cannot use runtime flags for this case. I'll use compile time flags.

[Yusuke Suzuki]

Created attachment 249685 [details]
Patch

[WebKit Commit Bot]

Attachment 249685 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 249685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249685&action=review

This patch looks good to me, but I think we need to find a way to share code before this lands.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:241
> +    TemplateElementListNode* templateElementsTail = m_templateElements;
> +    for (auto templateExpressionsTail = m_templateExpressions; templateExpressionsTail; templateExpressionsTail = templateExpressionsTail->next(), templateElementsTail = templateElementsTail->next()) {

"templateExpressionsTail->next" is duplicated here.

By calling this value "tail", you imply that you're iterating in last-to-first order. But I think you intend to iterate in first-to-last order. So, I would just call this value "templateExpression".

> Source/JavaScriptCore/parser/Lexer.cpp:1226
> +// While JavaScriptCore's lexer accepts <LF><CR> (not <CR><LF>) sequence

I would just call this "the lexer". Comments in JavaScriptCore are about JavaScriptCore.

> Source/JavaScriptCore/parser/Lexer.cpp:1227
> +// as one line terminator and increment one line number,

increment => increments.

> Source/JavaScriptCore/parser/Lexer.cpp:1239
> +// However, line number saved in the lexer should be increment only once for <LF><CR>.

"...the line number saved in the lexer should increment only once..."

> Source/JavaScriptCore/parser/Lexer.cpp:1242
> +// When TemplateLiteral lexer encounters a line terminator, it notify to LineNumberAdder.

notify => notifies.

> Source/JavaScriptCore/parser/Lexer.cpp:1243
> +// LineNumberAdder maintains the status and increments line number when it's necessary.

line number => the line number

> Source/JavaScriptCore/parser/Lexer.cpp:1244
> +// For example, LineNumberAdder increments line number only once for <LF><CR> and <CR><LF>.

line number => the line number

> Source/JavaScriptCore/parser/Lexer.cpp:1347
> +            } else if (UNLIKELY(isLineTerminator(m_current))) {
> +                if (m_current == '\r') {
> +                    lineNumberAdder.add(m_current);
> +                    shift();
> +                    if (m_current == '\n') {
> +                        lineNumberAdder.add(m_current);
> +                        shift();
> +                    }
> +                } else {
> +                    lineNumberAdder.add(m_current);
> +                    shift();
> +                }
> +            } else if (m_current == 'x') {
> +                shift();
> +                if (!isASCIIHexDigit(m_current) || !isASCIIHexDigit(peek(1))) {
> +                    m_lexErrorMessage = ASCIILiteral("\\x can only be followed by a hex character sequence");
> +                    return StringCannotBeParsed;
> +                }
> +                T prev = m_current;
> +                shift();
> +                if (shouldBuildStrings)
> +                    record16(convertHex(prev, m_current));
> +                shift();
> +            } else if (m_current == 'u') {
> +                shift();
> +                UnicodeHexValue character = parseFourDigitUnicodeHex();
> +                if (character.isValid()) {
> +                    if (shouldBuildStrings)
> +                        record16(character.value());
> +                } else {
> +                    m_lexErrorMessage = ASCIILiteral("\\u can only be followed by a Unicode character sequence");
> +                    return character.valueType() == UnicodeHexValue::IncompleteHex ? StringUnterminated : StringCannotBeParsed;
> +                }
> +            } else if (isASCIIDigit(m_current)) {
> +                // The only valid numeric escape in strict mode is '\0', and this must not be followed by a decimal digit.
> +                int character1 = m_current;
> +                shift();
> +                if (character1 != '0' || isASCIIDigit(m_current)) {
> +                    m_lexErrorMessage = ASCIILiteral("The only valid numeric escape in strict mode is '\\0'");
> +                    return StringCannotBeParsed;
> +                }
> +                if (shouldBuildStrings)
> +                    record16(0);
> +            } else if (!atEnd()) {
> +                if (shouldBuildStrings)
> +                    record16(m_current);
> +                shift();
> +            } else {
> +                m_lexErrorMessage = ASCIILiteral("Unterminated string constant");
> +                return StringUnterminated;
> +            }

Can you write a helper function to share this code with parseString? This is a lot of code to duplicate, and it looks like the only difference we want is the line numbering difference, which is already pretty well factored out into the LineNumberAdder class.

> Source/JavaScriptCore/parser/Lexer.cpp:1403
> +        // TODO: raw is not implemented correctly yet.
> +        // Line terminator normalization is not done for raw.
> +        // When implementing TaggedTemplate which uses raw value, it should be fixed.

What is the consequence of the missing raw implementation? This comment should specify what happens when you parse a raw value.

[Yusuke Suzuki]

Comment on attachment 249685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249685&action=review

Thank you for your review. I've commented. And I'll updated the patch.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:241
>> +    for (auto templateExpressionsTail = m_templateExpressions; templateExpressionsTail; templateExpressionsTail = templateExpressionsTail->next(), templateElementsTail = templateElementsTail->next()) {
> 
> "templateExpressionsTail->next" is duplicated here.
> 
> By calling this value "tail", you imply that you're iterating in last-to-first order. But I think you intend to iterate in first-to-last order. So, I would just call this value "templateExpression".

Ah, not duplicated. `templateExpressionsTail->next` and `templateElementsTail->next`. expressions and elements.
But, maybe it's confusing. I'll rename TemplateElement to TemplateString. It's clearer.

And agree to your suggestion about "tail". OK, I'll simply use "templateExpression" :D

>> Source/JavaScriptCore/parser/Lexer.cpp:1226
>> +// While JavaScriptCore's lexer accepts <LF><CR> (not <CR><LF>) sequence
> 
> I would just call this "the lexer". Comments in JavaScriptCore are about JavaScriptCore.

Thx! I've fixed it.

>> Source/JavaScriptCore/parser/Lexer.cpp:1227
>> +// as one line terminator and increment one line number,
> 
> increment => increments.

Done.

>> Source/JavaScriptCore/parser/Lexer.cpp:1239
>> +// However, line number saved in the lexer should be increment only once for <LF><CR>.
> 
> "...the line number saved in the lexer should increment only once..."

Done.

>> Source/JavaScriptCore/parser/Lexer.cpp:1242
>> +// When TemplateLiteral lexer encounters a line terminator, it notify to LineNumberAdder.
> 
> notify => notifies.

Done.

>> Source/JavaScriptCore/parser/Lexer.cpp:1243
>> +// LineNumberAdder maintains the status and increments line number when it's necessary.
> 
> line number => the line number

Done.

>> Source/JavaScriptCore/parser/Lexer.cpp:1244
>> +// For example, LineNumberAdder increments line number only once for <LF><CR> and <CR><LF>.
> 
> line number => the line number

Done.

>> Source/JavaScriptCore/parser/Lexer.cpp:1347
>> +            }
> 
> Can you write a helper function to share this code with parseString? This is a lot of code to duplicate, and it looks like the only difference we want is the line numbering difference, which is already pretty well factored out into the LineNumberAdder class.

Yes! I've extracted complicated escape code for \u, \x and \0 to parseComplexEscape.

>> Source/JavaScriptCore/parser/Lexer.cpp:1403
>> +        // When implementing TaggedTemplate which uses raw value, it should be fixed.
> 
> What is the consequence of the missing raw implementation? This comment should specify what happens when you parse a raw value.

Line terminator normalization (e.g. <CR> => <LF>) should be applied to both the raw and cooked representations.
But it doesn't do that for the raw representation.
So when parsing `<CR>`, <CR> appears in the raw representation of the result.

While non-tagged template literals don't use the raw representation, tagged templates use it.
So when implementing tagged templates, we should apply line terminator normalization to the raw representation.

OK! I've noted the above description here :D

[Yusuke Suzuki]

Created attachment 249969 [details]
Patch

[WebKit Commit Bot]

Attachment 249969 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 250029 [details]
Patch

[WebKit Commit Bot]

Attachment 250029 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Hi, the patch becomes ready for review.
Could you review this?

[Yusuke Suzuki]

could you review this?

[Yusuke Suzuki]

Created attachment 251090 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 251090 [details]
Patch

Could anyone review this patch? Just rebased the patch.

[WebKit Commit Bot]

Attachment 251090 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

ping?

[Joseph Pecoraro]

Comment on attachment 251090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251090&action=review

Very cool! I don't think I'm fully qualified to review this but I added some comments.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:896
> +    linkSlowCase(iter);
> +    linkSlowCase(iter);

Should this only be called once?

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:802
> +    linkSlowCase(iter);
> +    linkSlowCase(iter);

Should this only be called once?

> Source/JavaScriptCore/parser/Lexer.cpp:1320
> +            // Most common escape sequences first

Style: Comments should be sentences that end with a period.

> Source/JavaScriptCore/parser/Lexer.cpp:1399
> +        // While non-tagged template literals don't use the raw representation, tagged templates use the raw representaion.

Typo: "representaion" => "representation".

> Source/JavaScriptCore/parser/Lexer.cpp:1400
> +        // So line terminator nomralization should be applied to the raw representation when implementing tagged templates.

Typo: "nomralization" => "normalization"

> Source/JavaScriptCore/tests/stress/template-literal-syntax.js:51
> +testSyntaxError("`Hello${}", "SyntaxError: Unexpected token '}'");

I wonder if we can provide a better error message here. Something like: "SyntaxError: Template literal expression cannot be empty" may be more informative to users.

> Source/JavaScriptCore/tests/stress/template-literal.js:15
> +test(`Hello World`, "Hello World");

Tests I'd like to see but don't:

Empty template!

    test(``, "");

Nested templates:

    test(`Hello ${ `Cocoa` }`, "Hello Cocoa");
    test(`Hello ${ `Co${`c`}oa` }`, "Hello Cocoa");

Immediately adjacent template expressions, with empty intermediate template strings:

    test(`${"C"}${"o"}${"c"}${"o"}${"a"}`, "Cocoa");

[Yusuke Suzuki]

Comment on attachment 251090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251090&action=review

Thank you for your comment! Very helpful.

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:896
>> +    linkSlowCase(iter);
> 
> Should this only be called once?

In the JIT emitSlow op case, we need to call `linkSlowCase` the same times as times of calling `addSlowCase` in the fast op emitting function.
In this case, we called `addSlowCase` twice, so we need to call `linkSlowCase` twice.

But right. The current code is not good for readability. So I'll add comments about which `addSlowCase` is corresponding to the specific `linkSlowCase`.

>> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:802
>> +    linkSlowCase(iter);
> 
> Should this only be called once?

same as above.

>> Source/JavaScriptCore/parser/Lexer.cpp:1320
>> +            // Most common escape sequences first
> 
> Style: Comments should be sentences that end with a period.

Fixed! Thank you!

>> Source/JavaScriptCore/parser/Lexer.cpp:1399
>> +        // While non-tagged template literals don't use the raw representation, tagged templates use the raw representaion.
> 
> Typo: "representaion" => "representation".

Oops. Thank you. Fixed.

>> Source/JavaScriptCore/parser/Lexer.cpp:1400
>> +        // So line terminator nomralization should be applied to the raw representation when implementing tagged templates.
> 
> Typo: "nomralization" => "normalization"

Fixed.

>> Source/JavaScriptCore/tests/stress/template-literal-syntax.js:51
>> +testSyntaxError("`Hello${}", "SyntaxError: Unexpected token '}'");
> 
> I wonder if we can provide a better error message here. Something like: "SyntaxError: Template literal expression cannot be empty" may be more informative to users.

Sounds really nice!

>> Source/JavaScriptCore/tests/stress/template-literal.js:15
>> +test(`Hello World`, "Hello World");
> 
> Tests I'd like to see but don't:
> 
> Empty template!
> 
>     test(``, "");
> 
> Nested templates:
> 
>     test(`Hello ${ `Cocoa` }`, "Hello Cocoa");
>     test(`Hello ${ `Co${`c`}oa` }`, "Hello Cocoa");
> 
> Immediately adjacent template expressions, with empty intermediate template strings:
> 
>     test(`${"C"}${"o"}${"c"}${"o"}${"a"}`, "Cocoa");

That's really nice!

[Yusuke Suzuki]

Created attachment 251490 [details]
Patch

[WebKit Commit Bot]

Attachment 251490 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 251490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251490&action=review

> Source/JavaScriptCore/parser/Lexer.cpp:76
> +#if ENABLE(ES6_TEMPLATE_LITERAL_SYNTAX)
> +    CharacterBackQuote,
> +#endif

I would have made this unconditional; I think it does little harm to add one more character enum value even when this feature is off.

> Source/JavaScriptCore/parser/Lexer.cpp:1249
> +    m_buffer16.resize(0);

Always more efficient, I think, to use shrink(0) rather than resize(0).

> Source/JavaScriptCore/parser/Lexer.cpp:1273
> +template<typename T>

I would call these types CharacterType instead of T everywhere.

> Source/JavaScriptCore/parser/Lexer.cpp:1278
> +        , m_previous(0)

I would initialize this below rather than in the constructor.

> Source/JavaScriptCore/parser/Lexer.cpp:1290
> +        if ((character + m_previous) == '\n' + '\r')

Strange to use parentheses on one side of the expression and to on the other. If they are helpful on one side they are equally helpful on the other side. I would leave both sets out.

> Source/JavaScriptCore/parser/Lexer.cpp:1300
> +    T m_previous;

I’d initialize here in one of these two ways:

    T m_previous { };
    T m_previous { 0 };

> Source/JavaScriptCore/parser/Lexer.h:212
> +    enum class EscapeParseMode {
> +        Template,
> +        String
> +    };

I’d format this on a single line, not vertically.

> Source/JavaScriptCore/parser/NodeConstructors.h:105
> +        : m_next(nullptr)

Better to initialize m_next below instead of in all these constructors.

> Source/JavaScriptCore/parser/NodeConstructors.h:130
> +    inline TemplateStringListNode::TemplateStringListNode(TemplateStringListNode* prev, TemplateStringNode* node)

Why not write out the word “previous” instead of the abbreviation “prev”?

> Source/JavaScriptCore/parser/Nodes.h:440
> +        TemplateExpressionListNode* m_next;

Should initialize in one of these two ways:

    TemplateExpressionListNode* m_next { nullptr };
    TemplateExpressionListNode* m_next { };

I’d even do the same for fields like m_node that are always initialized “just in case”.

> ChangeLog:22
> [phone]  Yusuke Suzuki  <[e-mail]>
> +
> +        [ES6] Implement ES6 template literals
> +        https://bugs.webkit.org/show_bug.cgi?id=142691
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/cmake/WebKitFeatures.cmake:
> +        * Source/cmakeconfig.h.cmake:
> +
> [phone]  Yusuke Suzuki  <[e-mail]>
> +
> +        [ES6] Implement ES6 template literals
> +        https://bugs.webkit.org/show_bug.cgi?id=142691
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add ENABLE_ES6_TEMPLATE_LITERAL_SYNTAX compile time flag.
> +
> +        * Source/cmake/WebKitFeatures.cmake:
> +        * Source/cmakeconfig.h.cmake:
> +

Double change log here.

[Yusuke Suzuki]

Comment on attachment 251490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251490&action=review

Thank you for your review!

>> Source/JavaScriptCore/parser/Lexer.cpp:76
>> +#endif
> 
> I would have made this unconditional; I think it does little harm to add one more character enum value even when this feature is off.

Right. I've dropped it.

>> Source/JavaScriptCore/parser/Lexer.cpp:1249
>> +    m_buffer16.resize(0);
> 
> Always more efficient, I think, to use shrink(0) rather than resize(0).

Changed resize(0) => shrink(0) in Lexer.cpp.

>> Source/JavaScriptCore/parser/Lexer.cpp:1273
>> +template<typename T>
> 
> I would call these types CharacterType instead of T everywhere.

Nice. Changed.

>> Source/JavaScriptCore/parser/Lexer.cpp:1290
>> +        if ((character + m_previous) == '\n' + '\r')
> 
> Strange to use parentheses on one side of the expression and to on the other. If they are helpful on one side they are equally helpful on the other side. I would leave both sets out.

Thanks! I've changed to use parentheses in both side.

>> Source/JavaScriptCore/parser/Lexer.cpp:1300
>> +    T m_previous;
> 
> I’d initialize here in one of these two ways:
> 
>     T m_previous { };
>     T m_previous { 0 };

Very nice C++11 feature!

>> Source/JavaScriptCore/parser/Lexer.h:212
>> +    };
> 
> I’d format this on a single line, not vertically.

Done.

>> Source/JavaScriptCore/parser/NodeConstructors.h:105
>> +        : m_next(nullptr)
> 
> Better to initialize m_next below instead of in all these constructors.

OK, use C++11 feature.

>> Source/JavaScriptCore/parser/NodeConstructors.h:130
>> +    inline TemplateStringListNode::TemplateStringListNode(TemplateStringListNode* prev, TemplateStringNode* node)
> 
> Why not write out the word “previous” instead of the abbreviation “prev”?

Fixed. Use "previous".

>> Source/JavaScriptCore/parser/Nodes.h:440
>> +        TemplateExpressionListNode* m_next;
> 
> Should initialize in one of these two ways:
> 
>     TemplateExpressionListNode* m_next { nullptr };
>     TemplateExpressionListNode* m_next { };
> 
> I’d even do the same for fields like m_node that are always initialized “just in case”.

ok, changed to `m_next { nullptr };`. And `m_node { nullptr }` "just in case".

>> ChangeLog:22
>> +
> 
> Double change log here.

Oops! Thanks.

[Yusuke Suzuki]

Committed r183373: <http://trac.webkit.org/changeset/183373>

[Csaba Osztrogonác]

(In reply to comment #32)
> Committed r183373: <http://trac.webkit.org/changeset/183373>

The new stress/template-literal-syntax.js test asserts everywhere,
see https://build.webkit.org/waterfall for details and please fix.

[Yusuke Suzuki]

(In reply to comment #33)
> (In reply to comment #32)
> > Committed r183373: <http://trac.webkit.org/changeset/183373>
> 
> The new stress/template-literal-syntax.js test asserts everywhere,
> see https://build.webkit.org/waterfall for details and please fix.

Thank you. I'll investigate it.

[Yusuke Suzuki]

Opened. https://bugs.webkit.org/show_bug.cgi?id=144257