[parser] improve inferred function names for async arrow functions

[parser] improve inferred function names for async arrow functions

No longer include the "async" keyword, or an async arrow function's single
identifier parameter as part of its inferred name.

BUG=v8:5281, v8:4483
R=adamk@chromium.org, littledan@chromium.org, marja@chromium.org

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

[caitp, 2016-08-11 22:55:48]

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

[commit-bot: I haz the power, 2016-08-11 22:55:51]

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

[caitp, 2016-08-11 23:09:21]

Hi, PTAL.

I believe there's probably a slightly cleaner way to do this, as ordinary arrow
functions don't seem to require this. But it works.

[caitp, 2016-08-11 23:18:15]

On 2016/08/11 23:09:21, caitp wrote:
> Hi, PTAL.
> 
> I believe there's probably a slightly cleaner way to do this, as ordinary
arrow
> functions don't seem to require this. But it works.

I guess ordinary arrow functions do report the first parameter as an inferred
name, when they aren't anonymous, actually.

[commit-bot: I haz the power, 2016-08-11 23:19:23]

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

[commit-bot: I haz the power, 2016-08-11 23:19:24]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-08-12 00:39:21]

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

https://codereview.chromium.org/2235423003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2333: fni_->RemoveLastName();
I'm surprised that you have to remove two names here, while the following arrow
function code does not seem to have to remove any. How does this work?

https://codereview.chromium.org/2235423003/diff/1/test/mjsunit/harmony/async-...
File test/mjsunit/harmony/async-function-stacktrace.js (right):

https://codereview.chromium.org/2235423003/diff/1/test/mjsunit/harmony/async-...
test/mjsunit/harmony/async-function-stacktrace.js:109: }, ["test"]);
Maybe add a test to show that async functions with multiple named arguments are
also inferred appropriately?

[caitp, 2016-08-12 01:12:27]

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

https://codereview.chromium.org/2235423003/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2333: fni_->RemoveLastName();
On 2016/08/12 00:39:21, Dan Ehrenberg wrote:
> I'm surprised that you have to remove two names here, while the following
arrow
> function code does not seem to have to remove any. How does this work?

well, the `async` is removed on line 2896. As for "multiple arguments", looks
like it works already due to the FuncNameInferrer::State added in
ParseAssignmentExpression, so all names pushed within an AssignmentExpression
are omit.

I think it may be possible to rework this to use FuncNameInferrer::State, but
would still need to add extra code to prevent it from rewinding the name stack
if something turns out not to be an async function. I'm not sure if that's any
better than how it is now.

Either way, more tests coming.

[caitp, 2016-08-12 01:25:00]

added more tests. I'll try making it more elegant tomorrow, but as mentioned
inline, it still requires adding new apis to the FuncNameInferrer, so it won't
be perfect.

[caitp, 2016-08-12 01:25:24]

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

[commit-bot: I haz the power, 2016-08-12 01:25:26]

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, 2016-08-12 02:00:04]

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

[commit-bot: I haz the power, 2016-08-12 02:00:05]

Dry run: This issue passed the CQ dry run.

[marja, 2016-08-12 09:20:56]

Can you post a link to the relevant parts of the spec (if any) where the
function names are specified.

You say the function name should be empty (instead of not existing); why not
"(anonymous function)"?

Also (only tangentially related to this CL), I'm slightly confused by:


function baz(f) { f(); }

baz(function() { throw Error; })

The stack is:
(anonymous function)
baz
(anonymous function) << the interpreter

(Makes sense.)

But:
baz(() => { throw Error; })

The stack is:
baz
baz
(anonymous function) << the interpreter

Why is this?

(So, idk what to expect of async arrow functions either since I'm surprised by
this..)

[caitp, 2016-08-12 13:33:00]

On 2016/08/12 09:20:56, marja wrote:
> Can you post a link to the relevant parts of the spec (if any) where the
> function names are specified

The spec doesn't say anything about any of this. This was originally added in
https://codereview.chromium.org/62146, which has some rationales for it which
may or may not still apply today. I suppose it's useful for stacktraces to say
`<object var>..<function name> [as <property name>]` instead of just `<function
nam>`, but not sure who came up with it.
.
> 
> You say the function name should be empty (instead of not existing); why not
> "(anonymous function)"?

"empty" mostly refers to the stack of names that are used to produce the
inferred name, which should probably be empty for all arrow functions. Rendering
them as "(anonymous function)" would be fine, I think.

> 
> Also (only tangentially related to this CL), I'm slightly confused by:
> 
> 
> function baz(f) { f(); }
> 
> baz(function() { throw Error; })
> 
> The stack is:
> (anonymous function)
> baz
> (anonymous function) << the interpreter
> 
> (Makes sense.)
> 
> But:
> baz(() => { throw Error; })
> 
> The stack is:
> baz
> baz
> (anonymous function) << the interpreter
> 
> Why is this?
> 
> (So, idk what to expect of async arrow functions either since I'm surprised by
> this..)

I'm not completely sure. When parsing arguments for baz(), each argument gets
its own FNI state, which should reset the names that can be generated for them.
But somehow it slips up and includes the last name of the call in the list, for
arrow functions.

[caitp, 2016-08-12 16:57:13]

There are a lot of tests which depend on the current behaviour of the
FuncNameInferrer, so I don't want to alter the behaviour of including `baz` in
the stacktrace there.

