Strawman: check strong mode free variables against the global object.

[strong] Check strong mode free variables against the global object.

Gather references to unbound variables where the reference (VariableProxy) is
inside strong mode. Check them against the global object when a script is bound
to a context (during compilation).

This CL only checks unbound variables which are not inside lazy functions - TBD
how do we solve that; alternatives: add developer mode which disables laziness /
do the check whenever lazy functions are really compiled.

BUG=v8:3956
LOG=N

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

[marja, 2015-03-23 16:26:17]

marja@chromium.org changed reviewers:
+ dslomov@chromium.org

[marja, 2015-03-23 16:26:17]

dslomov, could you have an early look at this "dynamic strong mode scoping
errors CL?

The approach here is that when binding a script to a context, we check the free
variables we know of (so, not inside lazy functions) against the context.

This CL does *not* implement checking variables inside lazy functions whenever
the lazy func is compiled.

(Iirc this was the approach we talked about, though other approaches such as
doing the check whenever a function is called were on the table... so this
checks inside eager functions at compile time, not at call time.)

No tests yet because there's no trivial way to make a test consist of multiple
.js files (though, d8 does the right thing w/ "d8 foo.js bar.js", so, compiles
them separately).

[marja, 2015-03-24 09:57:11]

Hmm, this doesn't yet work for functions; functions introduced by previous
*strong mode* scripts are not seen by the later scripts - I'll need to debug
what's going on. Functions introduced by previous non-strong mode scripts work.

[Dmitry Lomov (no reviews), 2015-03-24 10:16:24]

overall looking good

https://codereview.chromium.org/1005063002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1005063002/diff/80001/src/api.cc#newcode1500
src/api.cc:1500: Maybe<bool> has = i::JSReceiver::HasOwnProperty(global, name);
Why only own properties?
Also use ScriptCongtextTable for lexicals as well

[marja, 2015-03-24 14:20:06]

dslomov, could you have another look?

This should be less strawmanny now. Changes:
- added cctests
- fixed the global object lookup and looking up lexical things (ScriptTable)
- made the error message refer to the var name & location of the reference
- fixed a bug: we need to check whether the *reference* (VariableProxy) is
inside strong mode, not whether the script scope (where the DYNAMIC_GLOBAL is)
is strong

https://codereview.chromium.org/1005063002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1005063002/diff/80001/src/api.cc#newcode1500
src/api.cc:1500: Maybe<bool> has = i::JSReceiver::HasOwnProperty(global, name);
On 2015/03/24 10:16:24, Dmitry Lomov (chromium) wrote:
> Why only own properties?
> Also use ScriptCongtextTable for lexicals as well

Done.

[Dmitry Lomov (no reviews), 2015-03-24 14:51:46]

lgtm % comments

https://codereview.chromium.org/1005063002/diff/200001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1005063002/diff/200001/src/api.cc#newcode1503
src/api.cc:1503: i::ScriptContextTable::LookupResult result;
Maybe change order of lookup: lexicals trump global object, so check them first.
Does not matter atm.

https://codereview.chromium.org/1005063002/diff/200001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/1005063002/diff/200001/src/scopeinfo.cc#newco...
src/scopeinfo.cc:132: if (Smi::IsValid(start_position)) {
Factory::NewNumberFromInt does this.

[marja, 2015-03-24 16:09:12]

Patchset #15 (id:280001) has been deleted

[marja, 2015-03-24 16:45:17]

The CQ bit was checked by marja@chromium.org

[marja, 2015-03-24 16:45:18]

The patchset sent to the CQ was uploaded after l-g-t-m from dslomov@chromium.org
Link to the patchset: https://codereview.chromium.org/1005063002/#ps260001
(title: "arrow func param fix")

[commit-bot: I haz the power, 2015-03-24 16:45:32]

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

[marja, 2015-03-24 16:46:15]

Thanks for review.

I had to fix arrow func params a bit (see the latest patch set) but it was
pretty trivial.

https://codereview.chromium.org/1005063002/diff/200001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1005063002/diff/200001/src/api.cc#newcode1503
src/api.cc:1503: i::ScriptContextTable::LookupResult result;
On 2015/03/24 14:51:46, Dmitry Lomov (chromium) wrote:
> Maybe change order of lookup: lexicals trump global object, so check them
first.
> Does not matter atm.

Done.

https://codereview.chromium.org/1005063002/diff/200001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/1005063002/diff/200001/src/scopeinfo.cc#newco...
src/scopeinfo.cc:132: if (Smi::IsValid(start_position)) {
On 2015/03/24 14:51:46, Dmitry Lomov (chromium) wrote:
> Factory::NewNumberFromInt does this.

Done.

[commit-bot: I haz the power, 2015-03-24 16:47:01]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-03-24 16:47:11]

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