Make sure that names of temporaries do not clash with real variables. (issue 11035054)

[ross...@chromium.org]

Reviewers: Michael Starzinger,

Description:
Make sure that names of temporaries do not clash with real variables.


R=mstar...@chromium.org
BUG=v8:2322


Please review this at https://codereview.chromium.org/11035054/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/parser.cc
M src/scopes.h
M test/mjsunit/regress/regress-2322.js


Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index
0326fc9996b6f8a1ed761e5e11ad614f35b2dbf9..c2fd015491bd0e117b1d0bc86ac87f8601f1be11
100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -2842,7 +2842,12 @@ Statement* Parser::ParseForStatement(ZoneStringList*
labels, bool* ok) {

// TODO(keuchel): Move the temporary variable to the block scope,
after
// implementing stack allocated block scoped variables.
- Variable* temp =
top_scope_->DeclarationScope()->NewTemporary(name);
+ Factory* heap_factory = isolate()->factory();
+ Handle<String> dot =
+ heap_factory->NewStringFromAscii(CStrVector(".for."));
+ Handle<String> tempstr = heap_factory->NewConsString(dot, name);
+ Handle<String> tempname = heap_factory->LookupSymbol(tempstr);
+ Variable* temp =
top_scope_->DeclarationScope()->NewTemporary(tempname);
VariableProxy* temp_proxy = factory()->NewVariableProxy(temp);
ForInStatement* loop = factory()->NewForInStatement(labels);
Target target(&this->target_stack_, loop);
Index: src/scopes.h
diff --git a/src/scopes.h b/src/scopes.h
index
6e35d05a7f5d07f3b9941b8489006fed3aa03530..b9d151cba5d69af23793c93e8e54da103fc825c1
100644
--- a/src/scopes.h
+++ b/src/scopes.h
@@ -189,7 +189,7 @@ class Scope: public ZoneObject {
// Creates a new temporary variable in this scope. The name is only used
// for printing and cannot be used to find the variable. In particular,
// the only way to get hold of the temporary is by keeping the Variable*
- // around.
+ // around. The name should not clash with a legitimate variable names.
Variable* NewTemporary(Handle<String> name);

// Adds the specific declaration node to the list of declarations in
Index: test/mjsunit/regress/regress-2322.js
diff --git a/test/mjsunit/regress/regress-2322.js
b/test/mjsunit/regress/regress-2322.js
index
a60af06148e4aacd8e3d442234aceaf03554bced..1195bab67cb6b9123e93c3bd32135e972799e876
100644
--- a/test/mjsunit/regress/regress-2322.js
+++ b/test/mjsunit/regress/regress-2322.js
@@ -27,5 +27,10 @@

// Flags: --harmony-scoping

+"use strict";
+
assertThrows("'use strict'; for (let x in x);", ReferenceError);

+let s;
+for (let pppp in {}) {};
+assertThrows(function() { pppp = true }, ReferenceError);

[mstar...@chromium.org]

LGTM (with comments).


https://codereview.chromium.org/11035054/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/11035054/diff/1/src/parser.cc#newcode2847
src/parser.cc:2847:
heap_factory->NewStringFromAscii(CStrVector(".for."));
You could move this symbol into the root-set by adding it to
SYMBOL_LIST, call it e.g. dot_for_symbol and then access it through
factory->dot_for_symbol().

Also does the name really need to be different for every temporary, or
could we use the one symbol for all temporaries?

https://codereview.chromium.org/11035054/

[ross...@chromium.org]

https://codereview.chromium.org/11035054/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/11035054/diff/1/src/parser.cc#newcode2847
src/parser.cc:2847:
heap_factory->NewStringFromAscii(CStrVector(".for."));

On 2012/10/05 12:31:38, Michael Starzinger wrote:
> You could move this symbol into the root-set by adding it to
SYMBOL_LIST, call
> it e.g. dot_for_symbol and then access it through
factory->dot_for_symbol().

Done.

> Also does the name really need to be different for every temporary, or
could we
> use the one symbol for all temporaries?

There can be several loop variables in the same scope, so they should be
distinguishable.

https://codereview.chromium.org/11035054/