I tried an approach to fix that (by inferring "(anonymous function)" or the
empty string) when the trying to infer a name outside of the current "entry".
This produced expected (to me) results for arrow functions (async or otherwise),
but it causes a lot of other tests which expect the current behaviour to fail
(including test-api.cc tests, which makes me really not want to break it :))

I think we may be stuck with the weird behaviour.

My attempts at using the FuncNameInferrer::State to simplify the fix here seems
to negatively impact performance, as it has to be applied in more than just
those few specialized cases, so it's probably not the way to go.

[caitp, 2016-08-12 18:11:38]

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

[commit-bot: I haz the power, 2016-08-12 18:11:39]

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, 2016-08-12 18:14:09]

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

[commit-bot: I haz the power, 2016-08-12 18:14:09]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10867)

[Dan Ehrenberg, 2016-08-12 18:16:41]

Seems like it's genuinely hard to do this without going back and removing it, as
you can have a function call like async(function() {}) and the function name
inferrer is supposed to work for that case. I think this patch is almost there,
I'd just recommend a cleanup.

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

https://codereview.chromium.org/2235423003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2896: fni_->RemoveNameAtIndex(async_index);
DCHECK that it is removing async? Maybe the fni API should be 'remove async at'
and do the DCHECK there, rather than exposing LastName() just for the sake of
the DCHECK in the caller.

[caitp, 2016-08-12 18:55:39]

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

[commit-bot: I haz the power, 2016-08-12 18:55:41]

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, 2016-08-12 19:12:48]

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

[commit-bot: I haz the power, 2016-08-12 19:12:49]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[caitp, 2016-08-12 21:25:19]

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

[commit-bot: I haz the power, 2016-08-12 21:25:23]

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

[Dan Ehrenberg, 2016-08-12 21:29:58]

I'm confortable with the logic now (seems unavoidable, unfortunately, to back up
and remove the async keyword, given how contextual things are), just a couple
style suggestions remaining.

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
File src/parsing/func-name-inferrer.cc (left):

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
src/parsing/func-name-inferrer.cc:23: 
Nit: Revert irrelevant whitespace change.

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
File src/parsing/func-name-inferrer.h (right):

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
src/parsing/func-name-inferrer.h:78: void RemoveAsyncKeywordAtIndex(int index);
Nit: Rather than exposing these two functions, why not just make this second
function take the index from the end of how far back it should remove the async
keyword?

[caitp, 2016-08-12 21:32:45]

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
File src/parsing/func-name-inferrer.h (right):

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
src/parsing/func-name-inferrer.h:78: void RemoveAsyncKeywordAtIndex(int index);
On 2016/08/12 21:29:58, Dan Ehrenberg wrote:
> Nit: Rather than exposing these two functions, why not just make this second
> function take the index from the end of how far back it should remove the
async
> keyword?

1. prone to break with future parser changes, and 2. I think positive indexes
are easier to think about than negative indexes.

If people prefer that, sgtm

[Dan Ehrenberg, 2016-08-12 21:35:28]

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
File src/parsing/func-name-inferrer.h (right):

https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-i...
src/parsing/func-name-inferrer.h:78: void RemoveAsyncKeywordAtIndex(int index);
On 2016/08/12 at 21:32:45, caitp wrote:
> On 2016/08/12 21:29:58, Dan Ehrenberg wrote:
> > Nit: Rather than exposing these two functions, why not just make this second
> > function take the index from the end of how far back it should remove the
async
> > keyword?
> 
> 1. prone to break with future parser changes, and 2. I think positive indexes
are easier to think about than negative indexes.
> 
> If people prefer that, sgtm

The logic would be, how many things from the top of the stack would you want to
pop? Probably 0 or 1. I can't think of a case where you'd want to pop from the
beginning of the stack--that's not a reasonable thing to want to do here.

[caitp, 2016-08-12 21:47:11]

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

[commit-bot: I haz the power, 2016-08-12 21:47:22]

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

[caitp, 2016-08-12 21:48:43]

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

[commit-bot: I haz the power, 2016-08-12 21:48:52]

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

[Dan Ehrenberg, 2016-08-12 22:07:18]

lgtm

https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (left):

https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:774: 
Nit: revert whitespace change

[caitp, 2016-08-12 22:18:44]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-08-12 22:18:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2235423003/#ps160001
(title: "add braces around single-line if stmt")

[commit-bot: I haz the power, 2016-08-12 22:18:48]

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

[caitp, 2016-08-12 22:19:09]

https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (left):

https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:774: 
On 2016/08/12 22:07:18, Dan Ehrenberg wrote:
> Nit: revert whitespace change

Done, and some other style mixups fixed

[commit-bot: I haz the power, 2016-08-12 22:47:02]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-12 22:47:22]

Description was changed from

==========
[parser] improve inferred function names for async arrow functions

No longer include the "async" keyword, or an async arrow function's single
identifier parameter as part of its inferred name.

BUG=v8:5281, v8:4483
R=adamk@chromium.org, littledan@chromium.org, marja@chromium.org
==========

to

==========
[parser] improve inferred function names for async arrow functions

No longer include the "async" keyword, or an async arrow function's single
identifier parameter as part of its inferred name.

BUG=v8:5281, v8:4483
R=adamk@chromium.org, littledan@chromium.org, marja@chromium.org

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

[commit-bot: I haz the power, 2016-08-12 22:47:23]

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