Remove AssignmentExpressionFlags enum, handle error checking in callers

Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.
CheckDestructuringElement has been further simplified to only emit the
errors that the parser depends on it emitting.

I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).

Committed: https://crrev.com/1003785ced44df0dc775b54cd95552784916b1e8
Cr-Commit-Position: refs/heads/master@{#33961}

[adamk, 2016-02-12 01:20:13]

Description was changed from

==========
Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.

In doing so, I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).
==========

to

==========
Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.
CheckDestructuringElement has been further simplified to only emit the
errors that the parser depends on it emitting.

I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).
==========

[adamk, 2016-02-12 01:21:42]

adamk@chromium.org changed reviewers:
+ caitpotter88@gmail.com, rossberg@chromium.org

[adamk, 2016-02-12 01:21:42]

This one isn't quite as obvious a cleanup as the previous ones, but I had it
sitting around from that other batch of cleanups, and I think it's always nice
to have one less way to call ParseAssignmentExpression().

Let me know what y'all think.

[caitp (gmail), 2016-02-12 01:30:35]

On 2016/02/12 01:21:42, adamk wrote:
> This one isn't quite as obvious a cleanup as the previous ones, but I had it
> sitting around from that other batch of cleanups, and I think it's always nice
> to have one less way to call ParseAssignmentExpression().
> 
> Let me know what y'all think.

On mobile so it's hard to comment, but:

Having the caller deal with it seems okay

Some of the cleanup works sort of by accident, particularly removing the binding
pattern stuff in CheckDestructuringElement --- this is because it gets an error
cause of the numerous unexpected token reports that happen elsewhere. I think
the deleted code here would be better than the unexpected token stuff, and would
help authors figure out their problems better

[rossberg, 2016-02-12 08:32:36]

lgtm

LGTM, nice!

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

