[es6] conditionally ignore TDZ semantics for formals

[es6] conditionally ignore TDZ semantics for formals

If HasParameterExpressions is false, a new lexical environment is not
meant to be created for formals without expressions, because they
cannot possibly leak information.

Implementing this change allows formal binding patterns and rest
parameters to be desugared using `VAR` bindings which do not require
hole-checking, and is significantly more performant for common cases.

BUG=v8:2160, v8:811
LOG=N
R=adamk, rossberg, wingo

[caitp (gmail), 2015-09-05 16:06:51]

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

[caitp (gmail), 2015-09-05 16:10:14]

PTAL --- The CL description doesn't cover everything happening here:

- Desugaring of rest parameters is rewritten to be closer to Babel's
implementation, which performs significantly faster (This is not currently
spec-correct, but it breaks in Babel, so it shouldn't bother most users --- and
it wasn't really correct before anyways).

- Adds a notion of "ContainsExpressions", used in the spec to determine if TDZ
semantics and special scoping are needed at all for formal parameters.

The addition of ContainsExpressions allows us to get away with using "var"
declarations and avoid the unnecessary hole-checking, which surprisingly
provides a significant performance boost (because of the scoping logic used for
hole-checking)

[caitp (gmail), 2015-09-05 16:14:32]

From the benchmarks at https://crrev.com/1317113007/

With patch:
```
>>> Running suite: JSTests/RestParameters
>>> Stdout (#1):
RestParameters-Object(Score): 1244310

>>> Stdout (#2):
RestParameters-Object(Score): 1276450

>>> Stdout (#3):
RestParameters-Object(Score): 1384960

>>> Stdout (#4):
RestParameters-Object(Score): 1414450

>>> Stdout (#5):
RestParameters-Object(Score): 1323190
```

Without the patch:

```
>>> Running suite: JSTests/RestParameters
>>> Stdout (#1):
RestParameters-Object(Score): 544610

>>> Stdout (#2):
RestParameters-Object(Score): 545360

>>> Stdout (#3):
RestParameters-Object(Score): 523450

>>> Stdout (#4):
RestParameters-Object(Score): 529600

>>> Stdout (#5):
RestParameters-Object(Score): 510410
```

Which looks like a pretty significant improvement for the
no-parameter-expressions case. The other cases seem mostly predicated on
improving hole-checking logic in the interpreter and compilers

[caitp (gmail), 2015-09-05 16:16:13]

On 2015/09/05 16:14:32, caitp wrote:
> From the benchmarks at https://crrev.com/1317113007/
> 
> With patch:
> ```
> >>> Running suite: JSTests/RestParameters
> >>> Stdout (#1):
> RestParameters-Object(Score): 1244310
> 
> >>> Stdout (#2):
> RestParameters-Object(Score): 1276450
> 
> >>> Stdout (#3):
> RestParameters-Object(Score): 1384960
> 
> >>> Stdout (#4):
> RestParameters-Object(Score): 1414450
> 
> >>> Stdout (#5):
> RestParameters-Object(Score): 1323190
> ```
> 
> Without the patch:
> 
> ```
> >>> Running suite: JSTests/RestParameters
> >>> Stdout (#1):
> RestParameters-Object(Score): 544610
> 
> >>> Stdout (#2):
> RestParameters-Object(Score): 545360
> 
> >>> Stdout (#3):
> RestParameters-Object(Score): 523450
> 
> >>> Stdout (#4):
> RestParameters-Object(Score): 529600
> 
> >>> Stdout (#5):
> RestParameters-Object(Score): 510410
> ```
> 
> Which looks like a pretty significant improvement for the
> no-parameter-expressions case. The other cases seem mostly predicated on
> improving hole-checking logic in the interpreter and compilers

also, the test names are sort of messed up due to careless copy/pasting from the
object-assign benchmarks. :(

[adamk, 2015-09-08 20:23:54]

I haven't looked at the patch yet (just back from vacation), but I don't think
we can switch from %AppendElement to simple assignment. V8 should match the
spec, not Babel.

What do you mean by "wasn't really correct before"?

[caitp (gmail), 2015-09-08 20:41:36]

On 2015/09/08 20:23:54, adamk wrote:
> I haven't looked at the patch yet (just back from vacation), but I don't think
> we can switch from %AppendElement to simple assignment. V8 should match the
> spec, not Babel.
> 
> What do you mean by "wasn't really correct before"?

I mean, it didn't really behave correctly, in the original desugaring version,
or in the original version.

Try http://jsfiddle.net/pkvfzubc/ in both M45 and M47 (with
--harmony-rest-parameters):

Both %AppendElement() and the old runtime approach in m45 were respecting the
prototype chain. They didn't throw themselves, but they are not creating the
data property when they should be.

This version would be mostly unobservable unless you do something weird like
define accessors on Array.prototype. I've filed a bug to implement
%_CreateDataProperty() so that we can get adequate performance out of this (and
many other areas in the spec) without doing the wrong thing
(https://code.google.com/p/v8/issues/detail?id=4414)

In the mean time, I don't think it would hurt terribly badly to make it broken
in a subtly different way here, but with improved performance.

[caitp (gmail), 2015-09-08 20:46:41]

On 2015/09/08 20:41:36, caitp wrote:
> On 2015/09/08 20:23:54, adamk wrote:
> > I haven't looked at the patch yet (just back from vacation), but I don't
think
> > we can switch from %AppendElement to simple assignment. V8 should match the
> > spec, not Babel.
> > 
> > What do you mean by "wasn't really correct before"?
> 
> I mean, it didn't really behave correctly, in the original desugaring version,
> or in the original version.
> 
> Try http://jsfiddle.net/pkvfzubc/ in both M45 and M47 (with
> --harmony-rest-parameters):
> 
> Both %AppendElement() and the old runtime approach in m45 were respecting the
> prototype chain. They didn't throw themselves, but they are not creating the
> data property when they should be.
> 
> This version would be mostly unobservable unless you do something weird like
> define accessors on Array.prototype. I've filed a bug to implement
> %_CreateDataProperty() so that we can get adequate performance out of this
(and
> many other areas in the spec) without doing the wrong thing
> (https://code.google.com/p/v8/issues/detail?id=4414)
> 
> In the mean time, I don't think it would hurt terribly badly to make it broken
> in a subtly different way here, but with improved performance.

Actually I'm wrong, they both do seem to be creating the data property, it's
just console.log that's somehow doing the wrong thing. Something else seems to
be broken?

[adamk, 2015-09-08 21:00:36]

On 2015/09/08 20:46:41, caitp wrote:
> On 2015/09/08 20:41:36, caitp wrote:
> > On 2015/09/08 20:23:54, adamk wrote:
> > > I haven't looked at the patch yet (just back from vacation), but I don't
> think
> > > we can switch from %AppendElement to simple assignment. V8 should match
the
> > > spec, not Babel.
> > > 
> > > What do you mean by "wasn't really correct before"?
> > 
> > I mean, it didn't really behave correctly, in the original desugaring
version,
> > or in the original version.
> > 
> > Try http://jsfiddle.net/pkvfzubc/ in both M45 and M47 (with
> > --harmony-rest-parameters):
> > 
> > Both %AppendElement() and the old runtime approach in m45 were respecting
the
> > prototype chain. They didn't throw themselves, but they are not creating the
> > data property when they should be.
> > 
> > This version would be mostly unobservable unless you do something weird like
> > define accessors on Array.prototype. I've filed a bug to implement
> > %_CreateDataProperty() so that we can get adequate performance out of this
> (and
> > many other areas in the spec) without doing the wrong thing
> > (https://code.google.com/p/v8/issues/detail?id=4414)
> > 
> > In the mean time, I don't think it would hurt terribly badly to make it
broken
> > in a subtly different way here, but with improved performance.
> 
> Actually I'm wrong, they both do seem to be creating the data property, it's
> just console.log that's somehow doing the wrong thing. Something else seems to
> be broken?

What output do you see? If I run it in d8, using print(JSON.stringify(...)), I
get

[]
[1]
[1,2]

as expected.

[adamk, 2015-09-09 00:07:02]

Another high-level comment below. Performance aside, we should try to get the
behavior correct.

https://codereview.chromium.org/1308123007/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1308123007/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:959: assertDoesNotThrow("function f({x}) {
var x; }; f({});");
So this is no longer an error, but yet

function f({x = undefined}) { var x; }

is an error.

Seems like there's a deeper problem here. I need to tech up my spec-reading to
fully grok IteratorBindingInitialization, but it seems like the let-binding-ness
shouldn't have anything to do with the ContainsExpression bit.

[caitp (gmail), 2015-09-09 00:24:48]

Thanks for the look. It's not clear this can land soon, but i can keep working
on it

https://codereview.chromium.org/1308123007/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1308123007/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:959: assertDoesNotThrow("function f({x}) {
var x; }; f({});");
On 2015/09/09 00:07:02, adamk wrote:
> So this is no longer an error, but yet
> 
> function f({x = undefined}) { var x; }
> 
> is an error.
> 
> Seems like there's a deeper problem here. I need to tech up my spec-reading to
> fully grok IteratorBindingInitialization, but it seems like the
let-binding-ness
> shouldn't have anything to do with the ContainsExpression bit.

