Track usage of "this" and "arguments" in Scope

Track usage of "this" and "arguments" in Scope

This adds flags in Scope to track wheter a Scope uses "this" and,
"arguments". The information is exposed via Scope::uses_this(),
and Scope::uses_arguments(), respectively. Flags for tracking
usage on any inner scope uses are available as well via
Scope::inner_uses_this(), and Scope::inner_uses_arguments().

Knowing whether scopes use "this" and "arguments" will be handy
to generate the code needed to capture their values when generating
the code for arrow functions.

BUG=v8:2700
LOG=

[arv (Not doing code reviews), 2014-08-04 18:33:11]

FYI, we will need to do the same for super soon...

[marja, 2014-08-11 08:51:39]

https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h#...
src/ast-value-factory.h:91: bool IsArguments() const { return
IsOneByteEqualTo("arguments"); }
This doesn't look correct. The AstStrings are unique, so you should be able to
compare the AstString* to AstValueFactory::arguments_string().

Is the problem that where this is used, there's no AstValueFactory? If so, that
should probably be fixed... I mean, surely the AstValueFactory is *somewhere*
since we managed to create the AstString...

[aperez, 2014-09-02 21:12:27]

On 2014/08/11 08:51:39, marja wrote:
> https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h
> File src/ast-value-factory.h (right):
> 
>
https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h#...
> src/ast-value-factory.h:91: bool IsArguments() const { return
> IsOneByteEqualTo("arguments"); }
> This doesn't look correct. The AstStrings are unique, so you should be able to
> compare the AstString* to AstValueFactory::arguments_string().
> 
> Is the problem that where this is used, there's no AstValueFactory? If so,
that
> should probably be fixed... I mean, surely the AstValueFactory is *somewhere*
> since we managed to create the AstString...

I have updated the CL, the new version picks the value returned by of
“Parser::ast_value_factory()->arguments_string()” to make the pointer
comparison. For PreParser, the flag is checked directly from the bits
int “PreParserIdentifier” and the AstValueFactory is unused.

[marja, 2014-09-04 07:36:36]

On 2014/09/02 21:12:27, aperez wrote:
> On 2014/08/11 08:51:39, marja wrote:
> > https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h
> > File src/ast-value-factory.h (right):
> > 
> >
>
https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h#...
> > src/ast-value-factory.h:91: bool IsArguments() const { return
> > IsOneByteEqualTo("arguments"); }
> > This doesn't look correct. The AstStrings are unique, so you should be able
to
> > compare the AstString* to AstValueFactory::arguments_string().
> > 
> > Is the problem that where this is used, there's no AstValueFactory? If so,
> that
> > should probably be fixed... I mean, surely the AstValueFactory is
*somewhere*
> > since we managed to create the AstString...
> 
> I have updated the CL, the new version picks the value returned by of
> “Parser::ast_value_factory()->arguments_string()” to make the pointer
> comparison. For PreParser, the flag is checked directly from the bits
> int “PreParserIdentifier” and the AstValueFactory is unused.

Okay, that part LG. The code for "is this string arguments" slightly obscure but
I think the obscurity is only because the PreParser doesn't create AstStrings
and Parser does, so working around this will make the code slightly awkward.

[aperez, 2014-09-04 12:18:49]

On 2014/09/04 07:36:36, marja wrote:
> On 2014/09/02 21:12:27, aperez wrote:
> > On 2014/08/11 08:51:39, marja wrote:
> > >
https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h
> > > File src/ast-value-factory.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/422923004/diff/20001/src/ast-value-factory.h#...
> > > src/ast-value-factory.h:91: bool IsArguments() const { return
> > > IsOneByteEqualTo("arguments"); }
> > > This doesn't look correct. The AstStrings are unique, so you should be
able
> to
> > > compare the AstString* to AstValueFactory::arguments_string().
> > > 
> > > Is the problem that where this is used, there's no AstValueFactory? If so,
> > that
> > > should probably be fixed... I mean, surely the AstValueFactory is
> *somewhere*
> > > since we managed to create the AstString...
> > 
> > I have updated the CL, the new version picks the value returned by of
> > “Parser::ast_value_factory()->arguments_string()” to make the pointer
> > comparison. For PreParser, the flag is checked directly from the bits
> > int “PreParserIdentifier” and the AstValueFactory is unused.
> 
> Okay, that part LG. The code for "is this string arguments" slightly obscure
but
> I think the obscurity is only because the PreParser doesn't create AstStrings
> and Parser does, so working around this will make the code slightly awkward.

Great. Shall we land this as it is now, or shall I add the methods to
check for usage of “super” in a Scope first? (This seems easy to derive
from SuperReference :D).

[marja, 2014-09-04 12:28:02]

