Don't reflect ES2015 Function name inference in Function.prototype.toString

Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST, parser,
and at runtime.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n

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

[adamk, 2016-02-18 21:52:42]

Description was changed from

==========
Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST and
parser.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n
==========

to

==========
Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST, parser,
and at runtime.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n
==========

[adamk, 2016-02-18 21:53:09]

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

[adamk, 2016-02-18 21:53:10]

Didn't end up using FunctionKind after all, but I still think this provides some
nice cleanup as it goes. PTAL.

[adamk, 2016-02-18 21:53:41]

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

[adamk, 2016-02-18 21:53:41]

+littledan, who could probably review this just as well

[Dan Ehrenberg, 2016-02-19 00:10:41]

This is a great cleanup together with the new tracking. I'm happy to not have
ArityRestriction and kGlobalOrEval anymore.

https://codereview.chromium.org/1712833002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1712833002/diff/1/src/ast/ast.h#newcode3421
src/ast/ast.h:3421: FunctionLiteral* NewGlobalOrEvalFunctionLiteral(
GlobalOrEval is a pretty confusing name. What if we referred to it as "Script"
(following the HTML usage).

https://codereview.chromium.org/1712833002/diff/1/src/objects-printer.cc
File src/objects-printer.cc (left):

https://codereview.chromium.org/1712833002/diff/1/src/objects-printer.cc#oldc...
src/objects-printer.cc:892: os << "\n - is expression = " << is_expression();
Maybe this should be left in, and add is_declaration?

https://codereview.chromium.org/1712833002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1712833002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:1036: // a safe default.
Could this cause the getter/setter/method-ness to be lost in practice, or are
those always going to be parsed together with the class/object literal?

[adamk, 2016-02-19 01:09:16]

https://codereview.chromium.org/1712833002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1712833002/diff/1/src/ast/ast.h#newcode3421
src/ast/ast.h:3421: FunctionLiteral* NewGlobalOrEvalFunctionLiteral(
On 2016/02/19 00:10:41, Dan Ehrenberg wrote:
> GlobalOrEval is a pretty confusing name. What if we referred to it as "Script"
> (following the HTML usage).

Hmm, but this is also used for Function-constructor-eval'd code. Does "Script"
fit in to that case, for you?

https://codereview.chromium.org/1712833002/diff/1/src/objects-printer.cc
File src/objects-printer.cc (left):

https://codereview.chromium.org/1712833002/diff/1/src/objects-printer.cc#oldc...
src/objects-printer.cc:892: os << "\n - is expression = " << is_expression();
On 2016/02/19 00:10:41, Dan Ehrenberg wrote:
> Maybe this should be left in, and add is_declaration?

Done (for all three bits). Since they're all mutually-exclusive I only print out
one.

https://codereview.chromium.org/1712833002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1712833002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:1036: // a safe default.
On 2016/02/19 00:10:41, Dan Ehrenberg wrote:
> Could this cause the getter/setter/method-ness to be lost in practice, or are
> those always going to be parsed together with the class/object literal?

So this is indeed the only really ugly bit of the change. kDeclaration,
kNamedExpression, and kAnonymousExpression are all bits that matter both at
initial parse (or preparse time) and at runtime.

But the other FunctionLiteral::FunctionType values only matter at initial parse
time. kMethod doesn't have any special behavior at all (besides not being one of
the others) while kGetter and kSetter produce an error (or not) on initial parse
(which means we'd never get into the position of lazily parsing again).

I could reconstruct the Method-ness from FunctionKind, but not the Getter vs
Setter.

Any suggestions? This indeed looks pretty brittle; if someone started to depend
on FunctionType::kGetter in another way, we'd end up with an inconsistency
between first parse and lazy parsing (or first compilation and subsequent
optimization).

[adamk, 2016-02-19 02:02:19]

https://codereview.chromium.org/1712833002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/1712833002/diff/1/src/ast/ast.h#newcode3421
src/ast/ast.h:3421: FunctionLiteral* NewGlobalOrEvalFunctionLiteral(
On 2016/02/19 01:09:15, adamk wrote:
> On 2016/02/19 00:10:41, Dan Ehrenberg wrote:
> > GlobalOrEval is a pretty confusing name. What if we referred to it as
"Script"
> > (following the HTML usage).
> 
> Hmm, but this is also used for Function-constructor-eval'd code. Does "Script"
> fit in to that case, for you?

Renamed a bit and added a comment.

https://codereview.chromium.org/1712833002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1712833002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:1036: // a safe default.
On 2016/02/19 01:09:15, adamk wrote:
> On 2016/02/19 00:10:41, Dan Ehrenberg wrote:
> > Could this cause the getter/setter/method-ness to be lost in practice, or
are
> > those always going to be parsed together with the class/object literal?
> 
> So this is indeed the only really ugly bit of the change. kDeclaration,
> kNamedExpression, and kAnonymousExpression are all bits that matter both at
> initial parse (or preparse time) and at runtime.
> 
> But the other FunctionLiteral::FunctionType values only matter at initial
parse
> time. kMethod doesn't have any special behavior at all (besides not being one
of
> the others) while kGetter and kSetter produce an error (or not) on initial
parse
> (which means we'd never get into the position of lazily parsing again).
> 
> I could reconstruct the Method-ness from FunctionKind, but not the Getter vs
> Setter.
> 
> Any suggestions? This indeed looks pretty brittle; if someone started to
depend
> on FunctionType::kGetter in another way, we'd end up with an inconsistency
> between first parse and lazy parsing (or first compilation and subsequent
> optimization).

Since I had the bit free anyway, split FunctionKind::kAccessorFunction into
kGetterFunction and kSetterFunction. Now the Getter or Setter-ness is stored in
the SFI. So we still avoid needing a separate ArityRestriction argument at the
cost of one bit in SFI.

Seems like a reasonable tradeoff for readability, to me.

[Dan Ehrenberg, 2016-02-19 02:10:55]

lgtm

Great. Modulo your STATIC_ASSERT failure, lgtm!

[adamk, 2016-02-19 02:33:05]

On 2016/02/19 02:10:55, Dan Ehrenberg wrote:
> lgtm
> 
> Great. Modulo your STATIC_ASSERT failure, lgtm!

STATIC_ASSERT handled by reordering bits in FunctionKind, tests seem happy.

[adamk, 2016-02-19 02:33:09]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-02-19 02:33:09]

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/1712833002/#ps60001
(title: "Move bits around to make STATIC_ASSERT happy")

[commit-bot: I haz the power, 2016-02-19 02:33:18]

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

[commit-bot: I haz the power, 2016-02-19 02:51:05]

Description was changed from

==========
Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST, parser,
and at runtime.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n
==========

to

==========
Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST, parser,
and at runtime.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n
==========

[commit-bot: I haz the power, 2016-02-19 02:51:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-19 02:51:21]

Description was changed from

==========
Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST, parser,
and at runtime.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n
==========

to

==========
Don't reflect ES2015 Function name inference in Function.prototype.toString

Various syntactic forms now cause functions to have names where they
didn't before. Per the upcoming changes to the toString spec, only
a name that was literally part of a function's expression or declaration
is meant to be reflected in toString. This also happens to be the same
set of names that V8 currently outputs (without the --harmony-function-name
flag).

This required distinguishing anonymous FunctionExpressions from other sorts
of function definitions (like methods and getters/setters) in the AST, parser,
and at runtime.

The patch also takes the opportunity to remove one more argument (and enum)
from FunctionLiteral, as well as adding a special factory method for the
case of a FunctionLiteral representing toplevel or eval'd code.

BUG=v8:4760
LOG=n

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

[commit-bot: I haz the power, 2016-02-19 02:51:22]

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