[es6] implement default/optional parameters (WIP / comments)

[es6] implement default/optional parameters (WIP / comments)

Partial refactor of older bitrotten optional params patch

More work on this over the next few weeks

TODO:
  - new scope for parameter expressions
  - proper parameter list TDZ
  - make sure AST numbering is done before initializing values
  - TF support
  - Function.length calculation needs an update

EXTRAS (might be good to land in separate intermediate patches):
  - ParameterKind unifies ways of denoting "special" parameter types --- hmm, it's less nice to use because of the clang-format restrictions on non-explicit single-parameter constructors :|
  - handy AstValueFactory::IsUndefined() helper (nope, it's awful, and Expression::IsUndefinedLiteral() is good --- canning that :>)

BUG=v8:2160
R=
LOG=N

[caitp (gmail), 2015-04-10 03:19:51]

Open for comments and suggestions, but still very much a work in progress.

In particular, I'm not totally sure how to make sure bailout IDs are prepared at
this point in full-codegen

[arv (Not doing code reviews), 2015-04-10 14:39:09]

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

[arv (Not doing code reviews), 2015-04-10 14:39:09]

Keep an eye on https://codereview.chromium.org/1078093002/ which cleans up the
FormalParameter parsing.

[arv (Not doing code reviews), 2015-04-10 15:08:48]

I don't see how you set up the new scope that the parameters need?

There are lots of weird cases here,
https://bugs.webkit.org/show_bug.cgi?id=38409#c4

https://codereview.chromium.org/1053773006/diff/1/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/1053773006/diff/1/src/ast-numbering.cc#newcod...
src/ast-numbering.cc:527: VisitArguments(node->default_values());
How are the default_values arguments?

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

https://codereview.chromium.org/1053773006/diff/1/src/ast.h#newcode2509
src/ast.h:2509: ZoneList<Expression*>* default_values() const { return
default_values_; }
I'm not sure we want a parallel list for this. It might be tempting to do that
for now but eventually we need to move away from this and store BindingElements
(which may or may not have an initializer).

https://codereview.chromium.org/1053773006/diff/1/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1053773006/diff/1/src/variables.h#newcode14
src/variables.h:14: class ParameterKind {
Not sure we need a class for this. Why isn't an enum enough?

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:340: EmitInitializeOptionalParameters(info->scope(),
info->function());
I'm not sure this gets the TDZ right... maybe the tests will tell.

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:4649: Expression* default_value =
defaults->at(current_default++);
So the default list is not a direct mapping from the params list?

function f(x, y = 1, z, w = 2) {}

params:   [x, y, z, w]
defaults: [1, 2]

?

That seems a bit strange.

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:4653: default_value->AsLiteral()->IsUndefined())
continue;
undefined is a mutable global binding. We cannot make this optimization.

[caitp (gmail), 2015-04-10 15:28:01]

thx for the comments, the more the better =) especially figuring out the little
bailout id numbering issue, would be a big help

https://codereview.chromium.org/1053773006/diff/1/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/1053773006/diff/1/src/ast-numbering.cc#newcod...
src/ast-numbering.cc:527: VisitArguments(node->default_values());
On 2015/04/10 15:08:48, arv wrote:
> How are the default_values arguments?

They aren't, VisitArguments() will visit each element of a ZoneList<Expression*>
though. I think VisitExpressions() would also work. Either way, the numbering is
broken right now, I need a bit of help with that. (noted in comment, nothing in
the graph seems to have IDs set up at the time EmitInitializeOptionalParameters
is called, so it will break if the default value is anything complicated)

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

https://codereview.chromium.org/1053773006/diff/1/src/ast.h#newcode2509
src/ast.h:2509: ZoneList<Expression*>* default_values() const { return
default_values_; }
On 2015/04/10 15:08:48, arv wrote:
> I'm not sure we want a parallel list for this. It might be tempting to do that
> for now but eventually we need to move away from this and store
BindingElements
> (which may or may not have an initializer).

I agree with you, see other comment. It's a quick hack for now, since I'm not
sure how much code needs to be fixed that is just expecting VariableProxy
expressions