Idk. Somebody should do an actual review (I was only looking at the
AstValueFactory handling part so far). dslomov?

[arv (Not doing code reviews), 2014-09-04 16:41:21]

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:950: { "var x = function () { this.foo = 42 };",
Why do we need to track the this reference inside a function expression?

[rossberg, 2014-09-10 06:33:56]

https://codereview.chromium.org/422923004/diff/60001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/422923004/diff/60001/src/preparser.h#newcode613
src/preparser.h:613: bool IsArguments(const AstValueFactory*) const {
Why is this change necessary when the argument isn't even used?

https://codereview.chromium.org/422923004/diff/60001/src/preparser.h#newcode831
src/preparser.h:831: if (ident.IsEval() || ident.IsArguments(nullptr) ||
ident.IsYield() ||
Unfortunately, we can't use C++11 yet...

https://codereview.chromium.org/422923004/diff/60001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/422923004/diff/60001/src/scopes.cc#newcode1175
src/scopes.cc:1175: if (inner->scope_uses_this_ ||
inner->inner_scope_uses_this_) {
If inner is a function scope and not an arrow function, then both new flags must
not be propagated.

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:950: { "var x = function () { this.foo = 42 };",
On 2014/09/04 16:41:21, arv wrote:
> Why do we need to track the this reference inside a function expression?

Indeed, this seems wrong. Although it should be true for an arrow function.

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:968: };
We probably need some tests with block scopes as well, e.g.

  "while (true) { let x; this, arguments; }"  =>  false, false, true, true

[aperez, 2014-10-13 17:59:38]

On 2014/09/10 06:33:56, rossberg wrote:
 
>
https://codereview.chromium.org/422923004/diff/60001/src/preparser.h#newcode613
> src/preparser.h:613: bool IsArguments(const AstValueFactory*) const {
> Why is this change necessary when the argument isn't even used?

This is to make the prototypes of “Parser::IsArguments()” and
“PreParser::IsArguments()” have the same arguments. The call
site is a template which gets expanded in both of “Parser”
and “PreParser”.
 
>
https://codereview.chromium.org/422923004/diff/60001/src/preparser.h#newcode831
> src/preparser.h:831: if (ident.IsEval() || ident.IsArguments(nullptr) ||
> ident.IsYield() ||
> Unfortunately, we can't use C++11 yet...

Fixed. My brain subconsciously keeps insisting in C+11 being the
way to go... ;-)

> https://codereview.chromium.org/422923004/diff/60001/src/scopes.cc
> File src/scopes.cc (right):
> 
> https://codereview.chromium.org/422923004/diff/60001/src/scopes.cc#newcode1175
> src/scopes.cc:1175: if (inner->scope_uses_this_ ||
> inner->inner_scope_uses_this_) {
> If inner is a function scope and not an arrow function, then both new flags
must
> not be propagated.

Done. To be able to implement this I had to go ahead and add the
“ARROW_SCOPE” scope type. For the sake of keeping things simple,
I have made “Scope::is_function_scope()” return true for both
normal functions and arrow functions. To tell apart a normal
function from an arrow, one has to use “Scope::is_arrow_scope()”.
I think this matches the conceptual idea that arrow functions
are functions — of a particular kind, but functions nevertheless.

Now, parsing an arrow function effectively returns a scope of
this new “ARROW_SCOPE” type, and a couple of TODO items are now
gone.


>
https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
> test/cctest/test-parsing.cc:950: { "var x = function () { this.foo = 42 };",
> On 2014/09/04 16:41:21, arv wrote:
> > Why do we need to track the this reference inside a function expression?
> 
> Indeed, this seems wrong. Although it should be true for an arrow function.

I am not sure how to proceed WRT this. Now that propagation of
the “inner_uses_$foo” flags does not go outside normal function
scopes, it is clearly something that pertains only to arrow
functions... so how about renaming to “arrow_inner_uses_$foo”?
That would better indicate that the flag is relevant only for
arrow functions.
 
>
https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
> test/cctest/test-parsing.cc:968: };
> We probably need some tests with block scopes as well, e.g.
> 
>   "while (true) { let x; this, arguments; }"  =>  false, false, true, true

I have now added this, and a couple more.

[rossberg, 2014-10-15 12:56:32]

LGTM, with monor nits

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:927: { "f = () => { \"use strict\";", "}" },
It can't harm to keep these variants, you never know...

https://codereview.chromium.org/422923004/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/422923004/diff/80001/src/objects.h#newcode4305
src/objects.h:4305: class AsmModuleField : public BitField<bool, 11, 1> {};
Nit: while you're here, reformat these to match the others.

https://codereview.chromium.org/422923004/diff/80001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/422923004/diff/80001/src/scopes.cc#newcode786
src/scopes.cc:786: case ARROW_SCOPE:
Nit: no newline

https://codereview.chromium.org/422923004/diff/80001/src/scopes.cc#newcode1184
src/scopes.cc:1184: if (inner->is_arrow_scope() || !inner->is_function_scope())
{
Nit: switch around the two conditions (the more general one should go first).

[rossberg, 2014-10-15 12:58:03]

On 2014/10/13 17:59:38, aperez wrote:
> On 2014/09/10 06:33:56, rossberg wrote:
> > Unfortunately, we can't use C++11 yet...
> 
> Fixed. My brain subconsciously keeps insisting in C+11 being the
> way to go... ;-)

In fact, you can use it now. :)


>
https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
> > test/cctest/test-parsing.cc:950: { "var x = function () { this.foo = 42 };",
> > On 2014/09/04 16:41:21, arv wrote:
> > > Why do we need to track the this reference inside a function expression?
> > 
> > Indeed, this seems wrong. Although it should be true for an arrow function.
> 
> I am not sure how to proceed WRT this. Now that propagation of
> the “inner_uses_$foo” flags does not go outside normal function
> scopes, it is clearly something that pertains only to arrow
> functions... so how about renaming to “arrow_inner_uses_$foo”?
> That would better indicate that the flag is relevant only for
> arrow functions.

Keeping the current name is fine with me.

[aperez, 2014-10-15 17:08:40]

On 2014/10/15 12:58:03, rossberg wrote:
> On 2014/10/13 17:59:38, aperez wrote:
> > On 2014/09/10 06:33:56, rossberg wrote:
> > > Unfortunately, we can't use C++11 yet...
> > 
> > Fixed. My brain subconsciously keeps insisting in C+11 being the
> > way to go... ;-)
> 
> In fact, you can use it now. :)

Yep, as per the discussion in v8-dev about C++11. I am happy
about that :-)

>
https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
> > > test/cctest/test-parsing.cc:950: { "var x = function () { this.foo = 42
};",
> > > On 2014/09/04 16:41:21, arv wrote:
> > > > Why do we need to track the this reference inside a function expression?
> > > 
> > > Indeed, this seems wrong. Although it should be true for an arrow
function.
> > 
> > I am not sure how to proceed WRT this. Now that propagation of
> > the “inner_uses_$foo” flags does not go outside normal function
> > scopes, it is clearly something that pertains only to arrow
> > functions... so how about renaming to “arrow_inner_uses_$foo”?
> > That would better indicate that the flag is relevant only for
> > arrow functions.
> 
> Keeping the current name is fine with me.

Keeping, then.

[aperez, 2014-10-15 17:09:52]

The nits should be fixed now.

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/422923004/diff/60001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:927: { "f = () => { \"use strict\";", "}" },
On 2014/10/15 12:56:32, rossberg wrote:
> It can't harm to keep these variants, you never know...

The issue with keeping those is that making the function
strict enables block scoping, so the expected test results
vary for some the cases below, for example in thi one:

   { "if (foo()) { this.f() }",
     true, false, false, false },

It would generate those two inputs:

   function () { if (foo()) { this.f() } }
   function () { "use strict"; if (foo()) { this.f() } }

For the first one “uses_this” is expected to be “true”
(and “inner_uses_this” expected to be “false”), but for
the second one it has to be “false” because the usage
of “this” is in the block scope of the “if”...

This is why I have removed the prefixes which introduce
add the “"use strict";” and added some extra cases at the
end of “source_data” that enable strict mode.

https://codereview.chromium.org/422923004/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/422923004/diff/80001/src/objects.h#newcode4305
src/objects.h:4305: class AsmModuleField : public BitField<bool, 11, 1> {};
On 2014/10/15 12:56:32, rossberg wrote:
> Nit: while you're here, reformat these to match the others.

Acknowledged.

https://codereview.chromium.org/422923004/diff/80001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/422923004/diff/80001/src/scopes.cc#newcode786
src/scopes.cc:786: case ARROW_SCOPE:
On 2014/10/15 12:56:32, rossberg wrote:
> Nit: no newline

Acknowledged.

https://codereview.chromium.org/422923004/diff/80001/src/scopes.cc#newcode1184
src/scopes.cc:1184: if (inner->is_arrow_scope() || !inner->is_function_scope())
{
On 2014/10/15 12:56:32, rossberg wrote:
> Nit: switch around the two conditions (the more general one should go first).

Acknowledged.

[wingo (chromium), 2014-10-16 13:22:19]

Committed patchset #6 manually as 24663 (presubmit failed due to an unformatted
patch, but overridden as the changes made by git cl format were to code that was
already present, and the changes that git cl format made were terrible).