Reject uses of lexical for-loop variable on the RHS. (issue 11031045)

[ross...@chromium.org]

Reviewers: Michael Starzinger,

Description:
Reject uses of lexical for-loop variable on the RHS.

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


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

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

Affected files:
M src/parser.cc
A + test/mjsunit/regress/regress-2322.js


Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index
a626d99fa0a078cbaffb97deb3fb448e4d6c8dd2..061abb2b634f360423199c0aa634423cec463115
100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -2839,17 +2839,20 @@ Statement*
Parser::ParseForStatement(ZoneStringList* labels, bool* ok) {
Variable* temp =
top_scope_->DeclarationScope()->NewTemporary(name);
VariableProxy* temp_proxy = factory()->NewVariableProxy(temp);
Interface* interface = Interface::NewValue();
- VariableProxy* each =
- top_scope_->NewUnresolved(factory(), name, interface);
ForInStatement* loop = factory()->NewForInStatement(labels);
Target target(&this->target_stack_, loop);

+ // The expression does not see the loop variable.
Expect(Token::IN, CHECK_OK);
+ top_scope_ = saved_scope;
Expression* enumerable = ParseExpression(true, CHECK_OK);
+ top_scope_ = for_scope;
Expect(Token::RPAREN, CHECK_OK);

Statement* body = ParseStatement(NULL, CHECK_OK);
Block* body_block = factory()->NewBlock(NULL, 3, false);
+ VariableProxy* each =
+ top_scope_->NewUnresolved(factory(), name, interface);
Assignment* assignment = factory()->NewAssignment(
Token::ASSIGN, each, temp_proxy, RelocInfo::kNoPosition);
Statement* assignment_statement =
Index: test/mjsunit/regress/regress-2322.js
diff --git a/test/mjsunit/regress/regress-113924.js
b/test/mjsunit/regress/regress-2322.js
similarity index 95%
copy from test/mjsunit/regress/regress-113924.js
copy to test/mjsunit/regress/regress-2322.js
index
3ecdec48f219b9ea545702ebf3a396debe7a93f8..fd88d5e6424a98480baaf4a5a174a94871529ef9
100644
--- a/test/mjsunit/regress/regress-113924.js
+++ b/test/mjsunit/regress/regress-2322.js
@@ -25,7 +25,7 @@
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

-var count=12000;
-while(count--) {
- eval("var a = new Object(10); a[2] += 7;");
-}
+// Flags: --harmony-scoping
+
+assertThrows("for (let x in x);", SyntaxError);
+

[mstar...@chromium.org]

LGTM (with nits).


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

https://codereview.chromium.org/11031045/diff/1/src/parser.cc#newcode2794
src/parser.cc:2794: top_scope_->NewUnresolved(factory(), name,
interface);
For consistency we could also move this loop variable and interface
down.

https://codereview.chromium.org/11031045/diff/1/src/parser.cc#newcode2841
src/parser.cc:2841: Interface* interface = Interface::NewValue();
Can we also move the interface down right before the loop variable is
created?

https://codereview.chromium.org/11031045/diff/1/test/mjsunit/regress/regress-2322.js
File test/mjsunit/regress/regress-2322.js (right):

https://codereview.chromium.org/11031045/diff/1/test/mjsunit/regress/regress-2322.js#newcode30
test/mjsunit/regress/regress-2322.js:30: assertThrows("for (let x in
x);", SyntaxError);
You probably want to add 'use strict' to this test case, to make sure
that the original problem repros.

https://codereview.chromium.org/11031045/

[ross...@chromium.org]

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

https://codereview.chromium.org/11031045/diff/1/src/parser.cc#newcode2794
src/parser.cc:2794: top_scope_->NewUnresolved(factory(), name,
interface);

On 2012/10/05 08:42:36, Michael Starzinger wrote:
> For consistency we could also move this loop variable and interface
down.

Done.

https://codereview.chromium.org/11031045/diff/1/src/parser.cc#newcode2841
src/parser.cc:2841: Interface* interface = Interface::NewValue();

On 2012/10/05 08:42:36, Michael Starzinger wrote:
> Can we also move the interface down right before the loop variable is
created?

Inlined instead.

https://codereview.chromium.org/11031045/diff/1/test/mjsunit/regress/regress-2322.js
File test/mjsunit/regress/regress-2322.js (right):

https://codereview.chromium.org/11031045/diff/1/test/mjsunit/regress/regress-2322.js#newcode30
test/mjsunit/regress/regress-2322.js:30: assertThrows("for (let x in
x);", SyntaxError);

On 2012/10/05 08:42:36, Michael Starzinger wrote:
> You probably want to add 'use strict' to this test case, to make sure
that the
> original problem repros.

Indeed.

https://codereview.chromium.org/11031045/