https://codereview.chromium.org/1053773006/diff/1/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1053773006/diff/1/src/variables.h#newcode14
src/variables.h:14: class ParameterKind {
On 2015/04/10 15:08:48, arv wrote:
> Not sure we need a class for this. Why isn't an enum enough?

I find it more readable, and it should end up being essentially an enum when
compiled anyways. Either way, it's better than having a bunch of separate
boolean flags, imo.

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:340: EmitInitializeOptionalParameters(info->scope(),
info->function());
On 2015/04/10 15:08:48, arv wrote:
> I'm not sure this gets the TDZ right... maybe the tests will tell.

I think it's mostly right, actually

1 thing I'm not positive on, is reference to formals declared to the right of
the optional parameter (which are currently initialized first).

Here's a demo:

```
d8> function f(a = b) { "use strict"; let b = 1; return a; }
undefined
d8> f()
(d8):1: ReferenceError: b is not defined
function f(a = b) { "use strict"; let b = 1; return a; }
               ^
ReferenceError: b is not defined
    at f ((d8):1:16)
    at (d8):1:1

d8> function f(a = b) { "use strict"; var b = 1; return a; }
undefined
d8> f()
undefined
```

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:4649: Expression* default_value =
defaults->at(current_default++);
On 2015/04/10 15:08:48, arv wrote:
> So the default list is not a direct mapping from the params list?
> 
> function f(x, y = 1, z, w = 2) {}
> 
> params:   [x, y, z, w]
> defaults: [1, 2]
> 
> ?
> 
> That seems a bit strange.

I think we will want to combine them into one list, since parameter lists get a
lot more complicated for destructuring. There is code which depends on the
expressions in parameter lists to all be VariableProxy, and it might be a lot of
work to go through and fix all of those cases

But I'm not sure I want to start that yet

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:4653: default_value->AsLiteral()->IsUndefined())
continue;
On 2015/04/10 15:08:49, arv wrote:
> undefined is a mutable global binding. We cannot make this optimization.

