Gracefully handle syntax errors in Parser::PatternRewriter.

Gracefully handle syntax errors in Parser::PatternRewriter.

PatternRewriter handles certain error cases with UNREACHABLE().
Apparently, RewriteDestructuringAssignment assumes syntax checking has
been done, so it's ok in this case. But DeclareAndInitializeVariables
is meant to report errors to the user, so that's rather inappropriate.
We leave the 1st case as a CHECK, but the 2nd will now report a syntax
error.

R=jochen@chromium.org, rossberg@chromium.org
BUG=chromium:582626
LOG=Y

[vogelheim, 2016-02-01 17:13:27]

@Andreas, do you have suggestion on what the proper error message should be?

Not quite ready yet, since I still need to add a unit test that a syntax error
is reported.

[adamk, 2016-02-01 19:43:37]

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

[adamk, 2016-02-01 19:43:38]

Can you CC me on the bug?

In general the design of the pattern-rewriter is that it's an internal error to
pass it an invalid pattern AST, so I'm a little concerned we're papering over
something with this patch.

[vogelheim, 2016-02-01 20:41:45]

On 2016/02/01 19:43:38, adamk wrote:
> Can you CC me on the bug?

Done.

> In general the design of the pattern-rewriter is that it's an internal error
to
> pass it an invalid pattern AST, so I'm a little concerned we're papering over
> something with this patch.

That's good feedback. I "guessed" the error handling strategy from the "bool*
ok" parameter, and it's quite possible I guessed wrong. Since we do end up in
the UNREACHABLE() cases for invalid ASTs... what would be the right place to
check for those errors?

[caitp (gmail), 2016-02-01 20:53:20]

On 2016/02/01 20:41:45, vogelheim wrote:
> On 2016/02/01 19:43:38, adamk wrote:
> > Can you CC me on the bug?
> 
> Done.
> 
> > In general the design of the pattern-rewriter is that it's an internal error
> to
> > pass it an invalid pattern AST, so I'm a little concerned we're papering
over
> > something with this patch.
> 
> That's good feedback. I "guessed" the error handling strategy from the "bool*
> ok" parameter, and it's quite possible I guessed wrong. Since we do end up in
> the UNREACHABLE() cases for invalid ASTs... what would be the right place to
> check for those errors?

Judging from the test, it looks like there are some RecordPatternError() calls
missing in ParseArrayLiteral and ParsePropertyDefinition

[caitp (gmail), 2016-02-01 21:01:49]

On 2016/02/01 20:53:20, caitp wrote:
> On 2016/02/01 20:41:45, vogelheim wrote:
> > On 2016/02/01 19:43:38, adamk wrote:
> > > Can you CC me on the bug?
> > 
> > Done.
> > 
> > > In general the design of the pattern-rewriter is that it's an internal
error
> > to
> > > pass it an invalid pattern AST, so I'm a little concerned we're papering
> over
> > > something with this patch.
> > 
> > That's good feedback. I "guessed" the error handling strategy from the
"bool*
> > ok" parameter, and it's quite possible I guessed wrong. Since we do end up
in
> > the UNREACHABLE() cases for invalid ASTs... what would be the right place to
> > check for those errors?
> 
> Judging from the test, it looks like there are some RecordPatternError() calls
> missing in ParseArrayLiteral and ParsePropertyDefinition

er, not in those functions, it's in ParsePrimaryExpression

[caitp (gmail), 2016-02-01 21:04:05]

On 2016/02/01 21:01:49, caitp wrote:
> On 2016/02/01 20:53:20, caitp wrote:
> > On 2016/02/01 20:41:45, vogelheim wrote:
> > > On 2016/02/01 19:43:38, adamk wrote:
> > > > Can you CC me on the bug?
> > > 
> > > Done.
> > > 
> > > > In general the design of the pattern-rewriter is that it's an internal
> error
> > > to
> > > > pass it an invalid pattern AST, so I'm a little concerned we're papering
> > over
> > > > something with this patch.
> > > 
> > > That's good feedback. I "guessed" the error handling strategy from the
> "bool*
> > > ok" parameter, and it's quite possible I guessed wrong. Since we do end up
> in
> > > the UNREACHABLE() cases for invalid ASTs... what would be the right place
to
> > > check for those errors?
> > 
> > Judging from the test, it looks like there are some RecordPatternError()
calls
> > missing in ParseArrayLiteral and ParsePropertyDefinition
> 
> er, not in those functions, it's in ParsePrimaryExpression

```
        classifier->RecordExpressionError(scanner()->location(),
                                          MessageTemplate::kUnexpectedToken,
                                          Token::String(Token::ELLIPSIS));
        classifier->RecordNonSimpleParameter();
        ExpressionT expr =
            this->ParseAssignmentExpression(true, classifier, CHECK_OK);
        // Missing CheckDestructuringElement() here
```

[vogelheim, 2016-02-02 09:33:31]

Description was changed from

==========
Gracefully handle syntax errors in Parser::PatternRewriter.

PatternRewriter handles certain error cases with UNREACHABLE().
Apparently, RewriteDestructuringAssignment assumes syntax checking has
been done, so it's ok in this case. But DeclareAndInitializeVariables
is meant to report errors to the user, so that's rather inappropriate.
We leave the 1st case as a CHECK, but the 2nd will now report a syntax
error.

R=jochen@chromium.org, rossberg@chromium.org
BUG=chromium:582626
LOG=Y
==========

to

==========
Gracefully handle syntax errors in Parser::PatternRewriter.

PatternRewriter handles certain error cases with UNREACHABLE().
Apparently, RewriteDestructuringAssignment assumes syntax checking has
been done, so it's ok in this case. But DeclareAndInitializeVariables
is meant to report errors to the user, so that's rather inappropriate.
We leave the 1st case as a CHECK, but the 2nd will now report a syntax
error.

R=jochen@chromium.org, rossberg@chromium.org
BUG=chromium:582626
LOG=Y
==========