Remove Scope::scope_uses_this_ flag

Remove Scope::scope_uses_this_ flag

Use of the "this" variable is now tracked using scopes, like any other variable.

R=arv@chromium.org
LOG=N
BUG=

Committed: https://crrev.com/afba55965118d9ba57e53c729f52be2340e626e0
Cr-Commit-Position: refs/heads/master@{#28268}

[wingo, 2015-05-06 13:50:57]

wingo@igalia.com changed reviewers:
+ arv@chromium.org
- rossberg@chromium.org

[wingo, 2015-05-06 13:54:27]

WDYT Erik?  Also is the "scope_uses_super_property" thing actually needed?  I
could remove that too.

Regarding MayUseThis: I initially wanted to walk the scope chain until I got to
one with has_this_declaration() and then grab the receiver() but scopes
deserialized from ScopeInfo don't get receiver_ set.  So, LookupThis() instead. 
I guess I could instead ensure that receiver_ gets set and replace LookupThis()
with this_declaration_scope(); wdyt?

[arv (Not doing code reviews), 2015-05-06 14:20:51]

On 2015/05/06 13:54:27, wingo wrote:
> WDYT Erik?  Also is the "scope_uses_super_property" thing actually needed?  I
> could remove that too.

scope_uses_super_property is needed. The reason is that when we use a
super.property in a method we need to install the [[HomeObject]] on the function
instance when we create it. In the case of lazy parsing, we need to store this
in the preparse data so that we can recreate this information from the scopes

https://github.com/v8/v8-git-mirror/blob/master/src/preparse-data.h#L87
https://github.com/v8/v8-git-mirror/blob/master/src/parser.cc#L4284-L4285

However, we do not need the "inner" version of this. We can just walk up the
scope chain until we find a non eval/arrow scope and set the "uses" bit there.

[[HomeObject]] could be lexically bound now that we removed toMethod in ES6 but
it would require that we add an extra scope around object literals.

Today:

let o = {
  m() { super.toString }
};

becomes something like:

let o = {
  m() { LookupSuperProperty(this, o.m.[[HomeObject]], 'toString') }
};
o.m.[[HomeObject]] = o;

Which we could change to:

let o;
{
  let home_object;
  o = {
    m() { LookupSuperProperty(this, home_object, 'toString') }    
  }
  home_object = o;
}

I'll file a bug about this too...

> Regarding MayUseThis: I initially wanted to walk the scope chain until I got
to
> one with has_this_declaration() and then grab the receiver() but scopes
> deserialized from ScopeInfo don't get receiver_ set.  So, LookupThis()
instead. 
> I guess I could instead ensure that receiver_ gets set and replace
LookupThis()
> with this_declaration_scope(); wdyt?

I like LookupThis better. Is there a performance benefit to setting receiver_
instead?

[arv (Not doing code reviews), 2015-05-06 14:23:52]

LGTM

Could you expand the CL description explaining why this change is awesome?

https://codereview.chromium.org/1129823002/diff/1/src/preparser.h
File src/preparser.h (left):

https://codereview.chromium.org/1129823002/diff/1/src/preparser.h#oldcode2218
src/preparser.h:2218: scope_->RecordThisUsage();
It is not clear to me how this works now? Don't we use this information to
determine if we should introduce a binding for this? Maybe we always introduce a
this binding now (-arrows and eval)?

[wingo, 2015-05-06 14:37:42]

Updated the description.  Thanks for the review.

https://codereview.chromium.org/1129823002/diff/1/src/preparser.h
File src/preparser.h (left):

https://codereview.chromium.org/1129823002/diff/1/src/preparser.h#oldcode2218
src/preparser.h:2218: scope_->RecordThisUsage();
On 2015/05/06 14:23:52, arv wrote:
> It is not clear to me how this works now? Don't we use this information to
> determine if we should introduce a binding for this? Maybe we always introduce
a
> this binding now (-arrows and eval)?

ThisExpression for the full parser will create an unresolved VariableProxy which
in Scope::Analyze() will be plugged to a Variable.  When that happens the
Variable itself, wherever it is declared, will be marked as used.  The variable
will also be marked as used if there is a nested direct eval(), though perhaps
those conditions are too conservative (see Scope::MustAllocate()).  Anyway, with
all of that, the flag is redundant.

[wingo, 2015-05-06 15:25:38]

The CQ bit was checked by wingo@igalia.com

[commit-bot: I haz the power, 2015-05-06 15:25:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129823002/1

[commit-bot: I haz the power, 2015-05-06 15:50:09]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-06 15:50:19]

Patchset 1 (id:??) landed as
https://crrev.com/afba55965118d9ba57e53c729f52be2340e626e0
Cr-Commit-Position: refs/heads/master@{#28268}

[Michael Achenbach, 2015-05-07 07:46:47]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1131713004/ by machenbach@chromium.org.

The reason for reverting is: The reason for reverting is: [Sheriff] Need to fix
compilation after this revert:
https://chromium.googlesource.com/v8/v8/+/5cab6be83a44567a3ee5747d728a399025d....