hmm :( that's true

[caitp (gmail), 2015-04-10 15:56:49]

https://codereview.chromium.org/1053773006/diff/1/src/variables.h
File src/variables.h (right):

https://codereview.chromium.org/1053773006/diff/1/src/variables.h#newcode14
src/variables.h:14: class ParameterKind {
On 2015/04/10 15:28:00, caitp wrote:
> On 2015/04/10 15:08:48, arv wrote:
> > Not sure we need a class for this. Why isn't an enum enough?
> 
> I find it more readable, and it should end up being essentially an enum when
> compiled anyways. Either way, it's better than having a bunch of separate
> boolean flags, imo.

Actually, clang-format isn't keen on this, and it looks too painful to use when
explicit constructors are required :( enum it is then

[arv (Not doing code reviews), 2015-04-10 15:58:16]

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
src/x64/full-codegen-x64.cc:340: EmitInitializeOptionalParameters(info->scope(),
info->function());
This looks wrong

> d8> function f(a = b) { "use strict"; var b = 1; return a; }
> undefined
> d8> f()
> undefined

Some rules:

 - The body is not in scope when the initializer is evaluated.
 - You cannot forward reference (the param has a TDZ)

I need to back that up with links to the spec though.

[arv (Not doing code reviews), 2015-04-10 16:04:22]

On 2015/04/10 15:58:16, arv wrote:
> https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc
> File src/x64/full-codegen-x64.cc (right):
> 
>
https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
> src/x64/full-codegen-x64.cc:340:
EmitInitializeOptionalParameters(info->scope(),
> info->function());
> This looks wrong
> 
> > d8> function f(a = b) { "use strict"; var b = 1; return a; }
> > undefined
> > d8> f()
> > undefined
> 
> Some rules:
> 
>  - The body is not in scope when the initializer is evaluated.
>  - You cannot forward reference (the param has a TDZ)
> 
> I need to back that up with links to the spec though.

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-functiondeclaration...

Step 28

[caitp (gmail), 2015-04-10 16:08:57]

On 2015/04/10 16:04:22, arv wrote:
> On 2015/04/10 15:58:16, arv wrote:
> >
https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc
> > File src/x64/full-codegen-x64.cc (right):
> > 
> >
>
https://codereview.chromium.org/1053773006/diff/1/src/x64/full-codegen-x64.cc...
> > src/x64/full-codegen-x64.cc:340:
> EmitInitializeOptionalParameters(info->scope(),
> > info->function());
> > This looks wrong
> > 
> > > d8> function f(a = b) { "use strict"; var b = 1; return a; }
> > > undefined
> > > d8> f()
> > > undefined
> > 
> > Some rules:
> > 
> >  - The body is not in scope when the initializer is evaluated.
> >  - You cannot forward reference (the param has a TDZ)
> > 
> > I need to back that up with links to the spec though.
> 
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-functiondeclaration...
> 
> Step 28

yeah, I see that too :( looks like gecko has a bug

[caitp (gmail), 2015-04-12 06:08:17]

Part of this is fixed:

```
d8> function f(cb = function() { return "needs a cb!"; }) { return cb(); }
undefined
d8> f()
"needs a cb!"
d8> f(function() { return "ok!"; })
"ok!"
d8> f()
"needs a cb!"
d8> function f(x = local) { var local = 1; return x; }
undefined
d8> f()
(d8):1: ReferenceError: local is not defined
function f(x = local) { var local = 1; return x; }
               ^
ReferenceError: local is not defined
    at f ((d8):1:16)
    at (d8):1:1
```

there's a problem with object literals, because the literal index gets confused
(since the literals are technically declared in a block scope of the enclosing
scope, there's no way to get at the literal index). I'm not totally sure what to
do about that yet

[caitp (gmail), 2015-04-12 06:57:51]

Patchset #3 (id:40001) has been deleted

[caitp (gmail), 2015-04-12 07:17:56]

On 2015/04/12 06:08:17, caitp wrote:
> Part of this is fixed:
> 
> ```
> d8> function f(cb = function() { return "needs a cb!"; }) { return cb(); }
> undefined
> d8> f()
> "needs a cb!"
> d8> f(function() { return "ok!"; })
> "ok!"
> d8> f()
> "needs a cb!"
> d8> function f(x = local) { var local = 1; return x; }
> undefined
> d8> f()
> (d8):1: ReferenceError: local is not defined
> function f(x = local) { var local = 1; return x; }
>                ^
> ReferenceError: local is not defined
>     at f ((d8):1:16)
>     at (d8):1:1
> ```
> 
> there's a problem with object literals, because the literal index gets
confused
> (since the literals are technically declared in a block scope of the enclosing
> scope, there's no way to get at the literal index). I'm not totally sure what
to
> do about that yet

wait, I think the fix for that is to just not switch to the old FunctionState,
so it should be fine then

[caitp (gmail), 2015-04-12 14:04:51]

Experimental parameter TDZ (it's not "really" achieving TDZ behaviour, but it
was very simple):

parameter_expressions_scope_ gets a reference to its target function scope, and
uses that auxiliary scope for lookup

A few of the cases proposed by oliver work reasonably well here, but not all of
them:

```
# this seems OK
d8> function f(x, y=x) { return y; }
undefined
d8> f()
undefined
d8> f(1)
1

# this seems to violate TDZ?
d8> function f(x=y, y) { return x; }
undefined
d8> f()
undefined
d8> f(undefined, 1)
1

# this seems OK
d8> function f(x, y=function() { return x; }) { return y(); }
undefined
d8> f(1)
1

# this also seems OK
d8> function f(x=function() { return y; }, y) { return x(); }
undefined
d8> f()
undefined
d8> f(undefined, 1)
1

# This seems OK
d8> function f(x=f, f) { return x("!"); }
undefined
d8> f()
(d8):1: TypeError: x is not a function
function f(x=f, f) { return x("!"); }
                            ^
TypeError: x is not a function
    at f ((d8):1:29)
    at (d8):1:1

d8> f(undefined, f)
(d8):1: TypeError: x is not a function
function f(x=f, f) { return x("!"); }
                            ^
TypeError: x is not a function
    at f ((d8):1:29)
    at f ((d8):1:29)
    at (d8):1:14

d8> f(undefined, print)
!
undefined
d8> function f(f, x=f) { return x("!"); }
undefined
d8> f()
(d8):1: TypeError: x is not a function
function f(f, x=f) { return x("!"); }
                            ^
TypeError: x is not a function
    at f ((d8):1:29)
    at (d8):1:1

d8> f(print)
!
undefined
d8> f(undefined, print)
!
undefined

# etc
```

So, there isn't a real TDZ, but the behaviour is mostly what you'd expect
Proper TDZ seems hard to do without allocating the variables multiple times,
which I'd like to avoid if possible.

(bug in the current impl: tries to set initialized parameters whether they're
allocated or not, so if an initialized variable is totally unused, you get a
problem... should be trivial to fix)

[caitp (gmail), 2015-04-12 18:03:51]

Trying to figure a way to do this:

If hasParameterExpressions is false:
  1. keep doing things the way they're already working, I guess?

else:
   1. for each bound name of parameters, allocate space (on heap or stack)
initialized to the hole
   2. for each parameter:
     a. if it's a "normal" parameter (no expressions), set the allocated
variable to the one passed on the stack
     b. else if it's an optional parameter, set the allocated variable to the
evaluation of the initializing expression
     c. else if it's a binding pattern parameter, perform step 2 for each
binding property/element of the binding pattern

   Constraints:
     - All references to the bound names of parameters need to refer to thees
variables initialized to the hole.
     - Within the function body, no hole-check is needed to reference one of
these names
     - Redeclaring one of these names using "var" should be legal
     - These allocated variables need to be reference-able from step 2 in the
algorithm above, but also from the
       function and its child scopes. However, the expressions in step 2 may not
reference any variables declared
       within the function or its nested scopes

Just thinking out loud, not totally sure how else the TDZ semantics would work

[caitp (gmail), 2015-05-01 14:47:07]

Patchset #6 (id:120001) has been deleted