Add support for quasi literals.

Add support for quasi literals.

  var x = 3;
  var y = 5;
  console.log(`$x + $y = ${x + y}`);

BUG=http://code.google.com/p/traceur-compiler/issues/detail?id=47
TEST=test/feature/QuasiLiterals/*

Committed: http://code.google.com/p/traceur-compiler/source/detail?r=335

[arv, 2011-10-10 07:15:47]

https://codereview.appspot.com/5244056/diff/1/src/syntax/trees/ParseTree.js
File src/syntax/trees/ParseTree.js (right):

https://codereview.appspot.com/5244056/diff/1/src/syntax/trees/ParseTree.js#n...
src/syntax/trees/ParseTree.js:151: case ParseTreeType.QUASI_LITERAL_EXPRESSION:
Sorted.

I only added the QUASI_LITERAL_EXPRESSION

[arv, 2011-10-10 17:06:26]

Last night I realized that I forgot to externalize the callsite
object. I'm fine doing that in this patch or in a follow up patch.
Either way works form me.

The change to support externalizing should not be a big change...
probably around 20 more lines so not a big deal.

erik








On Mon, Oct 10, 2011 at 00:15,  <arv@chromium.org> wrote:
> Reviewers: jmesserly, cburrows,
>
>
> https://codereview.appspot.com/5244056/diff/1/src/syntax/trees/ParseTree.js
> File src/syntax/trees/ParseTree.js (right):
>
>
https://codereview.appspot.com/5244056/diff/1/src/syntax/trees/ParseTree.js#n...
> src/syntax/trees/ParseTree.js:151: case
> ParseTreeType.QUASI_LITERAL_EXPRESSION:
> Sorted.
>
> I only added the QUASI_LITERAL_EXPRESSION
>
> Description:
> Add support for quasi literals.
>
>  var x = 3;
>  var y = 5;
>  console.log(`$x + $y = ${x + y}`);
>
> BUG=None
> TEST=None
>
>
> Please review this at https://codereview.appspot.com/5244056/
>
> Affected files:
>  M src/codegeneration/ParseTreeTransformer.js
>  M src/codegeneration/ParseTreeWriter.js
>  M src/codegeneration/ProgramTransformer.js
>  A src/codegeneration/QuasiLiteralTransformer.js
>  M src/runtime/runtime.js
>  M src/syntax/ParseTreeValidator.js
>  M src/syntax/ParseTreeVisitor.js
>  M src/syntax/Parser.js
>  M src/syntax/PredefinedName.js
>  M src/syntax/Scanner.js
>  M src/syntax/TokenType.js
>  M src/syntax/trees/ParseTree.js
>  M src/syntax/trees/ParseTrees.js
>  M src/traceur.js
>  A test/feature/QuasiLiterals/Default.js
>  A test/feature/QuasiLiterals/Error_InvalidSubstitution.js
>  A test/feature/QuasiLiterals/Error_InvalidSubstitution2.js
>  A test/feature/QuasiLiterals/Error_NoWhitespaceAllowed.js
>  A test/feature/QuasiLiterals/Error_NotClosed.js
>  A test/feature/QuasiLiterals/Tag.js
>  M test/feature/feature_test.html
>
>
>

[arv, 2011-10-10 19:12:51]

The callsite id is now externalized.

[Mike Samuel, 2011-10-10 22:01:51]

I can't view Tag.js in unified diff mode since
http://codereview.appspot.com/5244056/patch/6001/7020 contains lines with no
diff marker:

+  assertArrayEquals(['\u2028', '\\u2028'], expose`

${x}\u2028`[0].raw);


Do you have any tests for a quasi with an explicit handler?

http://codereview.appspot.com/5244056/diff/6001/test/feature/QuasiLiterals/De...
File test/feature/QuasiLiterals/Default.js (right):

http://codereview.appspot.com/5244056/diff/6001/test/feature/QuasiLiterals/De...
test/feature/QuasiLiterals/Default.js:27: assertEquals('$DOLLAR', `\$$$`);
Nice testcases.

[arv, 2011-10-10 22:05:43]

On 2011/10/10 22:01:51, Mike Samuel wrote:
> I can't view Tag.js in unified diff mode since
> http://codereview.appspot.com/5244056/patch/6001/7020 contains lines with no
> diff marker:
> 
> +  assertArrayEquals(['\u2028', '\\u2028'], expose`

> ${x}\u2028`[0].raw);

I guess Rietveld doesn't handle \u2028 correctly

> Do you have any tests for a quasi with an explicit handler?

I'm not sure what you mean here. Can you give an example?

[Mike Samuel, 2011-10-10 22:22:11]

On Mon, Oct 10, 2011 at 15:05,  <arv@chromium.org> wrote:
> On 2011/10/10 22:01:51, Mike Samuel wrote:
>>
>> I can't view Tag.js in unified diff mode since
>> http://codereview.appspot.com/5244056/patch/6001/7020 contains lines
>
> with no
>>
>> diff marker:
>
>> +  assertArrayEquals(['\u2028', '\\u2028'], expose`
>> ${x}\u2028`[0].raw);
>
> I guess Rietveld doesn't handle \u2028 correctly
>
>> Do you have any tests for a quasi with an explicit handler?
>
> I'm not sure what you mean here. Can you give an example?

f`x${y}z` should desugar to roughly

const _callsiteId1234_ = { raw: ['x', 'z'], cooked ['x', 'z'] };  // hoisted
f(_callsiteId1234_, y)  // in-place

[arv, 2011-10-10 22:31:45]

On Mon, Oct 10, 2011 at 15:22, ☻Mike Samuel <msamuel@google.com> wrote:
> f`x${y}z` should desugar to roughly
>
> const _callsiteId1234_ = { raw: ['x', 'z'], cooked ['x', 'z'] };  // hoisted
> f(_callsiteId1234_, y)  // in-place

The test Tag.js uses the a quasi tag called expose.

Here is the desugared code for your reference:

var f, y;
f`x${y}z`

desugars to

var $__0 = Object.freeze({
  raw: Object.freeze(["x", "z"]),
  cooked: Object.freeze(["x", "z"])
});
var f, y;
f($__0, y);

The intermediate desugar has a const in it too so it is identical to
what you wrote on the ES wiki.

[Mike Samuel, 2011-10-10 22:38:53]

On Mon, Oct 10, 2011 at 15:31, Erik Arvidsson <arv@chromium.org> wrote:
> On Mon, Oct 10, 2011 at 15:22, ☻Mike Samuel <msamuel@google.com> wrote:
>> f`x${y}z` should desugar to roughly
>>
>> const _callsiteId1234_ = { raw: ['x', 'z'], cooked ['x', 'z'] };  // hoisted
>> f(_callsiteId1234_, y)  // in-place
>
> The test Tag.js uses the a quasi tag called expose.
>
> Here is the desugared code for your reference:
>
> var f, y;
> f`x${y}z`
>
> desugars to
>
> var $__0 = Object.freeze({
>  raw: Object.freeze(["x", "z"]),
>  cooked: Object.freeze(["x", "z"])
> });
> var f, y;
> f($__0, y);
>
> The intermediate desugar has a const in it too so it is identical to
> what you wrote on the ES wiki.
>
Great.

[arv, 2011-10-12 21:59:02]

John, Chris? Do you want me to find some other reviewer?

erik








On Mon, Oct 10, 2011 at 15:38, ☻Mike Samuel <msamuel@google.com> wrote:
> On Mon, Oct 10, 2011 at 15:31, Erik Arvidsson <arv@chromium.org> wrote:
>> On Mon, Oct 10, 2011 at 15:22, ☻Mike Samuel <msamuel@google.com> wrote:
>>> f`x${y}z` should desugar to roughly
>>>
>>> const _callsiteId1234_ = { raw: ['x', 'z'], cooked ['x', 'z'] };  // hoisted
>>> f(_callsiteId1234_, y)  // in-place
>>
>> The test Tag.js uses the a quasi tag called expose.
>>
>> Here is the desugared code for your reference:
>>
>> var f, y;
>> f`x${y}z`
>>
>> desugars to
>>
>> var $__0 = Object.freeze({
>>  raw: Object.freeze(["x", "z"]),
>>  cooked: Object.freeze(["x", "z"])
>> });
>> var f, y;
>> f($__0, y);
>>
>> The intermediate desugar has a const in it too so it is identical to
>> what you wrote on the ES wiki.
>>
> Great.
>

[cburrows, 2011-10-12 22:27:29]

I started to look but got side-tracked. I'll take a look tonight!

chris

On Wed, Oct 12, 2011 at 2:58 PM, Erik Arvidsson <arv@chromium.org> wrote:

> John, Chris? Do you want me to find some other reviewer?
>
> erik
>
>
>
>
>
>
>
>
> On Mon, Oct 10, 2011 at 15:38, ☻Mike Samuel <msamuel@google.com> wrote:
> > On Mon, Oct 10, 2011 at 15:31, Erik Arvidsson <arv@chromium.org> wrote:
> >> On Mon, Oct 10, 2011 at 15:22, ☻Mike Samuel <msamuel@google.com> wrote:
> >>> f`x${y}z` should desugar to roughly
> >>>
> >>> const _callsiteId1234_ = { raw: ['x', 'z'], cooked ['x', 'z'] };  //
> hoisted
> >>> f(_callsiteId1234_, y)  // in-place
> >>
> >> The test Tag.js uses the a quasi tag called expose.
> >>
> >> Here is the desugared code for your reference:
> >>
> >> var f, y;
> >> f`x${y}z`
> >>
> >> desugars to
> >>
> >> var $__0 = Object.freeze({
> >>  raw: Object.freeze(["x", "z"]),
> >>  cooked: Object.freeze(["x", "z"])
> >> });
> >> var f, y;
> >> f($__0, y);
> >>
> >> The intermediate desugar has a const in it too so it is identical to
> >> what you wrote on the ES wiki.
> >>
> > Great.
> >
>

[John Messerly, 2011-10-12 23:34:31]

Thanks Chris. I'm still recovering from Dart tech preview launch :)
(or more accurately ... all the work leading up to Dart launch)

On Wed, Oct 12, 2011 at 3:27 PM, Chris Burrows <cburrows@google.com> wrote:

> I started to look but got side-tracked. I'll take a look tonight!
>
> chris
>
>
> On Wed, Oct 12, 2011 at 2:58 PM, Erik Arvidsson <arv@chromium.org> wrote:
>
>> John, Chris? Do you want me to find some other reviewer?
>>
>> erik
>>
>>
>>
>>
>>
>>
>>
>>
>> On Mon, Oct 10, 2011 at 15:38, ☻Mike Samuel <msamuel@google.com> wrote:
>> > On Mon, Oct 10, 2011 at 15:31, Erik Arvidsson <arv@chromium.org> wrote:
>> >> On Mon, Oct 10, 2011 at 15:22, ☻Mike Samuel <msamuel@google.com>
>> wrote:
>> >>> f`x${y}z` should desugar to roughly
>> >>>
>> >>> const _callsiteId1234_ = { raw: ['x', 'z'], cooked ['x', 'z'] };  //
>> hoisted
>> >>> f(_callsiteId1234_, y)  // in-place
>> >>
>> >> The test Tag.js uses the a quasi tag called expose.
>> >>
>> >> Here is the desugared code for your reference:
>> >>
>> >> var f, y;
>> >> f`x${y}z`
>> >>
>> >> desugars to
>> >>
>> >> var $__0 = Object.freeze({
>> >>  raw: Object.freeze(["x", "z"]),
>> >>  cooked: Object.freeze(["x", "z"])
>> >> });
>> >> var f, y;
>> >> f($__0, y);
>> >>
>> >> The intermediate desugar has a const in it too so it is identical to
>> >> what you wrote on the ES wiki.
>> >>
>> > Great.
>> >
>>
>
>

[cburrows, 2011-10-13 21:46:22]

I am a little nervous about the scanner scanning inside the literal. It seems
like you want to be able to get the parser and the scanner to communicate state,
or you want the scanner to tear out the literal `` like a string and scan it
separately (with the simpler grammer).

Maybe I'm projecting about embedded languages. I ripped my hair out for a long
time getting a C# parser/scanner to deal with xml doc comments embedded in
source, scanned inline. When it would have been so much more easy in the long
run to just rip out the comment and hand it over to an xml parser.

LGTM otherwise

https://codereview.appspot.com/5244056/diff/2001/src/syntax/Scanner.js
File src/syntax/Scanner.js (right):

https://codereview.appspot.com/5244056/diff/2001/src/syntax/Scanner.js#newcod...
src/syntax/Scanner.js:880: }
Why introduce a new token type for what is just an optional identifier in the
production?

https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
File src/codegeneration/QuasiLiteralTransformer.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
src/codegeneration/QuasiLiteralTransformer.js:90: replace(/(^|[^\\])"/g,
'$1\\"') +
I think I must not understand what is happening here. You're escaping quotes,
but leaving \" alone? What if you see \\"?

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js
File src/syntax/Parser.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
src/syntax/Parser.js:3329: * We diverge from the harmony wiki by allowing
arbitrary expressions inside
Did you do this to get the implementation up quickly or is the wiki wrong?

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
src/syntax/Parser.js:3368: var expression = this.parseExpression_();
I think I prefer the limited grammar. Nested quasis seem wrong. I want `` to
behave string-like, which means inner ` should be quoted.

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Scanner.js
File src/syntax/Scanner.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Scanner.js#newcod...
src/syntax/Scanner.js:498: }
Spec: No error otherwise? Should $ be escaped in a quasi?

https://codereview.appspot.com/5244056/diff/6001/src/syntax/TokenType.js
File src/syntax/TokenType.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/syntax/TokenType.js#newc...
src/syntax/TokenType.js:142: QUASI_TAG: 'quasi tag',
It seems weird to have a different token kind for this, why not just use an
identifier token?

[arv, 2011-10-13 22:39:52]

I agree that this is a bit tricky. Just like for regexps I think this requires a
special scanning mode which is what I implemented.

I don't think it is doable to let the scanner get the whole quasi literal and
then do another parsing pass on that. That does not work with arbitrary
expressions which is something that I want to support.

I fixed the comment case so that the following now works:

quasi/* comment */`text`