https://codereview.chromium.org/1696603002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:835: bool IsAssignmentExpression(ExpressionT
expression) {
Note: Niko has a patch in the making that, once landed, should make this method
unnececssary.

https://codereview.chromium.org/1696603002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:3287: return;
Nit: instead of early return here, can we invert the condition and record the
error here?

[adamk, 2016-02-12 18:21:12]

On 2016/02/12 01:30:35, caitp wrote:
> On 2016/02/12 01:21:42, adamk wrote:
> > This one isn't quite as obvious a cleanup as the previous ones, but I had it
> > sitting around from that other batch of cleanups, and I think it's always
nice
> > to have one less way to call ParseAssignmentExpression().
> > 
> > Let me know what y'all think.
> 
> On mobile so it's hard to comment, but:
> 
> Having the caller deal with it seems okay
> 
> Some of the cleanup works sort of by accident, particularly removing the
binding
> pattern stuff in CheckDestructuringElement --- this is because it gets an
error
> cause of the numerous unexpected token reports that happen elsewhere. I think
> the deleted code here would be better than the unexpected token stuff, and
would
> help authors figure out their problems better

Can you give an example of something that has a different error message after
this patch? The ExpressionClassifier only keeps track of the first error it
encounters of each type, so it seems like "the numerous unexpected token
reports" are in fact what makes most cases an error right now anyway, and that
this code might be dead. But I could well be missing something.

[caitp (gmail), 2016-02-12 18:29:35]

On 2016/02/12 18:21:12, adamk wrote:
> On 2016/02/12 01:30:35, caitp wrote:
> > On 2016/02/12 01:21:42, adamk wrote:
> > > This one isn't quite as obvious a cleanup as the previous ones, but I had
it
> > > sitting around from that other batch of cleanups, and I think it's always
> nice
> > > to have one less way to call ParseAssignmentExpression().
> > > 
> > > Let me know what y'all think.
> > 
> > On mobile so it's hard to comment, but:
> > 
> > Having the caller deal with it seems okay
> > 
> > Some of the cleanup works sort of by accident, particularly removing the
> binding
> > pattern stuff in CheckDestructuringElement --- this is because it gets an
> error
> > cause of the numerous unexpected token reports that happen elsewhere. I
think
> > the deleted code here would be better than the unexpected token stuff, and
> would
> > help authors figure out their problems better
> 
> Can you give an example of something that has a different error message after
> this patch? The ExpressionClassifier only keeps track of the first error it
> encounters of each type, so it seems like "the numerous unexpected token
> reports" are in fact what makes most cases an error right now anyway, and that
> this code might be dead. But I could well be missing something.

They don't have different messages, but the intent was to remove numerous
"unexpected token" messages, and replace them with the slightly clearer ones in
CheckDestructuringElement.

This means shrinking the workload a bit, and improving the comprehension of the
error message, which is nice for developers. My preference would be to keep the
CheckDestructuringElement bits, and remove the unneeded "unexpected token"
reports. Can always do that later, so your call

[adamk, 2016-02-12 18:30:39]

On 2016/02/12 18:29:35, caitp wrote:
> On 2016/02/12 18:21:12, adamk wrote:
> > On 2016/02/12 01:30:35, caitp wrote:
> > > On 2016/02/12 01:21:42, adamk wrote:
> > > > This one isn't quite as obvious a cleanup as the previous ones, but I
had
> it
> > > > sitting around from that other batch of cleanups, and I think it's
always
> > nice
> > > > to have one less way to call ParseAssignmentExpression().
> > > > 
> > > > Let me know what y'all think.
> > > 
> > > On mobile so it's hard to comment, but:
> > > 
> > > Having the caller deal with it seems okay
> > > 
> > > Some of the cleanup works sort of by accident, particularly removing the
> > binding
> > > pattern stuff in CheckDestructuringElement --- this is because it gets an
> > error
> > > cause of the numerous unexpected token reports that happen elsewhere. I
> think
> > > the deleted code here would be better than the unexpected token stuff, and
> > would
> > > help authors figure out their problems better
> > 
> > Can you give an example of something that has a different error message
after
> > this patch? The ExpressionClassifier only keeps track of the first error it
> > encounters of each type, so it seems like "the numerous unexpected token
> > reports" are in fact what makes most cases an error right now anyway, and
that
> > this code might be dead. But I could well be missing something.
> 
> They don't have different messages, but the intent was to remove numerous
> "unexpected token" messages, and replace them with the slightly clearer ones
in
> CheckDestructuringElement.
> 
> This means shrinking the workload a bit, and improving the comprehension of
the
> error message, which is nice for developers. My preference would be to keep
the
> CheckDestructuringElement bits, and remove the unneeded "unexpected token"
> reports. Can always do that later, so your call

Ah, I see. Is that a project that you started on at some point?

[caitp (gmail), 2016-02-12 18:34:03]

On 2016/02/12 18:30:39, adamk wrote:
> On 2016/02/12 18:29:35, caitp wrote:
> > On 2016/02/12 18:21:12, adamk wrote:
> > > On 2016/02/12 01:30:35, caitp wrote:
> > > > On 2016/02/12 01:21:42, adamk wrote:
> > > > > This one isn't quite as obvious a cleanup as the previous ones, but I
> had
> > it
> > > > > sitting around from that other batch of cleanups, and I think it's
> always
> > > nice
> > > > > to have one less way to call ParseAssignmentExpression().
> > > > > 
> > > > > Let me know what y'all think.
> > > > 
> > > > On mobile so it's hard to comment, but:
> > > > 
> > > > Having the caller deal with it seems okay
> > > > 
> > > > Some of the cleanup works sort of by accident, particularly removing the
> > > binding
> > > > pattern stuff in CheckDestructuringElement --- this is because it gets
an
> > > error
> > > > cause of the numerous unexpected token reports that happen elsewhere. I
> > think
> > > > the deleted code here would be better than the unexpected token stuff,
and
> > > would
> > > > help authors figure out their problems better
> > > 
> > > Can you give an example of something that has a different error message
> after
> > > this patch? The ExpressionClassifier only keeps track of the first error
it
> > > encounters of each type, so it seems like "the numerous unexpected token
> > > reports" are in fact what makes most cases an error right now anyway, and
> that
> > > this code might be dead. But I could well be missing something.
> > 
> > They don't have different messages, but the intent was to remove numerous
> > "unexpected token" messages, and replace them with the slightly clearer ones
> in
> > CheckDestructuringElement.
> > 
> > This means shrinking the workload a bit, and improving the comprehension of
> the
> > error message, which is nice for developers. My preference would be to keep
> the
> > CheckDestructuringElement bits, and remove the unneeded "unexpected token"
> > reports. Can always do that later, so your call
> 
> Ah, I see. Is that a project that you started on at some point?

Ive been planning to, but haven't started it yet

[adamk, 2016-02-12 21:49:57]

I'd like to carry on with removing this for now, since that code in
CheckDestructuringElement is currently dead. If a future refactor can get rid of
all the BindingPatternUnexpectedToken() calls, it'll be easy enough to resurrect
the bits of this that are needed.

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

https://codereview.chromium.org/1696603002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:835: bool IsAssignmentExpression(ExpressionT
expression) {
On 2016/02/12 08:32:36, rossberg wrote:
> Note: Niko has a patch in the making that, once landed, should make this
method
> unnececssary.

Sounds good, feel free to CC me on that one.

https://codereview.chromium.org/1696603002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:3287: return;
On 2016/02/12 08:32:36, rossberg wrote:
> Nit: instead of early return here, can we invert the condition and record the
> error here?

Done.

[adamk, 2016-02-12 21:50:00]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-02-12 21:50:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1696603002/#ps20001
(title: "Eliminate early return")

[commit-bot: I haz the power, 2016-02-12 21:50:04]

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

[commit-bot: I haz the power, 2016-02-12 22:38:23]

Description was changed from

==========
Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.
CheckDestructuringElement has been further simplified to only emit the
errors that the parser depends on it emitting.

I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).
==========

to

==========
Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.
CheckDestructuringElement has been further simplified to only emit the
errors that the parser depends on it emitting.

I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).
==========

[commit-bot: I haz the power, 2016-02-12 22:38:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-12 22:38:52]

Description was changed from

==========
Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.
CheckDestructuringElement has been further simplified to only emit the
errors that the parser depends on it emitting.

I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).
==========

to

==========
Remove AssignmentExpressionFlags enum, handle error checking in callers

This is hopefully the last in a series of cleanup patches around
destructuring assignment. It simplifies the ParseAssignmentExpression
API, making the callers call CheckDestructuringElement() where appropriate.
CheckDestructuringElement has been further simplified to only emit the
errors that the parser depends on it emitting.

I've also beefed up the test coverage in test-parsing.cc to
handling all the destructuring flags being on, which caught an oddity
in how we disallow initializers in spreads in patterns (we need to treat
RewritableAssignmentExpressions as Assignments for the purpose of
error checking).

Finally, I added a few helper methods to ParserBase to handle a few
classes of expressions (assignments and literals-as-patterns).

Committed: https://crrev.com/1003785ced44df0dc775b54cd95552784916b1e8
Cr-Commit-Position: refs/heads/master@{#33961}
==========

[commit-bot: I haz the power, 2016-02-12 22:38:53]

Patchset 2 (id:??) landed as
https://crrev.com/1003785ced44df0dc775b54cd95552784916b1e8
Cr-Commit-Position: refs/heads/master@{#33961}