[parser] implement error reporting for Scanner errors

[parser] implement error reporting for Scanner

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org

Committed: https://crrev.com/e6f4b7491c434773f0a59a36ce98db40ea31bce2
Cr-Commit-Position: refs/heads/master@{#34966}

[caitp (gmail), 2016-03-12 23:02:04]

here's a quick frontend fix --- i'll add some tests on monday.

[caitp (gmail), 2016-03-14 20:24:05]

On 2016/03/12 23:02:04, caitp wrote:
> here's a quick frontend fix --- i'll add some tests on monday.

Added tests

[caitp (gmail), 2016-03-15 21:06:00]

so, do people like the clearer error messages, or should the fix be limited to
handling Token::ILLEGAL in ParseMemberExpressionContinuation?

I think the messages could make things easier for people

[adamk, 2016-03-16 17:45:31]

On 2016/03/15 21:06:00, caitp wrote:
> so, do people like the clearer error messages, or should the fix be limited to
> handling Token::ILLEGAL in ParseMemberExpressionContinuation?
> 
> I think the messages could make things easier for people

Sorry for the delayed review, there's been a lot of other stuff going on this
week.

I'd prefer at least splitting this patch in two, so they can be considered
separately. The template string fix should be a super-easy review, whereas some
of the position math and boolean template parameters in Scanner look a little
bit trickier to me.

[caitp (gmail), 2016-03-16 18:33:10]

On 2016/03/16 17:45:31, adamk wrote:
> On 2016/03/15 21:06:00, caitp wrote:
> > so, do people like the clearer error messages, or should the fix be limited
to
> > handling Token::ILLEGAL in ParseMemberExpressionContinuation?
> > 
> > I think the messages could make things easier for people
> 
> Sorry for the delayed review, there's been a lot of other stuff going on this
> week.
> 
> I'd prefer at least splitting this patch in two, so they can be considered
> separately. The template string fix should be a super-easy review, whereas
some
> of the position math and boolean template parameters in Scanner look a little
> bit trickier to me.

The member expression thing is split out into
https://codereview.chromium.org/1806063002

[caitp (gmail), 2016-03-16 20:28:56]

Description was changed from

==========
[parser] implement error reporting for Scanner, fix MemberExpr bug

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

Also fixes the unhandled Token::ILLEGAL case in
ParseMemberExpressionContinuation,
which resulted in tagged templates reporting strange errors in expression
contexts.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] implement error reporting for Scanner, fix MemberExpr bug

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

[caitp (gmail), 2016-03-16 20:29:04]

Description was changed from

==========
[parser] implement error reporting for Scanner, fix MemberExpr bug

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] implement error reporting for Scanner

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

[caitp (gmail), 2016-03-16 20:29:28]

Alright, rebased on top of the split off patch now

[adamk, 2016-03-18 18:21:55]

Finally got this back to the top of my inbox.

https://codereview.chromium.org/1793913002/diff/100001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1793913002/diff/100001/src/messages.h#newcode469
src/messages.h:469: "Unclosed Unicode escape sequence --- Expected `}`")        
              \
This one is weird in a couple ways:

