Parse harmony let declarations.

Parse harmony let declarations.

Implementation of the harmony block scoped let bindings as proposed here:
http://wiki.ecmascript.org/doku.php?id=harmony:block_scoped_bindings

Changes to the syntax are explained there. They are active under the
harmony_block_scoping_ flag in the parser.

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

[Steven, 2011-08-11 13:41:16]

PTAL

[rossberg, 2011-08-11 14:34:59]

As per our off-line discussion, I think it would be highly preferable to make
the decision whether "let" is a proper keyword in the scanner. I think that
would simplify the parser, and be more scalable to the additional Harmony
keywords.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
test/mjsunit/harmony/block-scoping.js:58: // Lookup in and through block
contexts.
It would be great if you could add tests for variable shadowing (between args,
catch, and let, in various combinations).

[Lasse Reichstein, 2011-08-12 08:08:33]

I assume we will never enable Harmony syntax in non-Harmony-opt-in code. I.e.,
this is only ever going to be used for testing out the feature.

Will it live on a branch, or are we planning to merge this to
bleeding_edge/trunk?

I found myself wanting to try out the patch, but I couldn't find the baseline
code. Is there a combined patch somewhere that can be used to run the code?

http://codereview.chromium.org/7616009/diff/1/preparser/preparser-process.cc
File preparser/preparser-process.cc (right):

http://codereview.chromium.org/7616009/diff/1/preparser/preparser-process.cc#...
preparser/preparser-process.cc:41: } }  // namespace v8::internal
Put this in preparser-api.cc.
That's where other "essential" parts of V8 are emulated.

Or, preferably, don't use FLAG_harmony_block_scoping in the scanner or
preparser, but pass a flag to the scanner/parser when they are created. Better
to inject than to depend on global variables. It reduces dependencies.

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
Should let be grouped with dynamic? If it occurs inside a loop, and the scope
also contains a function expression, it's possible to capture multiple versions
of the same variable (similar to a catch-scope). I.e., it can't be assigned to a
single stack slot.

http://codereview.chromium.org/7616009/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1108
src/parser.cc:1108: return ParseVariableStatement(true, ok);
Please use an enum with meaningful names for the first argument. Generally try
to avoid passing booleans as true/false (or other magical constants).

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1535
src/parser.cc:1535: inside_with());
What baseline is this patch based on? I don't see any block-scope in the current
code?

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1553
src/parser.cc:1553: Statement* stat = ParseSourceElement(NULL, CHECK_OK);
Will this enable function declarations inside blocks in strict mode? It's
allowed as a SourceElement but not as a Statement.

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode3562
src/parser.cc:3562: next = Token::FUTURE_STRICT_RESERVED_WORD;
Why not just add a case for Token::LET next to
Token::FUTURE_STRICT_RESERVED_WORD?
Or not bother, and let it be handled like a keyword? 
Both should work.

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode3752
src/parser.cc:3752: !harmony_block_scoping_)
If harmony_block_scoping_ is set, function declarations are not hoisted?

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode4055
src/parser.cc:4055: // Parses and identifier that is valid for the current
scope, in particular it
"and"->"an" while you are here :)

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc
File src/preparser.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode89
src/preparser.cc:89: // fallthrough
No need to write "fallthrough" if the two case labels are adjacent. It's only if
there is some other code in the first one that it's necessary.

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode125
src/preparser.cc:125: }
Need fallthrough here. Or drop the switch and just do:
 if (peek()== i::Token::LET && harmony_block_scoping_) {
   return ParseVariabelStatement(ACCEPT_LET, ok);
 }
 return ParseStatement(ok);

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode344
src/preparser.cc:344: // is the entire function (ECMA-262, 3rd, 10.1.3, and
12.2). .
This comment seems to be outdated now.
And it has an extra '.' at the end :)

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
test/mjsunit/harmony/block-scoping.js:58: // Lookup in and through block
contexts.
Agree. There is no test here that can distinguish a let from a hoisted var.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
test/mjsunit/harmony/block-scoping.js:102: }
Try creating let variables in a loop. E.g.:

