Better error reporting for "return();"

Better error reporting for "return();"

R=rossberg@chromium.org
BUG=v8:4194
LOG=N

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

[wingo, 2015-06-19 09:59:09]

The CQ bit was checked by wingo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-19 09:59:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191303002/1

[commit-bot: I haz the power, 2015-06-19 09:59:17]

The author wingo@igalia.com has not signed Google Contributor License Agreement.
Please visit https://cla.developers.google.com to sign and manage CLA.

[commit-bot: I haz the power, 2015-06-19 10:00:44]

The author wingo@igalia.com has not signed Google Contributor License Agreement.
Please visit https://cla.developers.google.com to sign and manage CLA.

[wingo, 2015-06-19 12:28:35]

The CQ bit was checked by wingo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-19 12:28:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191303002/20001

[commit-bot: I haz the power, 2015-06-19 13:02:58]

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

[commit-bot: I haz the power, 2015-06-19 13:02:59]

Dry run: This issue passed the CQ dry run.

[rossberg, 2015-06-19 13:40:40]

https://codereview.chromium.org/1191303002/diff/20001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1191303002/diff/20001/src/messages.h#newcode327
src/messages.h:327: "Expected () to start arrow function, but got '%' instead of
'=>'")        \
Hm, isn't this also produced for (...x) and their ilk now? The message might
actually increase confusion in that case.

[wingo, 2015-06-19 13:45:33]

On 2015/06/19 13:40:40, rossberg wrote:
> https://codereview.chromium.org/1191303002/diff/20001/src/messages.h
> File src/messages.h (right):
> 
>
https://codereview.chromium.org/1191303002/diff/20001/src/messages.h#newcode327
> src/messages.h:327: "Expected () to start arrow function, but got '%' instead
of
> '=>'")        \
> Hm, isn't this also produced for (...x) and their ilk now? The message might
> actually increase confusion in that case.

Yes.  It's two cases: () and (...x), but no other ilk :)  Example:

d8> (...x);
(d8):1: SyntaxError: Expected () to start arrow function, but got ';' instead of
'=>'
(...x);
      ^
SyntaxError: Expected () to start arrow function, but got ';' instead of '=>'

It's not perfect but I don't find it terribly confusing, and fixing the (); case
seemed more important.  I can restrict it to the () case though.  Actually, I'll
do that, no problem.

[rossberg, 2015-06-19 13:55:49]

On 2015/06/19 13:45:33, wingo wrote:
> On 2015/06/19 13:40:40, rossberg wrote:
> > https://codereview.chromium.org/1191303002/diff/20001/src/messages.h
> > File src/messages.h (right):
> > 
> >
>
https://codereview.chromium.org/1191303002/diff/20001/src/messages.h#newcode327
> > src/messages.h:327: "Expected () to start arrow function, but got '%'
instead
> of
> > '=>'")        \
> > Hm, isn't this also produced for (...x) and their ilk now? The message might
> > actually increase confusion in that case.
> 
> Yes.  It's two cases: () and (...x), but no other ilk :)

Wouldn't it also include (...x, y) or (x, ...y) or any generalisation of those?

[rossberg, 2015-06-19 13:56:39]

On 2015/06/19 13:55:49, rossberg wrote:
> On 2015/06/19 13:45:33, wingo wrote:
> > On 2015/06/19 13:40:40, rossberg wrote:
> > > https://codereview.chromium.org/1191303002/diff/20001/src/messages.h
> > > File src/messages.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1191303002/diff/20001/src/messages.h#newcode327
> > > src/messages.h:327: "Expected () to start arrow function, but got '%'
> instead
> > of
> > > '=>'")        \
> > > Hm, isn't this also produced for (...x) and their ilk now? The message
might
> > > actually increase confusion in that case.
> > 
> > Yes.  It's two cases: () and (...x), but no other ilk :)
> 
> Wouldn't it also include (...x, y) or (x, ...y) or any generalisation of
those?

Point being that I'm not sure it's worth making () a special case. But I leave
that up to you.

[wingo, 2015-06-19 14:05:06]