Mike: I assume this is desired?

https://codereview.appspot.com/5244056/diff/2001/src/syntax/Scanner.js
File src/syntax/Scanner.js (right):

https://codereview.appspot.com/5244056/diff/2001/src/syntax/Scanner.js#newcod...
src/syntax/Scanner.js:880: }
On 2011/10/13 21:46:22, cburrows wrote:
> Why introduce a new token type for what is just an optional identifier in the
> production?

I did it this way for a few reasons.

The quasi tag needs to be directly in front of the BACK_QUOTE and I need a way
to differentiate the following two cases.

ident `abc`
ident`abc`

Maybe it would be better to do something like peekNextChar from the parser where
it parses the identifier expression? However, I do think we already skip the
whitespace in that case.

This makes me realize that the following doesn't currently work correctly:

ident/* */`abc`

since we skip the comment in peek_.

https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
File src/codegeneration/QuasiLiteralTransformer.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
src/codegeneration/QuasiLiteralTransformer.js:90: replace(/(^|[^\\])"/g,
'$1\\"') +
On 2011/10/13 21:46:22, cburrows wrote:
> I think I must not understand what is happening here. You're escaping quotes,
> but leaving \" alone? What if you see \\"?

The value does not contain the wrapping quotes so we add that. We therefore must
replace all raw " characters with \". However, if the string contains \" we do
not want to replace that with \\". This is a bit tricky but there are tests
covering this.

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js
File src/syntax/Parser.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
src/syntax/Parser.js:3329: * We diverge from the harmony wiki by allowing
arbitrary expressions inside
On 2011/10/13 21:46:22, cburrows wrote:
> Did you do this to get the implementation up quickly or is the wiki wrong?