var accessors = [];
for (var i = 0; i < 10; i++) {
  let x = i;
  accessors[i] = function() { return x; }
}
for (var i = 0; i < 10; i++) {
  assertEquals(i, accessors[i]());
}

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-bloc...
File test/mjsunit/harmony/debug-blockscopes.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-bloc...
test/mjsunit/harmony/debug-blockscopes.js:387: // CheckScopeContent({z:4}, 1,
exec_state);
Remove the TODO and commented code.
If it's not working, file a bug for it and put a test in mjsunit/bugs (those are
assumed to fail).

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-eval...
File test/mjsunit/harmony/debug-evaluate-blockscopes.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-eval...
test/mjsunit/harmony/debug-evaluate-blockscopes.js:28: // Flags:
--expose-debug-as debug
Needs harmony-block-scoping flag?
Does it run without? And if so, is something wrong?

[Steven, 2011-08-16 09:05:32]

Can you PTAL again.

http://codereview.chromium.org/7616009/diff/1/preparser/preparser-process.cc
File preparser/preparser-process.cc (right):

http://codereview.chromium.org/7616009/diff/1/preparser/preparser-process.cc#...
preparser/preparser-process.cc:41: } }  // namespace v8::internal
It's now injected into the scanner and preparser. I did not want to change the
public api of the preparser (i.e. v8::preparser::PreParser::PreParseProgram) so
the preparser reads the flag from the scanner it gets.

On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Put this in preparser-api.cc.
> That's where other "essential" parts of V8 are emulated.
> 
> Or, preferably, don't use FLAG_harmony_block_scoping in the scanner or
> preparser, but pass a flag to the scanner/parser when they are created. Better
> to inject than to depend on global variables. It reduces dependencies.

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
Can you elaborate why it should be grouped with dynamic?

If you mean your for-loop example:
var accessors = [];
for (var i = 0; i < 10; i++) {
  let x = i;
  accessors[i] = function() { return x; }
}
here x is a fresh variable for each iteration of the loop, i.e. the accessor
closures will point to different instances of it.

The semantics of
var accessors = [];
for (let i = 0; i < 10; i++) {
  accessors[i] = function() { return i; }
}
though is now quite fixed yet. There is a choice if i is fresh for each
iteration or not.

On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Should let be grouped with dynamic? If it occurs inside a loop, and the scope
> also contains a function expression, it's possible to capture multiple
versions
> of the same variable (similar to a catch-scope). I.e., it can't be assigned to
a
> single stack slot.

http://codereview.chromium.org/7616009/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1108
src/parser.cc:1108: return ParseVariableStatement(true, ok);
Done. See the VariableDeclarationContext enum. Also removed the accept_IN from
ParseVariableDeclarations.

On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Please use an enum with meaningful names for the first argument. Generally try
> to avoid passing booleans as true/false (or other magical constants).

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1535
src/parser.cc:1535: inside_with());
It is based on a CL that was still in review. It's committed now.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> What baseline is this patch based on? I don't see any block-scope in the
current
> code?

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode1553
src/parser.cc:1553: Statement* stat = ParseSourceElement(NULL, CHECK_OK);
The proposal allows local function declarations inside blocks, but they
shouldn't be allowed in a statement context that is not a source element
context. This function is not used in ES5 normal or strict mode.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Will this enable function declarations inside blocks in strict mode? It's
> allowed as a SourceElement but not as a Statement.

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode3562
src/parser.cc:3562: next = Token::FUTURE_STRICT_RESERVED_WORD;
Now obsolete, since the scanner now produces the correct token based on a flag.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Why not just add a case for Token::LET next to
> Token::FUTURE_STRICT_RESERVED_WORD?
> Or not bother, and let it be handled like a keyword? 
> Both should work.

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode3752
src/parser.cc:3752: !harmony_block_scoping_)
Yes, function declarations are then also expected to be block scoped.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> If harmony_block_scoping_ is set, function declarations are not hoisted?

