Declare 'this' as DYNAMIC_GLOBAL on the script_scope

Declare 'this' as DYNAMIC_GLOBAL on the script_scope

Follow-up cleanup to https://codereview.chromium.org/2231813003

BUG=

Committed: https://crrev.com/860a3ce26f29bb01da97a32f31e94160a48c0960
Cr-Commit-Position: refs/heads/master@{#38643}

[Toon Verwaest, 2016-08-11 15:25:16]

verwaest@chromium.org changed reviewers:
+ adamk@chromium.org

[Toon Verwaest, 2016-08-11 15:25:16]

ptal
cleanup follow-up to the other CL that you'll probably like

[Toon Verwaest, 2016-08-11 16:06:15]

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

https://codereview.chromium.org/2237873002/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1279: return NULL;
Doh. This "optimization" breaks the obvious case (for which we, surprisingly!,
don't have a test):

with ({}) a = ()=>{ "use strict"; delete this }; a()

It seems like this optimization will actually just slow down 99.999% of scope
resolutions by requiring the check. What about just dropping it?

[Toon Verwaest, 2016-08-11 17:24:21]

Seems like the latest cl breaks the debugger. I suspect a bug in debug-evaluate.
I'll check tomorrow, but you can already review the rest...

[adamk, 2016-08-11 18:27:17]

https://codereview.chromium.org/2237873002/diff/100001/src/ast/scopes.cc
File src/ast/scopes.cc (left):

https://codereview.chromium.org/2237873002/diff/100001/src/ast/scopes.cc#oldc...
src/ast/scopes.cc:1276: if (already_resolved() && is_with_scope()) {
I'm fine with dropping this check, it's a very strange thing to optimize for.

https://codereview.chromium.org/2237873002/diff/100001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2237873002/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1309: if (is_with_scope() && name_can_be_shadowed) {
This code has moved around due to your other CL, though as I noted over there
I'm not sure I agree that it was a cleanup...will want to see a rebase in any
case.

https://codereview.chromium.org/2237873002/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2237873002/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:1001: // If there's a chance that there's a reference to
global 'this', predeclare
What does the "If there's a chance" add to this comment?

https://codereview.chromium.org/2237873002/diff/100001/test/mjsunit/global-ar...
File test/mjsunit/global-arrow-delete-this.js (right):

https://codereview.chromium.org/2237873002/diff/100001/test/mjsunit/global-ar...
test/mjsunit/global-arrow-delete-this.js:12: a()()
Do you want to assert something here? (what was the behavior before?)

[Toon Verwaest, 2016-08-12 04:49:41]

I'll some answers to questions, still need to fix. I'll rebase in the process.

https://codereview.chromium.org/2237873002/diff/100001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2237873002/diff/100001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1309: if (is_with_scope() && name_can_be_shadowed) {
On 2016/08/11 18:27:17, adamk wrote:
> This code has moved around due to your other CL, though as I noted over there
> I'm not sure I agree that it was a cleanup...will want to see a rebase in any
> case.

It was a cleanup in the sense that doesn't have unnecessary control flow, which
I find more readable, but it's also more efficient. I guess the "cleaner" is a
matter of taste then.

https://codereview.chromium.org/2237873002/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2237873002/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:1001: // If there's a chance that there's a reference to
global 'this', predeclare
On 2016/08/11 18:27:17, adamk wrote:
> What does the "If there's a chance" add to this comment?

How would you reformulate? I'm just trying to indicate that there's no guarantee
that there's a reference, but that we don't insert it if there can't be a
reference.

https://codereview.chromium.org/2237873002/diff/100001/test/mjsunit/global-ar...
File test/mjsunit/global-arrow-delete-this.js (right):

https://codereview.chromium.org/2237873002/diff/100001/test/mjsunit/global-ar...
test/mjsunit/global-arrow-delete-this.js:12: a()()
On 2016/08/11 18:27:17, adamk wrote:
> Do you want to assert something here? (what was the behavior before?)

It dcheck fails in the back-ends if I don't create the this bindings in the
correct way. These two subtests check that it works in all cornercases.

[Toon Verwaest, 2016-08-12 04:58:46]

https://codereview.chromium.org/2237873002/diff/100001/test/mjsunit/global-ar...
File test/mjsunit/global-arrow-delete-this.js (right):

https://codereview.chromium.org/2237873002/diff/100001/test/mjsunit/global-ar...
test/mjsunit/global-arrow-delete-this.js:12: a()()
On 2016/08/12 04:49:40, Toon Verwaest wrote:
> On 2016/08/11 18:27:17, adamk wrote:
> > Do you want to assert something here? (what was the behavior before?)
> 
> It dcheck fails in the back-ends if I don't create the this bindings in the
> correct way. These two subtests check that it works in all cornercases.

Plus delete this throws in strict mode if it doesn't work properly. (Doesn't
identify this as this)

[Toon Verwaest, 2016-08-12 08:36:56]

verwaest@chromium.org changed reviewers:
+ yangguo@chromium.org

[Toon Verwaest, 2016-08-12 08:36:57]

ptal

I restored the "short-cut" just for debug-evaluate. It relied on this quirk to
do broken stuff. If we do scope resolution by hand anywhere outside of this
phase during parsing it'll be wrong in debug-evaluate. Auch. Added TODOs for
yang to properly create scope-info for debug-evaluate scopes.

[Toon Verwaest, 2016-08-12 08:38:26]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 08:38:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-08-12 08:50:20]

https://codereview.chromium.org/2237873002/diff/120001/src/ast/scopes.cc
File src/ast/scopes.cc (left):

https://codereview.chromium.org/2237873002/diff/120001/src/ast/scopes.cc#oldc...
src/ast/scopes.cc:557: DCHECK(scope_info_->StackSlotIndex(*name_handle) < 0 ||
is_block_scope());
I don't see how this would be valid for block scopes. We should only get here
for resolving variables of a scope outside of a function, and block-scoped
variables shouldn't be stack-allocated there either.

[commit-bot: I haz the power, 2016-08-12 09:07:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 09:07:23]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-08-12 10:29:11]

lgtm

[adamk, 2016-08-15 17:39:43]

lgtm

https://codereview.chromium.org/2237873002/diff/100001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2237873002/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:1001: // If there's a chance that there's a reference to
global 'this', predeclare
On 2016/08/12 04:49:40, Toon Verwaest wrote:
> On 2016/08/11 18:27:17, adamk wrote:
> > What does the "If there's a chance" add to this comment?
> 
> How would you reformulate? I'm just trying to indicate that there's no
guarantee
> that there's a reference, but that we don't insert it if there can't be a
> reference.

Makes sense, I missed the fact that the GetReceiverScope() is doing exactly the
"if there's a chance" thing. This is fine on a second reading.

[Toon Verwaest, 2016-08-16 07:18:55]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2016-08-16 07:19:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 07:44:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-16 07:44:21]

Description was changed from

==========
Declare 'this' as DYNAMIC_GLOBAL on the script_scope

Follow-up cleanup to https://codereview.chromium.org/2231813003

BUG=
==========

to

==========
Declare 'this' as DYNAMIC_GLOBAL on the script_scope

Follow-up cleanup to https://codereview.chromium.org/2231813003

BUG=

Committed: https://crrev.com/860a3ce26f29bb01da97a32f31e94160a48c0960
Cr-Commit-Position: refs/heads/master@{#38643}
==========

[commit-bot: I haz the power, 2016-08-16 07:44:22]

Patchset 7 (id:??) landed as
https://crrev.com/860a3ce26f29bb01da97a32f31e94160a48c0960
Cr-Commit-Position: refs/heads/master@{#38643}