The wiki has this as an open issue and Mike's prototype also supports
expressions. I find that supporting expressions is very important and I will
bring it up to the committee.

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
src/syntax/Parser.js:3368: var expression = this.parseExpression_();
On 2011/10/13 21:46:22, cburrows wrote:
> I think I prefer the limited grammar. Nested quasis seem wrong. I want `` to
> behave string-like, which means inner ` should be quoted.

With the limited grammar the following would not work.

`$x + $y = ${x + y}`

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Scanner.js
File src/syntax/Scanner.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/syntax/Scanner.js#newcod...
src/syntax/Scanner.js:498: }
On 2011/10/13 21:46:22, cburrows wrote:
> Spec: No error otherwise? Should $ be escaped in a quasi?

The spec allows the following:

`a $ b`

I'll make sure this is covered in a test.

https://codereview.appspot.com/5244056/diff/6001/src/syntax/TokenType.js
File src/syntax/TokenType.js (right):

https://codereview.appspot.com/5244056/diff/6001/src/syntax/TokenType.js#newc...
src/syntax/TokenType.js:142: QUASI_TAG: 'quasi tag',
On 2011/10/13 21:46:22, cburrows wrote:
> It seems weird to have a different token kind for this, why not just use an
> identifier token?

See other comment

[Mike Samuel, 2011-10-14 16:29:06]

On Thu, Oct 13, 2011 at 18:39,  <arv@chromium.org> wrote:
> I agree that this is a bit tricky. Just like for regexps I think this
> requires a special scanning mode which is what I implemented.
>
> I don't think it is doable to let the scanner get the whole quasi
> literal and then do another parsing pass on that. That does not work
> with arbitrary expressions which is something that I want to support.
>
> I fixed the comment case so that the following now works:
>
> quasi/* comment */`text`
>
> Mike: I assume this is desired?

Correct.  The way I implemented this in the patch attached to
https://bugzilla.mozilla.org/show_bug.cgi?id=688857 allows any member
expression followed by a TOK_QUASISTART (e.g. `foo ${`) token or
TOK_QUASIEND (e.g. `foo`) token.  By the point member expression is
parsed, comments have already been elided from the token stream.

TC39 of course might end up deciding otherwise.

> https://codereview.appspot.com/5244056/diff/2001/src/syntax/Scanner.js
> File src/syntax/Scanner.js (right):
>
>
https://codereview.appspot.com/5244056/diff/2001/src/syntax/Scanner.js#newcod...
> src/syntax/Scanner.js:880: }
> On 2011/10/13 21:46:22, cburrows wrote:
>>
>> Why introduce a new token type for what is just an optional identifier
>
> in the
>>
>> production?
>
> I did it this way for a few reasons.
>
> The quasi tag needs to be directly in front of the BACK_QUOTE and I need
> a way to differentiate the following two cases.
>
> ident `abc`
> ident`abc`
>
> Maybe it would be better to do something like peekNextChar from the
> parser where it parses the identifier expression? However, I do think we
> already skip the whitespace in that case.
>
> This makes me realize that the following doesn't currently work
> correctly:
>
> ident/* */`abc`
>
> since we skip the comment in peek_.
>
>
https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
> File src/codegeneration/QuasiLiteralTransformer.js (right):
>
>
https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
> src/codegeneration/QuasiLiteralTransformer.js:90: replace(/(^|[^\\])"/g,
> '$1\\"') +
> On 2011/10/13 21:46:22, cburrows wrote:
>>
>> I think I must not understand what is happening here. You're escaping
>
> quotes,
>>
>> but leaving \" alone? What if you see \\"?
>
> The value does not contain the wrapping quotes so we add that. We
> therefore must replace all raw " characters with \". However, if the
> string contains \" we do not want to replace that with \\". This is a
> bit tricky but there are tests covering this.
>
> https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js
> File src/syntax/Parser.js (right):
>
>
https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
> src/syntax/Parser.js:3329: * We diverge from the harmony wiki by
> allowing arbitrary expressions inside
> On 2011/10/13 21:46:22, cburrows wrote:
>>
>> Did you do this to get the implementation up quickly or is the wiki
>
> wrong?
>
> The wiki has this as an open issue and Mike's prototype also supports
> expressions. I find that supporting expressions is very important and I
> will bring it up to the committee.
>
>
https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
> src/syntax/Parser.js:3368: var expression = this.parseExpression_();
> On 2011/10/13 21:46:22, cburrows wrote:
>>
>> I think I prefer the limited grammar. Nested quasis seem wrong. I want
>
> `` to
>>
>> behave string-like, which means inner ` should be quoted.
>
> With the limited grammar the following would not work.
>
> `$x + $y = ${x + y}`
>
> https://codereview.appspot.com/5244056/diff/6001/src/syntax/Scanner.js
> File src/syntax/Scanner.js (right):
>
>
https://codereview.appspot.com/5244056/diff/6001/src/syntax/Scanner.js#newcod...
> src/syntax/Scanner.js:498: }
> On 2011/10/13 21:46:22, cburrows wrote:
>>
>> Spec: No error otherwise? Should $ be escaped in a quasi?
>
> The spec allows the following:
>
> `a $ b`
>
> I'll make sure this is covered in a test.
>
> https://codereview.appspot.com/5244056/diff/6001/src/syntax/TokenType.js
> File src/syntax/TokenType.js (right):
>
>
https://codereview.appspot.com/5244056/diff/6001/src/syntax/TokenType.js#newc...
> src/syntax/TokenType.js:142: QUASI_TAG: 'quasi tag',
> On 2011/10/13 21:46:22, cburrows wrote:
>>
>> It seems weird to have a different token kind for this, why not just
>
> use an
>>
>> identifier token?
>
> See other comment
>
> https://codereview.appspot.com/5244056/
>

