[parser] Lift template literal invalid escape restriction

[parser] Lift template literal invalid escape restriction

This implements the proposal at
https://github.com/tc39/proposal-template-literal-revision
staged behind a flag --harmony-template-escapes. The proposal allows
invalid octal, unicode, and hexadecimal escape sequences to appear in
tagged template literals, instead of being a syntax error. These have
a 'cooked' value of 'undefined', but are still accessible through the
'raw' property.

BUG=v8:5546
Review-Url: https://codereview.chromium.org/2665513002
Cr-Commit-Position: refs/heads/master@{#43384}
Committed: https://chromium.googlesource.com/v8/v8/+/18e4c46de5c5269ed1a23219a9cefbd7d3a2a449

[bakkot1, 2017-01-27 23:22:20]

Description was changed from

==========
[parser] Lift template literal invalid escape restriction

This implements the proposal at
https://github.com/tc39/proposal-template-literal-revision
staged behind a flag --harmony-template-escapes. The proposal allows
invalid octal, unicode, and hexadecimal escape sequences to appear in
tagged template literals, instead of being a syntax error. These have
a 'cooked' value of 'undefined', but are still accessible through the
'raw' property.

BUG=5546
==========

to

==========
[parser] Lift template literal invalid escape restriction

This implements the proposal at
https://github.com/tc39/proposal-template-literal-revision
staged behind a flag --harmony-template-escapes. The proposal allows
invalid octal, unicode, and hexadecimal escape sequences to appear in
tagged template literals, instead of being a syntax error. These have
a 'cooked' value of 'undefined', but are still accessible through the
'raw' property.

BUG=v8:5546
==========

[bakkot1, 2017-01-27 23:22:20]

bakkot@gmail.com changed reviewers:
+ adamk@chromium.org

[bakkot1, 2017-01-27 23:25:25]

PTAL

[adamk, 2017-02-01 18:14:47]

Am at a bunch of conferences this week, hope to get to this soon (Monday at the
latest).

[adamk, 2017-02-07 20:55:13]

adamk@chromium.org changed reviewers:
+ vogelheim@chromium.org

[adamk, 2017-02-07 20:55:13]

Sorry I didn't punt this before, but I think vogelheim is going to be a better
reviewer for this. In particular, the Scanner::has_error()/clear_error() stuff
looks a little funny to me, I wonder if he has thoughts on a better API here.

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser.h#ne...
src/parsing/parser.h:589: void AddTemplateSpan(TemplateLiteralState* state, bool
should_cook,
Can you give this method a comment? Neither "should_cook" nor "tail" mean much
to someone not familiar with template strings.

[vogelheim, 2017-02-13 17:36:06]

Thanks for working on this! I'm looking forward to better spec coverage!


-----------------------------

I must admit I'm a bit unsure about the approach though:   (Granted, this is
largely subjective comment, so this is up for negotiation. Please feel free to
disagree with me on this...)

This takes responsibility for the error checking for template literal tokens
(and only for those) out of the Scanner, into the Parser. Particularly, there's
now the logic of the Scanner reporting an error, and then the layer above
deciding it might not have been an error after all. That seems like the kind of
thing that future me will trip over....


How about this:
FLAG_harmony_template_escapes is visible in the Scanner - well, it could be, if
we include flags.h, or alternatively set it as a flag in the Scanner object. How
about this alternative:

- In ScanTemplateSpan, we naturally 'know' the exact error conditions. Depending
on --harmony-template-escapes, this will either report an error (as it currently
does), or alternatively sets a new uncookable_template_span_ flag (or something)
in the TokenDesc.
- Just to be sure, we'll reset the LiteralBuffer for the 'cooked' value at the
end of ScanTemplateScan if that flag is set. (Or maybe alternatively DCHECK that
nobody tries to read the 'cooked' value if that flag is set.)
- That way, the scanner could largely stay the same, except AddTemplateSpan now
needs to use Undefined for the 'cooked' value, if the scanner reports
uncookable_template_span.

Anything I'm overlooking?
Does this make sense to you?

https://codereview.chromium.org/2665513002/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2665513002/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:4131: static const char* harmony_template_escapes_natives[]
= {nullptr};
Do we need this? Why? This feature doesn't seem to implement any natives.

--------------------

Background:
The original 'startup snapshot' could only be applied whole, and therefore the
snapshot couldn't contain any optional features. This initially worked fine for
--harmony-*, where the snapshot contained no 'harmony' features, and instead
they were added - if enabled - during initialization of the isolate, at some
additional runtime cost. When (most of) the --harmony-* features were turned on
by the default, this had the unfortunate effect that most of our users 'paid'
for the extra initialization. That's when the snapshot was extended to allow for
additional sections, one for each feature to be [en|dis]abled. This is the
mechanism you're extending here.

But... does this feature have any representation in the snapshot at all? I'd
think it's purely a parser issue.

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser-base...
src/parsing/parser-base.h:1230: template <bool tagged>
Why is this a template parameter, rather than a regular one?

----

Background:
We're having a bit of a problem with rather large code size, and templates tend
to 'bloat' the code further, since the compiler will typically compile multiple
instances of this. As you can see, the original developers of this code were
exceedingly fond of templates for performance reasons, but current thinking is
that maybe they've gone a little to far already. Which is why I want to know...
what are you're reasons for going this route?

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6848: RunParserSyncTest(context_data, data,
kSuccess, NULL, 0, flags,
If I read this correctly, this tests the behaviour with the new flag.

Obviously, we'd also want to test whether these changes might have disturbed the
current implementation, and make sure the current behaviour is unchanged.

[adamk, 2017-02-13 19:14:10]

https://codereview.chromium.org/2665513002/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2665513002/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:4131: static const char* harmony_template_escapes_natives[]
= {nullptr};
On 2017/02/13 17:36:06, vogelheim wrote:
> Do we need this? Why? This feature doesn't seem to implement any natives.

This is needed to let the macro just below work. Agreed that we can probably get
rid of this soon, as we phase out experimental natives, but for now this is just
a bit of boilerplate with negligible runtime overhead.

> --------------------
> 
> Background:
> The original 'startup snapshot' could only be applied whole, and therefore the
> snapshot couldn't contain any optional features. This initially worked fine
for
> --harmony-*, where the snapshot contained no 'harmony' features, and instead
> they were added - if enabled - during initialization of the isolate, at some
> additional runtime cost. When (most of) the --harmony-* features were turned
on
> by the default, this had the unfortunate effect that most of our users 'paid'
> for the extra initialization. That's when the snapshot was extended to allow
for
> additional sections, one for each feature to be [en|dis]abled. This is the
> mechanism you're extending here.
> 
> But... does this feature have any representation in the snapshot at all? I'd
> think it's purely a parser issue.

[bakkot1, 2017-02-13 20:22:03]

On 2017/02/13 17:36:06, vogelheim wrote:
> Thanks for working on this! I'm looking forward to better spec coverage!
> 
> 
> -----------------------------
> 
> I must admit I'm a bit unsure about the approach though:   (Granted, this is
> largely subjective comment, so this is up for negotiation. Please feel free to
> disagree with me on this...)
> 
> This takes responsibility for the error checking for template literal tokens
> (and only for those) out of the Scanner, into the Parser. Particularly,
there's
> now the logic of the Scanner reporting an error, and then the layer above
> deciding it might not have been an error after all. That seems like the kind
of
> thing that future me will trip over....
> 

My concern with the approach you outline is that it wouldn't give accurate error
messages and locations for invalid escape sequences in untagged templates.
Only the parsers knows whether the template is tagged, but only the scanner
knows the exact location of the invalid escape sequence.

We take a similar approach for octal literals in directives, which might precede
a "use strict" directive.

Would you be happier if we weren't using the scanner_error_ field? I've added a
new field with a clearer name (although it's strictly worse performance-wise);
what are your thoughts on that?

Alternatively we could just rename the scanner_error_ field to indicate more
precisely that it represents an invalid escape sequence, which is treated as
an error everywhere except in tagged templates.

[bakkot1, 2017-02-13 20:22:25]

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser-base...
src/parsing/parser-base.h:1230: template <bool tagged>
On 2017/02/13 17:36:06, vogelheim wrote:
> Why is this a template parameter, rather than a regular one?

I default to using template parameters for statically-known booleans for the
same reasons as the original authors, is all. I went ahead and converted it to a
regular parameter; in this case it probably wasn't even a performance win.

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2665513002/diff/60001/src/parsing/parser.h#ne...
src/parsing/parser.h:589: void AddTemplateSpan(TemplateLiteralState* state, bool
should_cook,
On 2017/02/07 20:55:13, adamk wrote:
> Can you give this method a comment? Neither "should_cook" nor "tail" mean much
> to someone not familiar with template strings.

Done.

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6848: RunParserSyncTest(context_data, data,
kSuccess, NULL, 0, flags,
On 2017/02/13 17:36:06, vogelheim wrote:
> If I read this correctly, this tests the behaviour with the new flag.
> 
> Obviously, we'd also want to test whether these changes might have disturbed
the
> current implementation, and make sure the current behaviour is unchanged.

I was figuring the existing tests would suffice for that, but I'm happy to add
more if you think it's warranted.

[vogelheim, 2017-02-17 16:30:50]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 16:30:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 16:32:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 16:32:30]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35050)

[vogelheim, 2017-02-17 16:33:53]

When I run this locally, I get a check failure in mjsunit/compiler/literals.
Please have a look (at least if that reproduces on the trybots.)

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6848: RunParserSyncTest(context_data, data,
kSuccess, NULL, 0, flags,
On 2017/02/13 20:22:24, bakkot1 wrote:
> On 2017/02/13 17:36:06, vogelheim wrote:
> > If I read this correctly, this tests the behaviour with the new flag.
> > 
> > Obviously, we'd also want to test whether these changes might have disturbed
> the
> > current implementation, and make sure the current behaviour is unchanged.
> 
> I was figuring the existing tests would suffice for that, but I'm happy to add
> more if you think it's warranted.

Well, I was thinking more about also running the new test cases when the new
flag is off, making sure they still through the same error as today.

https://codereview.chromium.org/2665513002/diff/80001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/80001/src/parsing/scanner.cc#...
src/parsing/scanner.cc:1070: ScanEscape<capture_raw, in_template_literal>();
In a template w/ two invalid escapes, this would record the position of the last
one, right?

[vogelheim, 2017-02-17 16:35:33]

On 2017/02/13 20:22:03, bakkot1 wrote:
> On 2017/02/13 17:36:06, vogelheim wrote:
> > Thanks for working on this! I'm looking forward to better spec coverage!
> > 
> > 
> > -----------------------------
> > 
> > I must admit I'm a bit unsure about the approach though:   (Granted, this is
> > largely subjective comment, so this is up for negotiation. Please feel free
to
> > disagree with me on this...)
> > 
> > This takes responsibility for the error checking for template literal tokens
> > (and only for those) out of the Scanner, into the Parser. Particularly,
> there's
> > now the logic of the Scanner reporting an error, and then the layer above
> > deciding it might not have been an error after all. That seems like the kind
> of
> > thing that future me will trip over....
> > 
> 
> My concern with the approach you outline is that it wouldn't give accurate
error
> messages and locations for invalid escape sequences in untagged templates.
> Only the parsers knows whether the template is tagged, but only the scanner
> knows the exact location of the invalid escape sequence.

Ah, I didn't see that. I guess I'd be happier if the spec was different. I think
you're right: The spec requires some interaction between scanner + parser. I
don't really see a way around that.

[bakkot1, 2017-02-17 20:42:18]

Addressed comments; see below. PTAL.

I also noticed and corrected a bug from master: we accept as a valid program

`\1`
"\1"

but it has an octal escape in a template, so it's not. (Octal escapes are not
handled very well right now: this arises because we only record the location of
the *last* octal escape we've seen, and we parse one token ahead before checking
for invalid octal literals, and an octal escape is legal in a string in sloppy
mode. So the second \1 causes us to forget about the first before we check it,
and the second is not an error.)

--

I'm not super happy with ScanTemplateSpan saving and restoring the error state
(in the latest patches), but I believe it's correct, at least.

> I guess I'd be happier if the spec was different.

The problem is that `\u{110000}` has no interpretation in a untagged template,
so it kind of has to be a syntax error.

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2665513002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6848: RunParserSyncTest(context_data, data,
kSuccess, NULL, 0, flags,
On 2017/02/17 16:33:53, vogelheim wrote:
> On 2017/02/13 20:22:24, bakkot1 wrote:
> > On 2017/02/13 17:36:06, vogelheim wrote:
> > > If I read this correctly, this tests the behaviour with the new flag.
> > > 
> > > Obviously, we'd also want to test whether these changes might have
disturbed
> > the
> > > current implementation, and make sure the current behaviour is unchanged.
> > 
> > I was figuring the existing tests would suffice for that, but I'm happy to
add
> > more if you think it's warranted.
> 
> Well, I was thinking more about also running the new test cases when the new
> flag is off, making sure they still through the same error as today.

Done.

https://codereview.chromium.org/2665513002/diff/80001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/80001/src/parsing/scanner.cc#...
src/parsing/scanner.cc:1070: ScanEscape<capture_raw, in_template_literal>();
On 2017/02/17 16:33:53, vogelheim wrote:
> In a template w/ two invalid escapes, this would record the position of the
last
> one, right?

It would. Fixed.

Octal literals actually have the same issue currently: we report the location of
the *last* octal literal in a string or template literal. With this patch, we
report the last in a string the first in a template literal.

Actually, that exposes a bug, which I'll document elsewhere.

[bakkot1, 2017-02-17 21:05:59]

Oh, I forgot to mention: the DCHECK was an actual failure; I hadn't accounted
for the scanner scanning another token after an illegal one. (Which it does
because the error is reported by the parser, which happens after the scanner
reads the lookahead token.)

I removed the check and replaced it with logic to deal with an "invalid
hex/unicode escape" error already being stored in the scanner state. This
probably isn't necessary in general, since we should never read the token
*after* Token::ILLEGAL, but on the other hand it applies nicely to the "invalid
octal" case as well.

[vogelheim, 2017-02-20 11:23:23]

Almost there, only a few mostly mechanical nitpicks left... :)

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/parser-bas...
src/parsing/parser-base.h:884: return true;
style nitpick:

This code looks more complicated than it actually is. How about something like:

if (!scanner()->...)
  return true;

// Handle error case(s):
scanner()->clear_invalid_template_escape();
if (dont_throw)
  return false;

impl()->Report...
*ok = false;
return true;

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/parser-bas...
src/parsing/parser-base.h:4437: bool should_cook = true;
naming/style nitpick: If I read this correctly, 'should_cook' is really a local
variable (that is, every write feeds directly into the subsequent
AddTemplateSpan call, and not anywhere else) that determines whether the
previous CheckTemplateEscapes was successful. Maybe:

- declare it locally when needed
- name it escape_is_valid, or something.
  (Note that the term 'cook' isn't really used elsewhere in this method.)

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1041: scanner_error_location_ = Location::invalid();
This logic (here & below) is a bit obtuse. I don't see much away around the
logic itself, but maybe we can reduce the boilerplate a bit. Maybe:

Have a (local) helper class MessageSaveScope that
- saved the message template + location in the constructor
- reconstructs it in the destructor,
- has a bool has_message() const; and a void Reset(); method.

You'd then have two instances of this, for scanner_error + .._location and
octal_message_ + ..._pos_. That should reduce the boilerplate a bit.

[Please decide as you see fit.]

https://codereview.chromium.org/2665513002/diff/150001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2665513002/diff/150001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7172: // still an error without flag
super nitpick: Pls capitalize 'Still'. :-)

https://codereview.chromium.org/2665513002/diff/150001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7173: RunParserSyncTest(context_data, data, kError);
Thanks!

[vogelheim, 2017-02-20 11:30:19]

On 2017/02/17 20:42:18, bakkot1 wrote:
> > I guess I'd be happier if the spec was different.
> 
> The problem is that `\u{110000}` has no interpretation in a untagged template,
> so it kind of has to be a syntax error.

Yes, I understand that. But I'd wish that tagged + untagged templates would have
the same syntax rules, e.g. by giving both of them a '.raw' property w/
identical rules, so that this (otherwise completely unnecessary)
inter-dependence between scanner + parser wasn't necessary. The idea that
validity of a token depends on the current state of the parser grammar is...
unfortunate.

[vogelheim, 2017-02-20 11:31:21]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 11:31:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 11:32:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 11:32:33]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/35137)

