Introduce with scope and rework variable resolution.

Introduce with scope and rework variable resolution.

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

[Steven, 2011-09-15 08:44:05]

PTAL.

[Kevin Millikin (Chromium), 2011-09-15 09:38:13]

I have a few comments.

It's great to clean this up.  The existing code is kind of opaque.

But I think this change subtly modifies the way we handle variables declared
with var/const/function in the global scope.  If I'm not wrong about the change,
we should convince ourselves that it's OK.

http://codereview.chromium.org/7904008/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7904008/diff/1/src/parser.cc#newcode2083
src/parser.cc:2083: Scope* saved_scope = top_scope_;
Now that we have three or four places using this pattern, we should implement a
C++ stack-allocated object that saves the scope.  Unfortunately, "LexicalScope"
is used for something not quite that.  And "ScopeScope" is to silly.  Something
like:

{ SaveScope scope(NewScope(top_scope_, Scope::WITH_SCOPE, true));
  Statement* stmt = ParseStatement(labels, CHECK_OK):
}

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

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode227
src/scopes.cc:227: Scope* with_scope = new Scope(WITH_SCOPE);
Make the constructor take the current scope and call AddInnerScope.  It's
strange to have one variant that requires it to be configured manually.  Then
you don't need the local with_scope.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode304
src/scopes.cc:304: if (is_catch_scope() || is_block_scope() || is_with_scope())
{
Flip the condition and use:

if (is_declaration_scope()) { ... } else { ... }

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode512
src/scopes.cc:512: for (Scope* scope = decl->scope(); cond ; scope =
scope->outer_scope_) {
I found the logic of this loop too complicated.  You could try something like:

Scope* previous = NULL;
Scope* current = decl->scope();
do {
  ... current ...
  previous = current;
  current = current->outer_scope_;
} while (!previous->is_declaration_scope());

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode831
src/scopes.cc:831: if (is_global_scope()) return GLOBAL;
return is_global_scope() ? GLOBAL : FOUND;

Though I actually think this should always be FOUND.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode891
src/scopes.cc:891: // If we are inside a local 'with' statement, all bets are
off
I lot of these comments are confusing and should be rewritten as you clean
things up.  This one mentions a local 'with' statement.  I'm not sure what that
means, but I would just say something like "If we passed a with statement on our
way to the binding scope....".

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode907
src/scopes.cc:907: // We did not find the variable. We have a global variable
Or we did find it, and it was in the global scope.  It's also a bit confusing
(warty) that we don't care about eval in the global scope because we'll just do
a global object property lookup in any case.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode915
src/scopes.cc:915: !(scope_inside_with_ || outer_scope_is_eval_scope_ ||
Is all this junk still necessary?  If we got GLOBAL, then we are definitely not
inside with (because that would be FOUND_WITH), and we don't have this scope
(unless it's the global scope) or any outer scope that calls eval (except
possibly the global scope) because that would be FOUND_EVAL.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode919
src/scopes.cc:919: var = global_scope->DeclareGlobal(proxy->name());
Is it right to (re)declare this variable even in the case that we found a
global-declared var/function/const already?  It doesn't seem like it matches the
old behavior.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode924
src/scopes.cc:924: ASSERT(false);
Just get rid of this case completely and assert !scope_inside_with_.

BTW, ASSERT(false) is spelled "UNREACHABLE()" in the V8 code.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode927
src/scopes.cc:927: } else if (outer_scope_is_eval_scope_) {
It would be great if we could get rid of this flag too, and even the whole
propagate scope info pass.

http://codereview.chromium.org/7904008/diff/1/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode214
src/scopes.h:214: Type type() const { return type_; }
Don't introduce this until you have a caller for it (in fact, can you make the
Type enum private?).

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode424
src/scopes.h:424: FOUND,
Since GLOBAL can also be found, perhaps it's clearer to rename FOUND => LOCAL
and likewise with FOUND_EVAL and FOUND_WITH.

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode436
src/scopes.h:436: FOUND_WITH,
On the other hand, this case includes the case of a variable that statically
resolves to a global var/function/const binding.  The comment should reflect
that, as well as the name.  Something like: LOCAL_PAST_EVAL, FOUND_PAST_WITH or
the like.

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode440
src/scopes.h:440: GLOBAL
This case is also used for when the variable is resolved to a variable in the
global scope, so the comment should include that.

[Steven, 2011-09-15 19:54:06]

http://codereview.chromium.org/7904008/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7904008/diff/1/src/parser.cc#newcode2083
src/parser.cc:2083: Scope* saved_scope = top_scope_;
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Now that we have three or four places using this pattern, we should implement
a
> C++ stack-allocated object that saves the scope.  Unfortunately,
"LexicalScope"
> is used for something not quite that.  And "ScopeScope" is to silly. 
Something
> like:
> 
> { SaveScope scope(NewScope(top_scope_, Scope::WITH_SCOPE, true));
>   Statement* stmt = ParseStatement(labels, CHECK_OK):
> }

Done.

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

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode227
src/scopes.cc:227: Scope* with_scope = new Scope(WITH_SCOPE);
There's already such a constructor that is used during parsing to construct the
scope tree and it takes the outer scope rather than an inner scope. I will reuse
the constructor for function and block scopes instead.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Make the constructor take the current scope and call AddInnerScope.  It's
> strange to have one variant that requires it to be configured manually.  Then
> you don't need the local with_scope.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode304
src/scopes.cc:304: if (is_catch_scope() || is_block_scope() || is_with_scope())
{
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Flip the condition and use:
> 
> if (is_declaration_scope()) { ... } else { ... }

Done.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode512
src/scopes.cc:512: for (Scope* scope = decl->scope(); cond ; scope =
scope->outer_scope_) {
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> I found the logic of this loop too complicated.  You could try something like:
> 
> Scope* previous = NULL;
> Scope* current = decl->scope();
> do {
>   ... current ...
>   previous = current;
>   current = current->outer_scope_;
> } while (!previous->is_declaration_scope());

Done.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode831
src/scopes.cc:831: if (is_global_scope()) return GLOBAL;
We need to distinguish locally bound variables from global variables. If we use
FOUND here for variables found in the global scope I would have to include
multiple checks elsewhere.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> return is_global_scope() ? GLOBAL : FOUND;
> 
> Though I actually think this should always be FOUND.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode891
src/scopes.cc:891: // If we are inside a local 'with' statement, all bets are
off
I cleaned them up. PTAL.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> I lot of these comments are confusing and should be rewritten as you clean
> things up.  This one mentions a local 'with' statement.  I'm not sure what
that
> means, but I would just say something like "If we passed a with statement on
our
> way to the binding scope....".

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode907
src/scopes.cc:907: // We did not find the variable. We have a global variable
Eval in the global scope can't introduce shadowed bindings, so in that case the
variable shouldn't be marked as being potentially shadowed. If a global variable
is not shadowed it will get the location Variable::UNALLOCATED which should
result in a fast object property lookup. Otherwise it will be Variable::LOOKUP
which will result in EmitLoadGlobalCheckExtensions emitting code to traverse the
context chain to check if any extension objects are present.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Or we did find it, and it was in the global scope.  It's also a bit confusing
> (warty) that we don't care about eval in the global scope because we'll just
do
> a global object property lookup in any case.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode915
src/scopes.cc:915: !(scope_inside_with_ || outer_scope_is_eval_scope_ ||
Not necessary anymore.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Is all this junk still necessary?  If we got GLOBAL, then we are definitely
not
> inside with (because that would be FOUND_WITH), and we don't have this scope
> (unless it's the global scope) or any outer scope that calls eval (except
> possibly the global scope) because that would be FOUND_EVAL.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode919
src/scopes.cc:919: var = global_scope->DeclareGlobal(proxy->name());
I think it matched the old behaviour, but now I avoid redeclaration.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Is it right to (re)declare this variable even in the case that we found a
> global-declared var/function/const already?  It doesn't seem like it matches
the
> old behavior.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode924
src/scopes.cc:924: ASSERT(false);
Sorry, this is dead code I forgot to delete.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Just get rid of this case completely and assert !scope_inside_with_.
> 
> BTW, ASSERT(false) is spelled "UNREACHABLE()" in the V8 code.

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode927
src/scopes.cc:927: } else if (outer_scope_is_eval_scope_) {
Got rid of the flag, but still need the propagate pass
for outer_scope_calls_non_strict_eval which is also used
elsewhere.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> It would be great if we could get rid of this flag too, and even the whole
> propagate scope info pass.

http://codereview.chromium.org/7904008/diff/1/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode214
src/scopes.h:214: Type type() const { return type_; }
Will remove it for for now. But I will need it later.

The enum values are used in the parser to create scopes, so I wouldn't want to
make it private now.
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Don't introduce this until you have a caller for it (in fact, can you make the
> Type enum private?).

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode424
src/scopes.h:424: FOUND,
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> Since GLOBAL can also be found, perhaps it's clearer to rename FOUND => LOCAL
> and likewise with FOUND_EVAL and FOUND_WITH.

Done.

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode436
src/scopes.h:436: FOUND_WITH,
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> On the other hand, this case includes the case of a variable that statically
> resolves to a global var/function/const binding.  The comment should reflect
> that, as well as the name.  Something like: LOCAL_PAST_EVAL, FOUND_PAST_WITH
or
> the like.

Done.

http://codereview.chromium.org/7904008/diff/1/src/scopes.h#newcode440
src/scopes.h:440: GLOBAL
On 2011/09/15 09:38:13, Kevin Millikin wrote:
> This case is also used for when the variable is resolved to a variable in the
> global scope, so the comment should include that.

Done.

[Kevin Millikin (Chromium), 2011-09-16 05:23:21]

I haven't taken a close look at the latest changes yet.

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

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode828
src/scopes.cc:828: (*var)->MarkAsAccessedFromInnerScope();
This is annoying to check here.  Isn't it possible to just have the caller mark
the result of the recursive call to LookupRecursive (and to not mark the result
of the initial call to LookupRecursive) and then skip passing the "I am a
recursive call" flag and having the callee do the marking?

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode831
src/scopes.cc:831: if (is_global_scope()) return GLOBAL;
That's what I'm asking about.

The old code will return a variable if there is a global var/function/const
binding.  Then the code in ResolveVariable will just bind to it.

You've changed it to return GLOBAL and set *var in that case, but then you check
the LookupResult flag first and ignore *var in the GLOBAL case.  So the new code
does something different in that case.

Is that necessary?  Harmless?

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode907
src/scopes.cc:907: // We did not find the variable. We have a global variable
Yes, I know.  Perhaps the names are a just bit off.  It's also a bit
confusing/error prone to sometimes return a vlaue in the out output parameter
var and not use it.

I suggest that the true alternatives from LookupRecursive are:

BOUND(var): There is a definitely not-shadowed binding (it may be global, but
that doesn't matter, I thiink)

EVAL_SHADOWED(var): There is a binding for the var but there is some scope
between the variable reference scope (inclusive) and binding scope (exclusive)
that calls eval (and there is no with between the reference and binding).

EVAL_SHADOWED(NULL): There is no binding and there is a scope that calls eval
between the var reference (inclusive) and the global scope (exclusive).

INSIDE_WITH(NULL): There is a with scope in the chain and no binding for the var
before that.

UNBOUND(NULL): There is no binding, no scope calls eval, we're not inside a
with.  In that case, we introduce an implicit global declaration for the
variable.

Does that sound about right?

[Steven, 2011-09-16 10:48:14]

PTAL again.

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

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode828
src/scopes.cc:828: (*var)->MarkAsAccessedFromInnerScope();
Sure, done.
On 2011/09/16 05:23:21, Kevin Millikin wrote:
> This is annoying to check here.  Isn't it possible to just have the caller
mark
> the result of the recursive call to LookupRecursive (and to not mark the
result
> of the initial call to LookupRecursive) and then skip passing the "I am a
> recursive call" flag and having the callee do the marking?

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode831
src/scopes.cc:831: if (is_global_scope()) return GLOBAL;
Yes, I'm sorry you're right. The old code only threw away the found binding if
shadowing might occur, but I threw it always away. But the second patchset
already does it correctly.
On 2011/09/16 05:23:21, Kevin Millikin wrote:
> That's what I'm asking about.
> 
> The old code will return a variable if there is a global var/function/const
> binding.  Then the code in ResolveVariable will just bind to it.
> 
> You've changed it to return GLOBAL and set *var in that case, but then you
check
> the LookupResult flag first and ignore *var in the GLOBAL case.  So the new
code
> does something different in that case.
> 
> Is that necessary?  Harmless?

http://codereview.chromium.org/7904008/diff/1/src/scopes.cc#newcode907
src/scopes.cc:907: // We did not find the variable. We have a global variable
For BOUND it does indeed not matter much if the variable is global or not, just
if a scope that calls eval is found we need to distinguish them, i.e. we need to
discriminate possibly shadowed globally bound variables from possibly shadowed
locally bound variables. But ok, this can be done via var->is_global() instead
of distinguishing between non-shadowed global/local bindings from the beginning.
On 2011/09/16 05:23:21, Kevin Millikin wrote:
> Yes, I know.  Perhaps the names are a just bit off.  It's also a bit
> confusing/error prone to sometimes return a vlaue in the out output parameter
> var and not use it.
> 
> I suggest that the true alternatives from LookupRecursive are:
> 
> BOUND(var): There is a definitely not-shadowed binding (it may be global, but
> that doesn't matter, I thiink)
> 
> EVAL_SHADOWED(var): There is a binding for the var but there is some scope
> between the variable reference scope (inclusive) and binding scope (exclusive)
> that calls eval (and there is no with between the reference and binding).
> 
> EVAL_SHADOWED(NULL): There is no binding and there is a scope that calls eval
> between the var reference (inclusive) and the global scope (exclusive).
> 
> INSIDE_WITH(NULL): There is a with scope in the chain and no binding for the
var
> before that.
> 
> UNBOUND(NULL): There is no binding, no scope calls eval, we're not inside a
> with.  In that case, we introduce an implicit global declaration for the
> variable.
> 
> Does that sound about right?

[Kevin Millikin (Chromium), 2011-09-16 12:58:19]

This is a very nice cleanup.  I have a few comments below.

You should take a quick look at Scope::LocalLookup and decide if it should
always or never check the function name.

Then it LGTM if you set *var to NULL in the cases that it is not used/needed.

http://codereview.chromium.org/7904008/diff/9011/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7904008/diff/9011/src/parser.cc#newcode467
src/parser.cc:467: // These scopes are not kept around after parsing or
referenced by syntax trees
Does the preparser still use these guys?  If not we should get rid of that part
of the comment.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode302
src/scopes.cc:302: ASSERT(is_function_scope() ||
I don't think this ASSERT is necessary any more.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode807
src/scopes.cc:807: *var = LocalLookup(name);
For some reason I prefer returning the variable and having the lookup kind be
the output parameter.  Just saying.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode823
src/scopes.cc:823: if (function_ != NULL &&
function_->name().is_identical_to(name)) {
I wonder if we could make Scope::LocalLookup check the function name after
checking the variable map, and get rid of this here.

It seems like it Scope::LocalLookup already does that for scopes backed by a
ScopeInfo.  That seems fishy, even if it's not an actual bug.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode838
src/scopes.cc:838: return DYNAMIC_LOOKUP;
And I'd prefer setting var to NULL (or returning NULL if you take the suggestion
above) in the cases where it's meaningless/should not be used/will not be used.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7904008/diff/9011/src/scopes.h#newcode417
src/scopes.h:417: enum LookupResult {
This sounds like a silly request, but: we already have a type LookupResult (for
property lookups) that's used extensively.  Since C++ tools generally stink, it
might be easiest to choose some different name so it's easy to find each of
them.

BindingKind?  Something else?

[Steven, 2011-09-16 18:29:51]

Will land this on Monday.

http://codereview.chromium.org/7904008/diff/9011/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7904008/diff/9011/src/parser.cc#newcode467
src/parser.cc:467: // These scopes are not kept around after parsing or
referenced by syntax trees
There is PreParser::Scope that remotely corresponds to LexicalScope, but these
classes are unrelated in terms of their implementation, so the preparser doesn't
need to be mentioned here.
Moreover the only important piece of information is the stack-allocation. I will
keep that and remove the rest.
On 2011/09/16 12:58:19, Kevin Millikin wrote:
> Does the preparser still use these guys?  If not we should get rid of that
part
> of the comment.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode302
src/scopes.cc:302: ASSERT(is_function_scope() ||
On 2011/09/16 12:58:19, Kevin Millikin wrote:
> I don't think this ASSERT is necessary any more.

Done.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode807
src/scopes.cc:807: *var = LocalLookup(name);
Yeah, after all me too.
On 2011/09/16 12:58:19, Kevin Millikin wrote:
> For some reason I prefer returning the variable and having the lookup kind be
> the output parameter.  Just saying.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode823
src/scopes.cc:823: if (function_ != NULL &&
function_->name().is_identical_to(name)) {
Unfortunately we can't. The function name is still in an 'implicit' outer scope
and might be shadowed by eval introduced variables, i.e. something like
function y( eval("var y = 1;"); return y; )
Thus the lookup for the y reference should produce BOUND_EVAL_SHADOWED. But if
we let LocalLookup do this check it will be BOUND.

Yes it is fishy that the function name of the SerializedScopeInfo is checked.
Removing it does not break any tests, but I left it in there for another cleanup
CL. It is for example used in another fishy function Scope::Lookup which in turn
is used in the parser to detect direct 'eval' calls.
On 2011/09/16 12:58:19, Kevin Millikin wrote:
> I wonder if we could make Scope::LocalLookup check the function name after
> checking the variable map, and get rid of this here.
> 
> It seems like it Scope::LocalLookup already does that for scopes backed by a
> ScopeInfo.  That seems fishy, even if it's not an actual bug.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.cc#newcode838
src/scopes.cc:838: return DYNAMIC_LOOKUP;
On 2011/09/16 12:58:19, Kevin Millikin wrote:
> And I'd prefer setting var to NULL (or returning NULL if you take the
suggestion
> above) in the cases where it's meaningless/should not be used/will not be
used.

Done.

http://codereview.chromium.org/7904008/diff/9011/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7904008/diff/9011/src/scopes.h#newcode417
src/scopes.h:417: enum LookupResult {
Used BindingKind for lack of a better name.
On 2011/09/16 12:58:19, Kevin Millikin wrote:
> This sounds like a silly request, but: we already have a type LookupResult
(for
> property lookups) that's used extensively.  Since C++ tools generally stink,
it
> might be easiest to choose some different name so it's easy to find each of
> them.
> 
> BindingKind?  Something else?