http://codereview.chromium.org/7616009/diff/1/src/parser.cc#newcode4055
src/parser.cc:4055: // Parses and identifier that is valid for the current
scope, in particular it
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> "and"->"an" while you are here :)

Done.

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc
File src/preparser.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode89
src/preparser.cc:89: // fallthrough
The scanner is now producing the correct token based on a flag. The default case
is now used for let.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> No need to write "fallthrough" if the two case labels are adjacent. It's only
if
> there is some other code in the first one that it's necessary.

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode125
src/preparser.cc:125: }
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Need fallthrough here. Or drop the switch and just do:
>  if (peek()== i::Token::LET && harmony_block_scoping_) {
>    return ParseVariabelStatement(ACCEPT_LET, ok);
>  }
>  return ParseStatement(ok);

Done.

http://codereview.chromium.org/7616009/diff/1/src/preparser.cc#newcode344
src/preparser.cc:344: // is the entire function (ECMA-262, 3rd, 10.1.3, and
12.2). .
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> This comment seems to be outdated now.
> And it has an extra '.' at the end :)

Done.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
test/mjsunit/harmony/block-scoping.js:58: // Lookup in and through block
contexts.
Added your for-loop test that distinguishes those.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Agree. There is no test here that can distinguish a let from a hoisted var.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
test/mjsunit/harmony/block-scoping.js:58: // Lookup in and through block
contexts.
On 2011/08/11 14:34:59, rossberg wrote:
> It would be great if you could add tests for variable shadowing (between args,
> catch, and let, in various combinations).

Done.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/block-scop...
test/mjsunit/harmony/block-scoping.js:102: }
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Try creating let variables in a loop. E.g.:
> 
> var accessors = [];
> for (var i = 0; i < 10; i++) {
>   let x = i;
>   accessors[i] = function() { return x; }
> }
> for (var i = 0; i < 10; i++) {
>   assertEquals(i, accessors[i]());
> }

Done.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-bloc...
File test/mjsunit/harmony/debug-blockscopes.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-bloc...
test/mjsunit/harmony/debug-blockscopes.js:387: // CheckScopeContent({z:4}, 1,
exec_state);
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Remove the TODO and commented code.
> If it's not working, file a bug for it and put a test in mjsunit/bugs (those
are
> assumed to fail).

Done.

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-eval...
File test/mjsunit/harmony/debug-evaluate-blockscopes.js (right):

http://codereview.chromium.org/7616009/diff/1/test/mjsunit/harmony/debug-eval...
test/mjsunit/harmony/debug-evaluate-blockscopes.js:28: // Flags:
--expose-debug-as debug
My fault. The flag is needed.
On 2011/08/12 08:08:33, Lasse Reichstein wrote:
> Needs harmony-block-scoping flag?
> Does it run without? And if so, is something wrong?

[rossberg, 2011-08-16 09:49:43]

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:118: // Variable shadowing.
What about shadowing between "var" and "let"?

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:123: let b = 2;
You might want to put in more assertions (here and below) checking that lookup
also gives the right binding in all shadowing scenarios.

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:169: function f8() {
You never invoke this. ;-)

[Lasse Reichstein, 2011-08-16 11:00:46]

LGTM

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
I was just trying to understand the comment above. If a variable is assigned a
fixed context slot, there can only be one instance of the variable in the
context. That doesn't seem to match the behavior of let-inside-loop, since
multiple instances can be alive at the same time.

On the other hand, this code doesn't seem to care about that, just that we know
whether the variable is read-only or not, so it's probably fine.

I believe the current consensus about for(let x = ...; ...; ...)... is that it
declares an implicit block around the for where the let lives (but for(let x in
...) will use a new instance for each iteration).

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-let-declaration.js (right):

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-declaration.js:45: 
Try checking the value of x and y here, just for sanity.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-declaration.js:51: 
Notic that these code blocks are executed using a direct call to eval in
non-strict code - i.e., they use the declaration environment of its surrounding
code (which is inside a block).
If that eval turned into an indirect call (which it REALLY should), the code
would be global (and no longer throw?).