- More traditional formatting would be "[...] sequence: expected '}'" (note
forward-single quotes, lower case, and colon instead of "---").
- In the example message test, the error doesn't actually seem all that helpful,
since there _is_ a '}', it's just too late. But maybe that's ok?

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/parser-bas...
src/parsing/parser-base.h:1015: if (token == Token::ILLEGAL &&
scanner()->has_error()) {
Can you put this logic inside GetUnexpectedToken instead? That'll require adding
a Scanner::Location* arg, but that seems fine.

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc
File src/parsing/scanner.cc (left):

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:87: return -1;
Losing this and the below early return makes me a little nervous. Why don't you
just want to return here and below and handle the error-reporting in the caller,
since you already have error-reporting there? Is it so that your reported
location is wider? That seems odd to me, given that the problem happens as soon
as you hit a non-hex character. Scanning after that is pretty meaningless.

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:86: uc32 Scanner::ScanUnlimitedLengthHexNumber(int
max_value, bool& bad_codepoint) {
The right type here is bool* in V8

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.h#...
src/parsing/scanner.h:495: void ReportScannerError(MessageTemplate::Template
error, int begin, int end) {
Please put the location before |error| (same below), which matches the Parser
methods. Also this one should just take a Location.

https://codereview.chromium.org/1793913002/diff/100001/test/message/unicode-e...
File test/message/unicode-escape-invalid.out (right):

https://codereview.chromium.org/1793913002/diff/100001/test/message/unicode-e...
test/message/unicode-escape-invalid.out:8: SyntaxError: Unclosed Unicode escape
sequence --- Expected `}`
As mentioned elsewhere, in this context just saying "Invalid unicode escape
sequence" or "Invalid hexadecimal escape sequence" seems fine, since the error
is the "Y", not the lack of a "}".

[caitp (gmail), 2016-03-18 19:00:36]

i probably wont have these fixed until monday, but sgtm

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc
File src/parsing/scanner.cc (left):

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:87: return -1;
On 2016/03/18 18:21:55, adamk wrote:
> Losing this and the below early return makes me a little nervous. Why don't
you
> just want to return here and below and handle the error-reporting in the
caller,
> since you already have error-reporting there? Is it so that your reported
> location is wider? That seems odd to me, given that the problem happens as
soon
> as you hit a non-hex character. Scanning after that is pretty meaningless.

the idea is, if you get hex characters until the }, you can say that its an
invalid codepoint, and if you fail to get a hex digit or closing brace, you can
report the unclosed error. I'd like to get the chromium devtools using the
location data to clarify the problem, and in this case it will point right to
the invalid codepoint.

its not worth much without location data being used, though. i can remove it
until after that change is made, I guess.

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:86: uc32 Scanner::ScanUnlimitedLengthHexNumber(int
max_value, bool& bad_codepoint) {
On 2016/03/18 18:21:55, adamk wrote:
> The right type here is bool* in V8

dont you think reference ls are better for preventing accidents here? anyway,
will do

https://codereview.chromium.org/1793913002/diff/100001/test/message/unicode-e...
File test/message/unicode-escape-invalid.out (right):

https://codereview.chromium.org/1793913002/diff/100001/test/message/unicode-e...
test/message/unicode-escape-invalid.out:8: SyntaxError: Unclosed Unicode escape
sequence --- Expected `}`
On 2016/03/18 18:21:55, adamk wrote:
> As mentioned elsewhere, in this context just saying "Invalid unicode escape
> sequence" or "Invalid hexadecimal escape sequence" seems fine, since the error
> is the "Y", not the lack of a "}".

Acknowledged.

[adamk, 2016-03-18 19:09:57]

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc
File src/parsing/scanner.cc (left):

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:87: return -1;
On 2016/03/18 19:00:36, caitp wrote:
> On 2016/03/18 18:21:55, adamk wrote:
> > Losing this and the below early return makes me a little nervous. Why don't
> you
> > just want to return here and below and handle the error-reporting in the
> caller,
> > since you already have error-reporting there? Is it so that your reported
> > location is wider? That seems odd to me, given that the problem happens as
> soon
> > as you hit a non-hex character. Scanning after that is pretty meaningless.
> 
> the idea is, if you get hex characters until the }, you can say that its an
> invalid codepoint, and if you fail to get a hex digit or closing brace, you
can
> report the unclosed error. I'd like to get the chromium devtools using the
> location data to clarify the problem, and in this case it will point right to
> the invalid codepoint.
> 
> its not worth much without location data being used, though. i can remove it
> until after that change is made, I guess.

Yeah, I'd just prefer a minimal change if the location data isn't being used;
the improved message seems like the main point.

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:86: uc32 Scanner::ScanUnlimitedLengthHexNumber(int
max_value, bool& bad_codepoint) {
On 2016/03/18 19:00:36, caitp wrote:
> On 2016/03/18 18:21:55, adamk wrote:
> > The right type here is bool* in V8
> 
> dont you think reference ls are better for preventing accidents here? anyway,
> will do

I think bool args are dangerous in C++ no matter what you do :)

As for reference vs pointer, V8 follows Google style and uses pointers for
outparams (Blink inherited WebKit style, which is the reason it uses
references).

[caitp (gmail), 2016-03-21 16:15:03]

okay @_@

https://codereview.chromium.org/1793913002/diff/100001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1793913002/diff/100001/src/messages.h#newcode469
src/messages.h:469: "Unclosed Unicode escape sequence --- Expected `}`")        
              \
On 2016/03/18 18:21:55, adamk wrote:
> This one is weird in a couple ways:
> 
> - More traditional formatting would be "[...] sequence: expected '}'" (note
> forward-single quotes, lower case, and colon instead of "---").

I rewrote the message to match similar-ish messages in ToT. Some of the Proxy
invariant check messages have the (expected ...) stuff in parentheses, lower
cased.

> - In the example message test, the error doesn't actually seem all that
helpful,
> since there _is_ a '}', it's just too late. But maybe that's ok?

Maybe once there's more of a reason to, it could march ahead up to a set number
of characters to look for a  `}`, and choose a better error message if it finds
one? But I'm not really sure it matters that much ---- Either way it's still
more information than "Unexpected or illegal token"

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/parser-bas...
src/parsing/parser-base.h:1015: if (token == Token::ILLEGAL &&
scanner()->has_error()) {
On 2016/03/18 18:21:55, adamk wrote:
> Can you put this logic inside GetUnexpectedToken instead? That'll require
adding
> a Scanner::Location* arg, but that seems fine.

Done.

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc
File src/parsing/scanner.cc (left):

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:87: return -1;
On 2016/03/18 19:09:57, adamk wrote:
> On 2016/03/18 19:00:36, caitp wrote:
> > On 2016/03/18 18:21:55, adamk wrote:
> > > Losing this and the below early return makes me a little nervous. Why
don't
> > you
> > > just want to return here and below and handle the error-reporting in the
> > caller,
> > > since you already have error-reporting there? Is it so that your reported
> > > location is wider? That seems odd to me, given that the problem happens as
> > soon
> > > as you hit a non-hex character. Scanning after that is pretty meaningless.
> > 
> > the idea is, if you get hex characters until the }, you can say that its an
> > invalid codepoint, and if you fail to get a hex digit or closing brace, you
> can
> > report the unclosed error. I'd like to get the chromium devtools using the
> > location data to clarify the problem, and in this case it will point right
to
> > the invalid codepoint.
> > 
> > its not worth much without location data being used, though. i can remove it
> > until after that change is made, I guess.
> 
> Yeah, I'd just prefer a minimal change if the location data isn't being used;
> the improved message seems like the main point.

Done.

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.cc...
src/parsing/scanner.cc:86: uc32 Scanner::ScanUnlimitedLengthHexNumber(int
max_value, bool& bad_codepoint) {
On 2016/03/18 19:09:57, adamk wrote:
> On 2016/03/18 19:00:36, caitp wrote:
> > On 2016/03/18 18:21:55, adamk wrote:
> > > The right type here is bool* in V8
> > 
> > dont you think reference ls are better for preventing accidents here?
anyway,
> > will do
> 
> I think bool args are dangerous in C++ no matter what you do :)
> 
> As for reference vs pointer, V8 follows Google style and uses pointers for
> outparams (Blink inherited WebKit style, which is the reason it uses
> references).

Since the early return was put back, there isn't much value in the parameter
anymore anyways, so it's gone

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

https://codereview.chromium.org/1793913002/diff/100001/src/parsing/scanner.h#...
src/parsing/scanner.h:495: void ReportScannerError(MessageTemplate::Template
error, int begin, int end) {
On 2016/03/18 18:21:55, adamk wrote:
> Please put the location before |error| (same below), which matches the Parser
> methods. Also this one should just take a Location.

Done.

https://codereview.chromium.org/1793913002/diff/100001/test/message/unicode-e...
File test/message/unicode-escape-invalid.out (right):

https://codereview.chromium.org/1793913002/diff/100001/test/message/unicode-e...
test/message/unicode-escape-invalid.out:8: SyntaxError: Unclosed Unicode escape
sequence --- Expected `}`
On 2016/03/18 19:00:36, caitp wrote:
> On 2016/03/18 18:21:55, adamk wrote:
> > As mentioned elsewhere, in this context just saying "Invalid unicode escape
> > sequence" or "Invalid hexadecimal escape sequence" seems fine, since the
error
> > is the "Y", not the lack of a "}".
> 
> Acknowledged.

well, if you think about it, the scanner is in a state where if the next input
isn't a hex digit, it must be a `}` --- as far as the state machine is
concerned, it really is an unclosed unicode escape sequence.

But alright, I've removed the message, maybe it will be helpful to put back
later on.

[caitp (gmail), 2016-03-21 16:15:05]

okay @_@

[adamk, 2016-03-21 19:13:56]

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

https://codereview.chromium.org/1793913002/diff/200001/src/parsing/scanner.cc...
src/parsing/scanner.cc:95: return -1;
Seems like this is the right place to report the going-over-the-max error, since
that's the constraint this method implements.

https://codereview.chromium.org/1793913002/diff/200001/src/parsing/scanner.cc...
src/parsing/scanner.cc:102: if (!nof_digits || (c0_ != '}')) {
I'd leave the closing brace check in the caller, since it has the opening brace
check.

And the InvalidUnicodeEscapeSequence can also live in the caller, once the "max
value" check is reported here. That'll make the code shape exactly the same as
it was before the addition of error messages.

[caitp (gmail), 2016-03-21 19:40:05]

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

https://codereview.chromium.org/1793913002/diff/200001/src/parsing/scanner.cc...
src/parsing/scanner.cc:95: return -1;
On 2016/03/21 19:13:56, adamk wrote:
> Seems like this is the right place to report the going-over-the-max error,
since
> that's the constraint this method implements.

Done.

https://codereview.chromium.org/1793913002/diff/200001/src/parsing/scanner.cc...
src/parsing/scanner.cc:102: if (!nof_digits || (c0_ != '}')) {
On 2016/03/21 19:13:56, adamk wrote:
> I'd leave the closing brace check in the caller, since it has the opening
brace
> check.
> 
> And the InvalidUnicodeEscapeSequence can also live in the caller, once the
"max
> value" check is reported here. That'll make the code shape exactly the same as
> it was before the addition of error messages.

Done.

[adamk, 2016-03-21 19:44:31]

https://codereview.chromium.org/1793913002/diff/220001/src/parsing/scanner.cc
File src/parsing/scanner.cc (left):

https://codereview.chromium.org/1793913002/diff/220001/src/parsing/scanner.cc...
src/parsing/scanner.cc:88: if (d < 0) {
I think you still need this early return...

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

https://codereview.chromium.org/1793913002/diff/220001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1120: if (cp < 0) {
...and then report an error here, right?

[caitp (gmail), 2016-03-21 19:51:23]

https://codereview.chromium.org/1793913002/diff/220001/src/parsing/scanner.cc
File src/parsing/scanner.cc (left):

https://codereview.chromium.org/1793913002/diff/220001/src/parsing/scanner.cc...
src/parsing/scanner.cc:88: if (d < 0) {
On 2016/03/21 19:44:31, adamk wrote:
> I think you still need this early return...

you're correct, `\u{}` parses successfully with this :(

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

https://codereview.chromium.org/1793913002/diff/220001/src/parsing/scanner.cc...
src/parsing/scanner.cc:1120: if (cp < 0) {
On 2016/03/21 19:44:31, adamk wrote:
> ...and then report an error here, right?

Done.

[adamk, 2016-03-21 19:54:55]

lgtm

[caitp (gmail), 2016-03-21 20:01:41]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-03-21 20:01:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1793913002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1793913002/240001

[commit-bot: I haz the power, 2016-03-21 20:27:07]

Description was changed from

==========
[parser] implement error reporting for Scanner

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] implement error reporting for Scanner

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

[commit-bot: I haz the power, 2016-03-21 20:27:08]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-21 20:27:54]

Description was changed from

==========
[parser] implement error reporting for Scanner

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org
==========

to

==========
[parser] implement error reporting for Scanner

Enables the Scanner to provide a better error message when errors occur
in escape sequences, numbers, strings, etc.

BUG=v8:4829, v8:3230
LOG=N
R=adamk@chromium.org, littledan@chromium.org

Committed: https://crrev.com/e6f4b7491c434773f0a59a36ce98db40ea31bce2
Cr-Commit-Position: refs/heads/master@{#34966}
==========

[commit-bot: I haz the power, 2016-03-21 20:27:55]

Patchset 13 (id:??) landed as
https://crrev.com/e6f4b7491c434773f0a59a36ce98db40ea31bce2
Cr-Commit-Position: refs/heads/master@{#34966}