[bakkot1, 2017-02-21 21:54:15]

Addressed more comments, PTAL.

> The idea that validity of a token depends on the current state of the parser
grammar is... unfortunate.

Yeah. I don't know that it's any worse than forbidding octal literals in strings
in strict mode, though. In both cases you can choose to look at it as the token
always being valid, and the parser rejecting that token as not matching the
grammar. At least, that's how I think of it; it makes me feel better.

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/parser-bas...
src/parsing/parser-base.h:884: return true;
On 2017/02/20 11:23:20, vogelheim wrote:
> style nitpick:
> 
> This code looks more complicated than it actually is. How about something
like:
> 
> if (!scanner()->...)
>   return true;
> 
> // Handle error case(s):
> scanner()->clear_invalid_template_escape();
> if (dont_throw)
>   return false;
> 
> impl()->Report...
> *ok = false;
> return true;

Done. Or, well, not quite that, because you can't clear the error state before
reporting it, but something similar.

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1041: scanner_error_location_ = Location::invalid();
On 2017/02/20 11:23:20, vogelheim wrote:
> This logic (here & below) is a bit obtuse. I don't see much away around the
> logic itself, but maybe we can reduce the boilerplate a bit. Maybe:
> 
> Have a (local) helper class MessageSaveScope that
> - saved the message template + location in the constructor
> - reconstructs it in the destructor,
> - has a bool has_message() const; and a void Reset(); method.
> 
> You'd then have two instances of this, for scanner_error + .._location and
> octal_message_ + ..._pos_. That should reduce the boilerplate a bit.
> 
> [Please decide as you see fit.]

