Static resolution of outer variables in eval code.

Static resolution of outer variables in eval code.

  
So far free variables references in eval code are not statically
resolved. For example in
    function foo() { var x = 1; eval("y = x"); }
the variable x will get mode DYNAMIC and y will get mode DYNAMIC_GLOBAL,
i.e. free variable references trigger dynamic lookups with a fast case
handling for global variables.

The CL introduces static resolution of free variables references in eval
code. If possible variable references are resolved to bindings belonging to
outer scopes of the eval call site.

This is achieved by deserializing the outer scope chain using
Scope::DeserializeScopeChain prior to parsing the eval code similar to lazy
parsing of functions. The existing code for variable resolution is used,
however resolution starts at the first outer unresolved scope instead of
always starting at the root of the scope tree.

This is a prerequisite for statically checking validity of assignments in
the extended code as specified by the current ES.next draft which will be
introduced by a subsequent CL. More specifically section 11.13 of revision 4
of the ES.next draft reads:
* It is a Syntax Error if the AssignmentExpression is contained in extended
  code and the LeftHandSideExpression is an Identifier that does not
  statically resolve to a declarative environment record binding or if the
  resolved binding is an immutable binding.

TEST=existing tests in mjsunit

Committed: http://code.google.com/p/v8/source/detail?r=9999

[Steven, 2011-11-10 11:44:19]

PTAL.

[Kevin Millikin (Chromium), 2011-11-10 12:55:20]

Drive by comment.

http://codereview.chromium.org/8508052/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8508052/diff/1/src/runtime.cc#newcode12232
src/runtime.cc:12232: // Create a function context first, than put 'with'
context on top of it.
This comment is wrong.

http://codereview.chromium.org/8508052/diff/1/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/8508052/diff/1/src/scopes.cc#newcode592
src/scopes.cc:592: DeserializeScopeChain(*calling_context, global_scope);
I don't like this.  It doesn't seem consistent.  For lazily-compiled code, we
deserialize right up front in the parser, so the parser always has the correct
scope chain.

I think of eval code as being the same as lazy compilation, so it seems better
to deserialize earlier: somewhere before Parser::DoParseProgram parses the
program body, inspect the compilation info to see if it's an eval call and
deserialize there.

[Steven, 2011-11-11 08:35:04]

Please take another look.

http://codereview.chromium.org/8508052/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8508052/diff/1/src/runtime.cc#newcode12232
src/runtime.cc:12232: // Create a function context first, than put 'with'
context on top of it.
On 2011/11/10 12:55:21, Kevin Millikin wrote:
> This comment is wrong.

Done.

http://codereview.chromium.org/8508052/diff/1/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/8508052/diff/1/src/scopes.cc#newcode592
src/scopes.cc:592: DeserializeScopeChain(*calling_context, global_scope);
On 2011/11/10 12:55:21, Kevin Millikin wrote:
> I don't like this.  It doesn't seem consistent.  For lazily-compiled code, we
> deserialize right up front in the parser, so the parser always has the correct
> scope chain.
> 
> I think of eval code as being the same as lazy compilation, so it seems better
> to deserialize earlier: somewhere before Parser::DoParseProgram parses the
> program body, inspect the compilation info to see if it's an eval call and
> deserialize there.

Done.

[fschneider, 2011-11-14 15:20:51]

I did not get through all the files yet. Here are my first comments.

Also I find the issue description not very informative. A commit message of a
non-trivial change should usually include something along these lines:

1. What is the problem this CL solves? (problem description)
2. How is the problem solved in this CL? (outline of the approach)
3. What is the benefit? (e.g. better performance on x, features y supported,
etc.)
4. BUG= and TEST= lines if applicable.

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc#newcode132
src/scopes.cc:132: ASSERT((type == GLOBAL_SCOPE) == (outer_scope == NULL));
Use ASSERT_EQ instead, so that you get a better error message if the assertion
fails:

ASSERT_EQ(type == GLOBAL_SCOPE, outer_scope == NULL);

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc#newcode269
src/scopes.cc:269: !top->outer_scope()->already_resolved()) {
This loop condition seems inconsistent with the comment.

[fschneider, 2011-11-15 10:13:56]

lgtm

[Steven, 2011-11-15 11:53:23]

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc#newcode132
src/scopes.cc:132: ASSERT((type == GLOBAL_SCOPE) == (outer_scope == NULL));
On 2011/11/14 15:20:52, fschneider wrote:
> Use ASSERT_EQ instead, so that you get a better error message if the assertion
> fails:
> 
> ASSERT_EQ(type == GLOBAL_SCOPE, outer_scope == NULL);

Done.

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc#newcode269
src/scopes.cc:269: !top->outer_scope()->already_resolved()) {
I've updated the comment with a more thorough description.
On 2011/11/14 15:20:52, fschneider wrote:
> This loop condition seems inconsistent with the comment.

[fschneider, 2011-11-15 12:06:45]

lgtm

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc#newcode269
src/scopes.cc:269: !top->outer_scope()->already_resolved()) {
On 2011/11/15 11:53:23, Steven wrote:
> I've updated the comment with a more thorough description.
> On 2011/11/14 15:20:52, fschneider wrote:
> > This loop condition seems inconsistent with the comment.
> 

Maybe say: Traverse until the first unresolved scope or the global scope.

[Steven, 2011-11-15 13:51:15]

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/8508052/diff/6020/src/scopes.cc#newcode269
src/scopes.cc:269: !top->outer_scope()->already_resolved()) {
On 2011/11/15 12:06:46, fschneider wrote:
> On 2011/11/15 11:53:23, Steven wrote:
> > I've updated the comment with a more thorough description.
> > On 2011/11/14 15:20:52, fschneider wrote:
> > > This loop condition seems inconsistent with the comment.
> > 
> 
> Maybe say: Traverse until the first unresolved scope or the global scope.

Done.