[cburrows, 2011-10-14 16:56:38]

On 2011/10/13 22:39:52, arv1 wrote:
> The quasi tag needs to be directly in front of the BACK_QUOTE and I need a way
> to differentiate the following two cases.
> 
> ident `abc`
> ident`abc`

I did not realize that the grammar regarded that whitespace as significant.
Sounds good!

https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
> src/codegeneration/QuasiLiteralTransformer.js:90: replace(/(^|[^\\])"/g,
> '$1\\"') +
> On 2011/10/13 21:46:22, cburrows wrote:
> > I think I must not understand what is happening here. You're escaping
quotes,
> > but leaving \" alone? What if you see \\"?
> 
> The value does not contain the wrapping quotes so we add that. We therefore
must
> replace all raw " characters with \". However, if the string contains \" we do
> not want to replace that with \\". This is a bit tricky but there are tests
> covering this.

But you don't know whether the \ that the regexp finds is an escape \ or an
actual \ that was previously escaped. I don't see a test that does something
like `"\\"`, which I have not run through your code but is the sort of thing I
think might be broken. 
https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
> src/syntax/Parser.js:3368: var expression = this.parseExpression_();
> On 2011/10/13 21:46:22, cburrows wrote:
> > I think I prefer the limited grammar. Nested quasis seem wrong. I want `` to
> > behave string-like, which means inner ` should be quoted.
> 
> With the limited grammar the following would not work.
> 
> `$x + $y = ${x + y}`

Well, there is a middle ground. You can permit some reasonable subset of
expressions. Without having participated in the discussion, I think nested
quasis are silly, as are probably some expressions that introduce local names or
statement blocks (like function expressions). You could totally do member, call,
arithmetic binops, etc, and that would be cool.

[Mike Samuel, 2011-10-14 17:59:32]

On Fri, Oct 14, 2011 at 12:56,  <cburrows@google.com> wrote:
> On 2011/10/13 22:39:52, arv1 wrote:
>>
>> The quasi tag needs to be directly in front of the BACK_QUOTE and I
>
> need a way
>>
>> to differentiate the following two cases.
>
>> ident `abc`
>> ident`abc`
>
> I did not realize that the grammar regarded that whitespace as
> significant. Sounds good!
>
>
https://codereview.appspot.com/5244056/diff/6001/src/codegeneration/QuasiLite...
>>
>> src/codegeneration/QuasiLiteralTransformer.js:90:
>
> replace(/(^|[^\\])"/g,
>>
>> '$1\\"') +
>> On 2011/10/13 21:46:22, cburrows wrote:
>> > I think I must not understand what is happening here. You're
>
> escaping quotes,
>>
>> > but leaving \" alone? What if you see \\"?
>
>> The value does not contain the wrapping quotes so we add that. We
>
> therefore must
>>
>> replace all raw " characters with \". However, if the string contains
>
> \" we do
>>
>> not want to replace that with \\". This is a bit tricky but there are
>
> tests
>>
>> covering this.
>
> But you don't know whether the \ that the regexp finds is an escape \ or
> an actual \ that was previously escaped. I don't see a test that does
> something like `"\\"`, which I have not run through your code but is the
> sort of thing I think might be broken.
>
https://codereview.appspot.com/5244056/diff/6001/src/syntax/Parser.js#newcode...
>>
>> src/syntax/Parser.js:3368: var expression = this.parseExpression_();
>> On 2011/10/13 21:46:22, cburrows wrote:
>> > I think I prefer the limited grammar. Nested quasis seem wrong. I
>
> want `` to
>>
>> > behave string-like, which means inner ` should be quoted.
>
>> With the limited grammar the following would not work.
>
>> `$x + $y = ${x + y}`
>
> Well, there is a middle ground. You can permit some reasonable subset of
> expressions. Without having participated in the discussion, I think
> nested quasis are silly, as are probably some expressions that introduce
> local names or statement blocks (like function expressions). You could
> totally do member, call, arithmetic binops, etc, and that would be cool.

It is actually much harder to spec quasi substitutions as arbitrary
expressions minus quasis than as arbitrary expressions.

[arv, 2011-10-14 18:55:15]

Chris, you were right that the regexp replace was broken.

I ended up rewriting the cooking regexp replaces with a single loop instead. It
might be less efficient but it is a lot clearer and also correct which is a
bonus ;-)

[cburrows, 2011-10-14 19:05:45]

On 2011/10/14 18:55:15, arv1 wrote:
> Chris, you were right that the regexp replace was broken.
> 
> I ended up rewriting the cooking regexp replaces with a single loop instead.
It
> might be less efficient but it is a lot clearer and also correct which is a
> bonus ;-)

LGTM