Implement proper scoping for "this" in arrow functions

Implement proper scoping for "this" in arrow functions

BUG=v8:2700
LOG=Y

[aperez, 2015-01-27 18:22:31]

aperez@igalia.com changed reviewers:
+ wingo@igalia.com

[wingo, 2015-02-04 09:18:58]

Sorry for the delay.  So I would first note that only ia32, x64, arm, and arm64
are "supported" targets -- so don't bother with other arches.  The maintainers
of those arches will pick up the pieces.  It sounds irresponsible but really
it's not, because it reduces the review burden, and you get experts on those
arches making the needed changes, and it makes your burden as a patch author
less.  Please update patch without the other arches :)

[wingo, 2015-02-04 10:02:13]

A first review.

https://codereview.chromium.org/883823002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/883823002/diff/20001/src/ast.h#newcode1633
src/ast.h:1633: return !is_this() && (!is_resolved() ||
var()->IsValidReference());
So it used to be that we would eagerly resolve "this" references, and we would
fall through to the second case where var()->IsValidReference() returns false. 
But now that we create unresolved "this" references we have to eagerly prevent
unresolved "this" references from being valid left-hand-sides.

Given that, it seems to me that we need to update Variable to not have an
is_valid_ref() field, as it's not needed any more -- the only cases I could see
where it mattered were for "this".  (SuperExpression is not a ThisExpression so
the comment in variables.h is outdated.)  Can you give that a try?  Then we
remove the RHS of the && here.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1178
src/scopes.cc:1178: 
extra whitespace

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1206
src/scopes.cc:1206: // which is not preallocated.
Confusing comment.  Also  I think the conditional below should be reassociated:
if var->is_this(), then certainly var->is_used() (per the if block above).

i.e. return var->is_this() || (var->is_used() &&
!var->IsGlobalObjectProperty());

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1299
src/scopes.cc:1299: DCHECK(receiver()->scope() == this);
the second DCHECK is sufficient, I think, if we add the non-null DCHECK to
receiver() itself