That's a known issue. I have two plans to fix it. But, there's a cost: one
requires the implementation of the spec change from July 29th re: directives in
functions with non-simple parameters, and the other requires a change to the ast
node to track the declaration kind of a variable declaration. So i thought I'd
keep that as a TODO for now. What do you think? The AST change is just so the
redeclaration error can be avoided for lexical parameters.

[adamk, 2015-09-09 00:32:04]

On 2015/09/09 00:24:48, caitp wrote:
> Thanks for the look. It's not clear this can land soon, but i can keep working
> on it
> 
>
https://codereview.chromium.org/1308123007/diff/1/test/mjsunit/harmony/destru...
> File test/mjsunit/harmony/destructuring.js (right):
> 
>
https://codereview.chromium.org/1308123007/diff/1/test/mjsunit/harmony/destru...
> test/mjsunit/harmony/destructuring.js:959: assertDoesNotThrow("function f({x})
{
> var x; }; f({});");
> On 2015/09/09 00:07:02, adamk wrote:
> > So this is no longer an error, but yet
> > 
> > function f({x = undefined}) { var x; }
> > 
> > is an error.
> > 
> > Seems like there's a deeper problem here. I need to tech up my spec-reading
to
> > fully grok IteratorBindingInitialization, but it seems like the
> let-binding-ness
> > shouldn't have anything to do with the ContainsExpression bit.
> 
> That's a known issue. I have two plans to fix it. But, there's a cost: one
> requires the implementation of the spec change from July 29th re: directives
in
> functions with non-simple parameters, and the other requires a change to the
ast
> node to track the declaration kind of a variable declaration. So i thought I'd
> keep that as a TODO for now. What do you think? The AST change is just so the
> redeclaration error can be avoided for lexical parameters.

Is the known issue catalogued somewhere (with the proper behavior spelled out)?
I realize I'm a bit late to the destructuring party, but it's worrying to me
that there are so many bugs in plain sight. Mostly in the sense that it's
unclear at any point what's done and what's still-to-be-done.