On 2015/06/19 13:55:49, rossberg wrote:
> On 2015/06/19 13:45:33, wingo wrote:
> > On 2015/06/19 13:40:40, rossberg wrote:
> > > https://codereview.chromium.org/1191303002/diff/20001/src/messages.h
> > > File src/messages.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1191303002/diff/20001/src/messages.h#newcode327
> > > src/messages.h:327: "Expected () to start arrow function, but got '%'
> instead
> > of
> > > '=>'")        \
> > > Hm, isn't this also produced for (...x) and their ilk now? The message
might
> > > actually increase confusion in that case.
> > 
> > Yes.  It's two cases: () and (...x), but no other ilk :)
> 
> Wouldn't it also include (...x, y) or (x, ...y) or any generalisation of
those?

Not for (...x, y) because the (... case reads an identifier and then does:

        if (peek() == Token::COMMA) {
          ReportMessageAt(scanner()->peek_location(),
                          MessageTemplate::kParamAfterRest);
          *ok = false;
          return this->EmptyExpression();
        }

and then Expect()'s an ).

Not for (x, ...y) because that only passes through to ParseArrowFunctionLiteral
after it has seen the =>.

Anyway updated the patch for ();.  It's the one I think that JS developers will
run into in practice, and for which the error message is most confusing.  The
... gives you a visual hint that you've just read formal parameters and now will
be reading an arrow, whereas () is used often in other places (most notably
function calls), and getting "unexpected token: ;" in respose to "return();"
seems weird to me.  Maybe that's just me though :)  Your call, please let me
know.

[rossberg, 2015-06-19 14:08:38]

LGTM

> Not for (...x, y) because the (... case reads an identifier and then does:
[...]

> Not for (x, ...y) because that only passes through to
ParseArrowFunctionLiteral
> after it has seen the =>.

I see. What about about (...pattern) ? :)

[wingo, 2015-06-19 14:15:13]

On 2015/06/19 14:08:38, rossberg wrote:
> LGTM
> 
> > Not for (...x, y) because the (... case reads an identifier and then does:
> [...]
> 
> > Not for (x, ...y) because that only passes through to
> ParseArrowFunctionLiteral
> > after it has seen the =>.
> 
> I see. What about about (...pattern) ? :)

Hummm!  AFAIU parsing this is broken ATM, but FunctionRestParameter is
BindingRestElement which is "... BindingIdentifier", so really (... can only be
followed by an identifier.  However ParseFormalParameter does not appear to be
checking this, currently.  A bug, but not a bug which is shipping yet :)

[wingo, 2015-06-19 14:15:19]

The CQ bit was checked by wingo@igalia.com

[commit-bot: I haz the power, 2015-06-19 14:15:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191303002/40001

[wingo, 2015-06-19 14:16:49]

On 2015/06/19 14:15:13, wingo wrote:
> On 2015/06/19 14:08:38, rossberg wrote:
> > LGTM
> > 
> > > Not for (...x, y) because the (... case reads an identifier and then does:
> > [...]
> > 
> > > Not for (x, ...y) because that only passes through to
> > ParseArrowFunctionLiteral
> > > after it has seen the =>.
> > 
> > I see. What about about (...pattern) ? :)
> 
> Hummm!  AFAIU parsing this is broken ATM, but FunctionRestParameter is
> BindingRestElement which is "... BindingIdentifier", so really (... can only
be
> followed by an identifier.  However ParseFormalParameter does not appear to be
> checking this, currently.  A bug, but not a bug which is shipping yet :)

I should also say that (... can only start an arrow function formal parameter
list AFAIU, and that the initial ( already signalled a BindingPatternUnexpected
at the (.

[rossberg, 2015-06-19 14:26:48]

On 2015/06/19 14:15:13, wingo wrote:
> Hummm!  AFAIU parsing this is broken ATM, but FunctionRestParameter is
> BindingRestElement which is "... BindingIdentifier", so really (... can only
be
> followed by an identifier.

Interesting, I wasn't aware of this limitation! I had assumed that rest can be
destructured like everything else. Strange, even though I suppose it would not
be particularly useful without a more expressive pattern language.

[commit-bot: I haz the power, 2015-06-19 15:10:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-19 15:10:24]

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