If you want to explicitly check eval code, wrap a call to eval in a function
instead of relying on the implementation of assertThrows.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:179: y = j + 10;
Should y not just always be 10?
And I can't see "j" defined anywhere.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:183: }
And you don't call f8 (which is why it could be bugged and not fail :).

[rossberg, 2011-08-16 11:23:14]

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
On 2011/08/16 11:00:46, Lasse Reichstein wrote:
> I believe the current consensus about for(let x = ...; ...; ...)... is that it
> declares an implicit block around the for where the let lives (but for(let x
in
> ...) will use a new instance for each iteration).

There hasn't been any decision on that yet. At the last meeting, there were
voices in both directions, and Brendon himself seemed rather undecided. So I
would suggest that we go for what we consider more sensible for now (which I'd
argue are rules that are coherent between different let forms).

[Steven, 2011-08-16 15:06:08]

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
The context slot for the variable is fixed, but the context isn't. At the moment
each iteration will have a separate block context for the loop body, each one
having its once instances of block local variables.

As for the semantics of freshness of iteration variables, I had the impression
that there is no consensus yet :) The specific semantics is not important yet
and I think (hope) the implementation doesn't differ that much.

On 2011/08/16 11:00:46, Lasse Reichstein wrote:
> I was just trying to understand the comment above. If a variable is assigned a
> fixed context slot, there can only be one instance of the variable in the
> context. That doesn't seem to match the behavior of let-inside-loop, since
> multiple instances can be alive at the same time.
> 
> On the other hand, this code doesn't seem to care about that, just that we
know
> whether the variable is read-only or not, so it's probably fine.
> 
> I believe the current consensus about for(let x = ...; ...; ...)... is that it
> declares an implicit block around the for where the let lives (but for(let x
in
> ...) will use a new instance for each iteration).

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:118: // Variable shadowing.
On 2011/08/16 09:49:43, rossberg wrote:
> What about shadowing between "var" and "let"?

Done.

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:123: let b = 2;
On 2011/08/16 09:49:43, rossberg wrote:
> You might want to put in more assertions (here and below) checking that lookup
> also gives the right binding in all shadowing scenarios.

Done.

http://codereview.chromium.org/7616009/diff/11001/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:169: function f8() {
On 2011/08/16 09:49:43, rossberg wrote:
> You never invoke this. ;-)

Done.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-let-declaration.js (right):

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-declaration.js:45: 
On 2011/08/16 11:00:46, Lasse Reichstein wrote:
> Try checking the value of x and y here, just for sanity.

Done.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-let-declaration.js:51: 
It's more like checking the parser.

On 2011/08/16 11:00:46, Lasse Reichstein wrote:
> Notic that these code blocks are executed using a direct call to eval in
> non-strict code - i.e., they use the declaration environment of its
surrounding
> code (which is inside a block).
> If that eval turned into an indirect call (which it REALLY should), the code
> would be global (and no longer throw?).
> 
> If you want to explicitly check eval code, wrap a call to eval in a function
> instead of relying on the implementation of assertThrows.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:179: y = j + 10;
On 2011/08/16 11:00:46, Lasse Reichstein wrote:
> Should y not just always be 10?
> And I can't see "j" defined anywhere.

Done.

http://codereview.chromium.org/7616009/diff/13014/test/mjsunit/harmony/block-...
test/mjsunit/harmony/block-scoping.js:183: }
On 2011/08/16 11:00:46, Lasse Reichstein wrote:
> And you don't call f8 (which is why it could be bugged and not fail :).

Done.

[Lasse Reichstein, 2011-08-17 10:57:04]

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc
File src/contexts.cc (right):

http://codereview.chromium.org/7616009/diff/1/src/contexts.cc#newcode183
src/contexts.cc:183: case Variable::LET:
I was relying on BE's comment in
http://www.mail-archive.com/es-discuss@mozilla.org/msg08908.html
It sounded authoritative enough for me to assume it was the consensus., although
obviously, I couldn't know for sure.
(It is, however, the what I would prefer too :)