Temporal dead zone behaviour for let bindings.

Temporal dead zone behaviour for let bindings.

BUG=
TEST=mjsunit/harmony/block-let-semantics.js

Committed: http://code.google.com/p/v8/source/detail?r=9070

[Steven, 2011-08-18 11:47:49]

Hi Florian, PTAL at the following code for semantics of let bindings from the
harmony proposal. 

A let binding is created uninitialized. That state is represented by using the
hole value. Initialization happens
upon evaluation of the declaration. Every access to an
uninitialized let binding should throw a reference error. For more information
about the proposed behaviour see:
http://wiki.ecmascript.org/doku.php?id=harmony:block_scoped_bindings

This CL does not yet address a blocked scoped implementation of const nor does
it deal with global let bindings.

Thanks!
-- Steven

[rossberg, 2011-08-22 15:12:10]

http://codereview.chromium.org/7671042/diff/3001/test/mjsunit/harmony/block-l...
File test/mjsunit/harmony/block-let-semantics.js (right):

http://codereview.chromium.org/7671042/diff/3001/test/mjsunit/harmony/block-l...
test/mjsunit/harmony/block-let-semantics.js:92: // initialized upon entering a
function / block scope.
Do we have tests that check that function declarations are not actually hoisted
anymore, and that they see the inner scope?

[fschneider, 2011-08-23 08:36:12]

http://codereview.chromium.org/7671042/diff/3001/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7671042/diff/3001/src/contexts.cc#newcode98
src/contexts.cc:98: 
It's not so ideal to have the output parameter binding_flags uninitialized on
some paths. I'd also initialize binding_flags to a default value as it is done
for the other two parameters index and attributes.

http://codereview.chromium.org/7671042/diff/3001/src/contexts.h
File src/contexts.h (right):

http://codereview.chromium.org/7671042/diff/3001/src/contexts.h#newcode379
src/contexts.h:379: BindingFlags* binding_flags);
Reformat to make all parameters on a separate line.

http://codereview.chromium.org/7671042/diff/3001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7671042/diff/3001/src/runtime.cc#newcode8568
src/runtime.cc:8568: Handle<Context> context =
Handle<Context>(Context::cast(*holder));
I think you should rewrite this to avoid creating a new handle:

Handle<Context> context = Handle<Context>::cast(holder);

[Steven, 2011-08-23 12:35:44]

Addressed the comments. PTAL again.

http://codereview.chromium.org/7671042/diff/3001/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7671042/diff/3001/src/contexts.cc#newcode98
src/contexts.cc:98: 
On 2011/08/23 08:36:12, fschneider wrote:
> It's not so ideal to have the output parameter binding_flags uninitialized on
> some paths. I'd also initialize binding_flags to a default value as it is done
> for the other two parameters index and attributes.

Done.

http://codereview.chromium.org/7671042/diff/3001/src/contexts.h
File src/contexts.h (right):

http://codereview.chromium.org/7671042/diff/3001/src/contexts.h#newcode379
src/contexts.h:379: BindingFlags* binding_flags);
On 2011/08/23 08:36:12, fschneider wrote:
> Reformat to make all parameters on a separate line.

Done.

http://codereview.chromium.org/7671042/diff/3001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7671042/diff/3001/src/runtime.cc#newcode8568
src/runtime.cc:8568: Handle<Context> context =
Handle<Context>(Context::cast(*holder));
On 2011/08/23 08:36:12, fschneider wrote:
> I think you should rewrite this to avoid creating a new handle:
> 
> Handle<Context> context = Handle<Context>::cast(holder);

Done.

http://codereview.chromium.org/7671042/diff/3001/test/mjsunit/harmony/block-l...
File test/mjsunit/harmony/block-let-semantics.js (right):

http://codereview.chromium.org/7671042/diff/3001/test/mjsunit/harmony/block-l...
test/mjsunit/harmony/block-let-semantics.js:92: // initialized upon entering a
function / block scope.
On 2011/08/22 15:12:10, rossberg wrote:
> Do we have tests that check that function declarations are not actually
hoisted
> anymore, and that they see the inner scope?

Done.

[fschneider, 2011-08-23 13:14:33]

I think you need to change the Crankshaft graph builder to bailout in case of
let declarations. Otherwise, there won't be the expected behavior from generated
code. e.g. the following example would optimize f and doesn't throw a
ReferenceError when invoking f(42, false) after optimization:


function f(x, b) {
  let y = (b ? y : x) + 42;
  return y;
}

for (var i=0; i<10000000; i++) f(42, false);
print(f(42, true));

[fschneider, 2011-08-24 08:38:43]

One more comment: If the plan is to support this flag only on ia32, then you
need to disable it for the other platforms (x64, arm, ...)

On 2011/08/23 13:14:33, fschneider wrote:
> I think you need to change the Crankshaft graph builder to bailout in case of
> let declarations. Otherwise, there won't be the expected behavior from
generated
> code. e.g. the following example would optimize f and doesn't throw a
> ReferenceError when invoking f(42, false) after optimization:
> 
> 
> function f(x, b) {
>   let y = (b ? y : x) + 42;
>   return y;
> }
> 
> for (var i=0; i<10000000; i++) f(42, false);
> print(f(42, true));

[Steven, 2011-08-29 14:43:53]

Added bailouts to hydrogen in case LET bindings are encountered and ported the
code to x64 and arm.

On 2011/08/24 08:38:43, fschneider wrote:
> One more comment: If the plan is to support this flag only on ia32, then you
> need to disable it for the other platforms (x64, arm, ...)
> 
> On 2011/08/23 13:14:33, fschneider wrote:
> > I think you need to change the Crankshaft graph builder to bailout in case
of
> > let declarations. Otherwise, there won't be the expected behavior from
> generated
> > code. e.g. the following example would optimize f and doesn't throw a
> > ReferenceError when invoking f(42, false) after optimization:
> > 
> > 
> > function f(x, b) {
> >   let y = (b ? y : x) + 42;
> >   return y;
> > }
> > 
> > for (var i=0; i<10000000; i++) f(42, false);
> > print(f(42, true));

[fschneider, 2011-08-30 06:58:40]

LGTM.

http://codereview.chromium.org/7671042/diff/11001/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-let-crankshaft.js (right):

http://codereview.chromium.org/7671042/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-crankshaft.js:55: } catch (e) {
Use assertThrows to test if there actually was an exception thrown.

http://codereview.chromium.org/7671042/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-crankshaft.js:60: g(42, true);
Also assertThrows here.

http://codereview.chromium.org/7671042/diff/11001/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-let-semantics.js (right):

http://codereview.chromium.org/7671042/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-semantics.js:36: } catch (e) {
Use AssertThrows?

http://codereview.chromium.org/7671042/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-semantics.js:45: eval("(function(){ {" + s + ";}
})")();
AssertThrows here too.