Added a ErrorState scoped helper which does this. Instead of has_message
and Reset I just have MoveErrorTo, which is what's actually used.

IIRC scoped helpers - at least the BlockState helper - don't usually expose much
of an interface, so this approach seemed more in line with that. Also less code
duplication.

I was tempted to have the constructor take the destination as a parameter too,
and have the destructor do the copy to the destination, but I'm not sure if
that's good style. Also, it would end up reporting the last invalid escape in
the template, instead of the first. Thoughts?

https://codereview.chromium.org/2665513002/diff/150001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/2665513002/diff/150001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:7172: // still an error without flag
On 2017/02/20 11:23:20, vogelheim wrote:
> super nitpick: Pls capitalize 'Still'. :-)

Done.

[vogelheim, 2017-02-22 09:39:05]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 09:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vogelheim, 2017-02-22 10:03:02]

lgtm

lgtm. Thanks for implementing this, and thanks for working through the review
comments. :-)

(Two more nitpicks; but they aren't particularly relevant. Also, feel free to
submit when done, so hopefully we won't require another rebase... )

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/150001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1041: scanner_error_location_ = Location::invalid();
On 2017/02/21 21:54:15, bakkot1 wrote:
> On 2017/02/20 11:23:20, vogelheim wrote:
> > This logic (here & below) is a bit obtuse. I don't see much away around the
> > logic itself, but maybe we can reduce the boilerplate a bit. Maybe:
> > 
> > Have a (local) helper class MessageSaveScope that
> > - saved the message template + location in the constructor
> > - reconstructs it in the destructor,
> > - has a bool has_message() const; and a void Reset(); method.
> > 
> > You'd then have two instances of this, for scanner_error + .._location and
> > octal_message_ + ..._pos_. That should reduce the boilerplate a bit.
> > 
> > [Please decide as you see fit.]
> 
> Added a ErrorState scoped helper which does this. Instead of has_message
> and Reset I just have MoveErrorTo, which is what's actually used.

Thanks.

> IIRC scoped helpers - at least the BlockState helper - don't usually expose
much
> of an interface, so this approach seemed more in line with that. Also less
code
> duplication.
> 
> I was tempted to have the constructor take the destination as a parameter too,
> and have the destructor do the copy to the destination, but I'm not sure if
> that's good style. Also, it would end up reporting the last invalid escape in
> the template, instead of the first. Thoughts?

I think both are fine. I think I like it a little better the way you've now done
it. I'll be happy to let you pick, in case you prefer one or the other.

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1075: ScanEscape<capture_raw, in_template_literal>();
Maybe: 
  bool success = ScanEscape<...>
  DCHECK_EQ(!success, has_error());

(Not sure this adds much... I was briefly irritated why we ignore the return
value. It's correct, of course, because we expect the return value to signal the
same as the error state. If so, it might make sense to DCHECK this.)

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.h#...
src/parsing/scanner.h:368: class ErrorState {
One more nitpick: I'd prefer if this moved to scanner.cc.


Reason: The current placement is arguably more consistent with what we currently
do; it's just that our current header structure is a bit of a mess. The
LiteralBuffer + LiteralScope etc. helpers are used as class members, and are
used from a whole lot of inline code, which means one can't remove them from the
header without a significant refactoring. But the ErrorState is - at least for
now - only used within a method, and not from inlined cpde, so it doesn't have
to be in the header.

(I'm convinced we can throw out *most* of the inlining, and with that we could
also slim down the header significantly. But figuring out which bits of inlined
code are actually useful is going to be quite a bit of work, unfortunately.)

[commit-bot: I haz the power, 2017-02-22 10:04:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 10:04:52]

Dry run: This issue passed the CQ dry run.

[bakkot1, 2017-02-22 20:23:53]

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1075: ScanEscape<capture_raw, in_template_literal>();
On 2017/02/22 10:03:02, vogelheim wrote:
> Maybe: 
>   bool success = ScanEscape<...>
>   DCHECK_EQ(!success, has_error());
> 
> (Not sure this adds much... I was briefly irritated why we ignore the return
> value. It's correct, of course, because we expect the return value to signal
the
> same as the error state. If so, it might make sense to DCHECK this.)

Seems worthwhile; done.

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.h
File src/parsing/scanner.h (right):

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.h#...
src/parsing/scanner.h:368: class ErrorState {
On 2017/02/22 10:03:02, vogelheim wrote:
> One more nitpick: I'd prefer if this moved to scanner.cc.
> 
> 
> Reason: The current placement is arguably more consistent with what we
currently
> do; it's just that our current header structure is a bit of a mess. The
> LiteralBuffer + LiteralScope etc. helpers are used as class members, and are
> used from a whole lot of inline code, which means one can't remove them from
the
> header without a significant refactoring. But the ErrorState is - at least for
> now - only used within a method, and not from inlined cpde, so it doesn't have
> to be in the header.
> 
> (I'm convinced we can throw out *most* of the inlining, and with that we could
> also slim down the header significantly. But figuring out which bits of
inlined
> code are actually useful is going to be quite a bit of work, unfortunately.)
> 

Sounds good to me! I'm always happier having less stuff in the headers; I only
put it there for consistency with similar classes. Done.

[bakkot1, 2017-02-22 20:28:25]

The CQ bit was checked by bakkot@gmail.com

[bakkot1, 2017-02-22 20:28:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2665513002/#ps230001
(title: "address comments")

[commit-bot: I haz the power, 2017-02-22 20:28:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bakkot1, 2017-02-22 20:41:14]

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/210001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1075: ScanEscape<capture_raw, in_template_literal>();
On 2017/02/22 20:23:53, bakkot1 wrote:
> On 2017/02/22 10:03:02, vogelheim wrote:
> > Maybe: 
> >   bool success = ScanEscape<...>
> >   DCHECK_EQ(!success, has_error());
> > 
> > (Not sure this adds much... I was briefly irritated why we ignore the return
> > value. It's correct, of course, because we expect the return value to signal
> the
> > same as the error state. If so, it might make sense to DCHECK this.)
> 
> Seems worthwhile; done.

Update: undone, since having `success` not be used in release causes a
compile failure. Going to leave it out instead of wrapping the
`bool success = ` in an IFDEF or making the check unconditional.

[bakkot1, 2017-02-22 20:41:20]

The CQ bit was checked by bakkot@gmail.com

[bakkot1, 2017-02-22 20:41:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2665513002/#ps270001
(title: "remove DCHECK_EQ to avoid compile errors in release builds")

[commit-bot: I haz the power, 2017-02-22 20:41:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2017-02-22 20:45:36]

https://codereview.chromium.org/2665513002/diff/250001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/250001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1116: DCHECK_EQ(!success, has_error());
You could add

USE(success);

to avoid the failure in non-DCHECK builds.

[bakkot1, 2017-02-22 20:53:44]

The CQ bit was unchecked by bakkot@gmail.com

[bakkot1, 2017-02-22 20:56:44]

https://codereview.chromium.org/2665513002/diff/250001/src/parsing/scanner.cc
File src/parsing/scanner.cc (right):

https://codereview.chromium.org/2665513002/diff/250001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1116: DCHECK_EQ(!success, has_error());
On 2017/02/22 20:45:36, adamk wrote:
> You could add
> 
> USE(success);
> 
> to avoid the failure in non-DCHECK builds.

Ah, thanks. Once more with feeling...

[bakkot1, 2017-02-22 20:57:11]

The CQ bit was checked by bakkot@gmail.com

[bakkot1, 2017-02-22 20:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2665513002/#ps290001
(title: "reintroduce DCHECK_EQ")

[commit-bot: I haz the power, 2017-02-22 20:57:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 21:20:33]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1487797031522740,
"parent_rev": "6a09a41622d65d07fcb4436ddfaaaa6a9cad2467", "commit_rev":
"18e4c46de5c5269ed1a23219a9cefbd7d3a2a449"}

[commit-bot: I haz the power, 2017-02-22 21:20:39]

Description was changed from

==========
[parser] Lift template literal invalid escape restriction

This implements the proposal at
https://github.com/tc39/proposal-template-literal-revision
staged behind a flag --harmony-template-escapes. The proposal allows
invalid octal, unicode, and hexadecimal escape sequences to appear in
tagged template literals, instead of being a syntax error. These have
a 'cooked' value of 'undefined', but are still accessible through the
'raw' property.

BUG=v8:5546
==========

to

==========
[parser] Lift template literal invalid escape restriction

This implements the proposal at
https://github.com/tc39/proposal-template-literal-revision
staged behind a flag --harmony-template-escapes. The proposal allows
invalid octal, unicode, and hexadecimal escape sequences to appear in
tagged template literals, instead of being a syntax error. These have
a 'cooked' value of 'undefined', but are still accessible through the
'raw' property.

BUG=v8:5546

Review-Url: https://codereview.chromium.org/2665513002
Cr-Commit-Position: refs/heads/master@{#43384}
Committed:
https://chromium.googlesource.com/v8/v8/+/18e4c46de5c5269ed1a23219a9cefbd7d3a...
==========

[commit-bot: I haz the power, 2017-02-22 21:20:40]

Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/v8/v8/+/18e4c46de5c5269ed1a23219a9cefbd7d3a...