https://codereview.chromium.org/883823002/diff/20001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode149
src/scopes.h:149: bool is_this = false) {
Boolean parameters are an antipattern, especially optional boolean parameters,
because they read so poorly.  Please create a small enum somewhere.  I know that
is_this is used in other parts of the API but it's bad there too.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode343
src/scopes.h:343: DCHECK(has_this_declaration());
DCHECK(receiver_) also perhaps?

[aperez, 2015-02-04 21:00:42]

https://codereview.chromium.org/883823002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/883823002/diff/20001/src/ast.h#newcode1633
src/ast.h:1633: return !is_this() && (!is_resolved() ||
var()->IsValidReference());
On 2015/02/04 10:02:13, wingo wrote:
> So it used to be that we would eagerly resolve "this" references, and we would
> fall through to the second case where var()->IsValidReference() returns false.

> But now that we create unresolved "this" references we have to eagerly prevent
> unresolved "this" references from being valid left-hand-sides.
> 
> Given that, it seems to me that we need to update Variable to not have an
> is_valid_ref() field, as it's not needed any more -- the only cases I could
see
> where it mattered were for "this".  (SuperExpression is not a ThisExpression
so
> the comment in variables.h is outdated.)  Can you give that a try?  Then we
> remove the RHS of the && here.

Acknowledged.

Also, I am going to write it as:

  return !(is_this() || is_unresolved());

Somehow my mind can parse it more easily that way.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1178
src/scopes.cc:1178: 
On 2015/02/04 10:02:13, wingo wrote:
> extra whitespace

Acknowledged.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1206
src/scopes.cc:1206: // which is not preallocated.
On 2015/02/04 10:02:13, wingo wrote:
> Confusing comment.  Also  I think the conditional below should be
reassociated:
> if var->is_this(), then certainly var->is_used() (per the if block above).
> 
> i.e. return var->is_this() || (var->is_used() &&
> !var->IsGlobalObjectProperty());

Acknowledged.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.cc#newcode1299
src/scopes.cc:1299: DCHECK(receiver()->scope() == this);
On 2015/02/04 10:02:13, wingo wrote:
> the second DCHECK is sufficient, I think, if we add the non-null DCHECK to
> receiver() itself

Acknowledged.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode149
src/scopes.h:149: bool is_this = false) {
On 2015/02/04 10:02:13, wingo wrote:
> Boolean parameters are an antipattern, especially optional boolean parameters,
> because they read so poorly.  Please create a small enum somewhere.  I know
that
> is_this is used in other parts of the API but it's bad there too.

Acknowledged. Even better: the actual name of the variable is always passed in,
so why not checking that “name == ast_value_factory->this_string()”?  I am gonna
go with that, and it will save one argument being passed around.

https://codereview.chromium.org/883823002/diff/20001/src/scopes.h#newcode343
src/scopes.h:343: DCHECK(has_this_declaration());
On 2015/02/04 10:02:13, wingo wrote:
> DCHECK(receiver_) also perhaps?

Acknowledged.

[wingo, 2015-02-05 11:08:30]

Looking much better!  A nit to fix.  I haven't reviewed the codegen/compiler
tests yet.

https://codereview.chromium.org/883823002/diff/60001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/883823002/diff/60001/src/ast.h#newcode1696
src/ast.h:1696: VariableProxy(AstValueFactory *ast_value_factory,
Humm, seems to me that passing in the factory is not quite right -- the
VariableProxy doesn't need to create values, just check if the value is "this"
or not.  How about going back to what it was, but replacing the "bool is_this"
with Variable::Kind variable_kind.

[aperez, 2015-02-06 16:05:08]

https://codereview.chromium.org/883823002/diff/60001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/883823002/diff/60001/src/ast.h#newcode1696
src/ast.h:1696: VariableProxy(AstValueFactory *ast_value_factory,
On 2015/02/05 11:08:30, wingo wrote:
> Humm, seems to me that passing in the factory is not quite right -- the
> VariableProxy doesn't need to create values, just check if the value is "this"
> or not.  How about going back to what it was, but replacing the "bool is_this"
> with Variable::Kind variable_kind.

Acknowledged.

[aperez, 2015-02-06 16:08:43]

On 2015/02/06 16:05:08, aperez wrote:
> https://codereview.chromium.org/883823002/diff/60001/src/ast.h
> File src/ast.h (right):
> 
> https://codereview.chromium.org/883823002/diff/60001/src/ast.h#newcode1696
> src/ast.h:1696: VariableProxy(AstValueFactory *ast_value_factory,
> On 2015/02/05 11:08:30, wingo wrote:
> > Humm, seems to me that passing in the factory is not quite right -- the
> > VariableProxy doesn't need to create values, just check if the value is
"this"
> > or not.  How about going back to what it was, but replacing the "bool
is_this"
> > with Variable::Kind variable_kind.
> 
> Acknowledged.

Also, I have added one of the small changes needed into the test suite needed to
take into account that “this” can now end up in the context. I am working on
some
more small fixes to the test suites, and the debugger support that handle “this”
possibly being in the context.

[wingo, 2015-02-06 17:31:19]

https://codereview.chromium.org/883823002/diff/120001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/883823002/diff/120001/src/ast.h#newcode3424
src/ast.h:3424: (name == ast_value_factory_->this_string())
Can we instead change from "bool is_this," to "Variable::Kind variable_kind" ?

That way we avoid the load and comparison of this_string on what is a relatively
hot path of the parser -- perhaps a silly concern -- and we preserve readability
of callers.

https://codereview.chromium.org/883823002/diff/120001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/883823002/diff/120001/src/parser.cc#newcode679
src/parser.cc:679: ast_value_factory()->this_string(),
here probably we will have to pass in Variable::THIS

https://codereview.chromium.org/883823002/diff/120001/src/parser.cc#newcode735
src/parser.cc:735: ? factory->NewVariableProxy(name, interface, pos)
instead of removing false, replace with Variable::NORMAL

https://codereview.chromium.org/883823002/diff/120001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/883823002/diff/120001/src/scopes.h#newcode155
src/scopes.h:155: factory->NewVariableProxy(name, interface, position);
Please make this function take an additional Variable::Kind parameter, and pass
that parameter through to NewVariableProxy

[aperez, 2015-02-09 16:08:14]

https://codereview.chromium.org/883823002/diff/120001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/883823002/diff/120001/src/ast.h#newcode3424
src/ast.h:3424: (name == ast_value_factory_->this_string())
On 2015/02/06 17:31:19, wingo wrote:
> Can we instead change from "bool is_this," to "Variable::Kind variable_kind" ?
> 
> That way we avoid the load and comparison of this_string on what is a
relatively
> hot path of the parser -- perhaps a silly concern -- and we preserve
readability
> of callers.

Acknowledged.

https://codereview.chromium.org/883823002/diff/120001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/883823002/diff/120001/src/parser.cc#newcode679
src/parser.cc:679: ast_value_factory()->this_string(),
On 2015/02/06 17:31:19, wingo wrote:
> here probably we will have to pass in Variable::THIS

Acknowledged.

https://codereview.chromium.org/883823002/diff/120001/src/parser.cc#newcode735
src/parser.cc:735: ? factory->NewVariableProxy(name, interface, pos)
On 2015/02/06 17:31:19, wingo wrote:
> instead of removing false, replace with Variable::NORMAL

Acknowledged.