From 0406fa2237acb05af24a9e264d7d451f5901834f Mon Sep 17 00:00:00 2001 From: nikolaos Date: Fri, 8 Jan 2016 07:43:34 -0800 Subject: [PATCH] Fix for temporaries in parameter initializers This patch introduces a mechanism for changing the scope of temporary variables, which is necessary for rewriting arrow parameter initializers. It also fixes a potential bug in AstExpressionVisitor, which did not visit the automatically generated members of ForEachStatement. Fixes test/mjsunit/harmony/regress/regress-4658.js R=rossberg@chromium.org BUG=v8:4658 LOG=N Review URL: https://codereview.chromium.org/1564343002 Cr-Commit-Position: refs/heads/master@{#33183} --- src/ast/ast-expression-visitor.cc | 5 +++++ src/ast/scopes.cc | 15 ++++++++++++- src/ast/scopes.h | 9 ++++++++ src/ast/variables.h | 4 ++++ src/parsing/parameter-initializer-rewriter.cc | 10 +++++++-- test/mjsunit/harmony/regress/regress-4658.js | 21 +++++++++++++++++++ 6 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 test/mjsunit/harmony/regress/regress-4658.js diff --git a/src/ast/ast-expression-visitor.cc b/src/ast/ast-expression-visitor.cc index 6ee2d5988c..6b2550c541 100644 --- a/src/ast/ast-expression-visitor.cc +++ b/src/ast/ast-expression-visitor.cc @@ -171,6 +171,11 @@ void AstExpressionVisitor::VisitForInStatement(ForInStatement* stmt) { void AstExpressionVisitor::VisitForOfStatement(ForOfStatement* stmt) { RECURSE(Visit(stmt->iterable())); + RECURSE(Visit(stmt->each())); + RECURSE(Visit(stmt->assign_iterator())); + RECURSE(Visit(stmt->next_result())); + RECURSE(Visit(stmt->result_done())); + RECURSE(Visit(stmt->assign_each())); RECURSE(Visit(stmt->body())); } diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc index 21c9e2e373..c2b05b7c04 100644 --- a/src/ast/scopes.cc +++ b/src/ast/scopes.cc @@ -575,11 +575,24 @@ Variable* Scope::NewTemporary(const AstRawString* name) { TEMPORARY, Variable::NORMAL, kCreatedInitialized); - scope->temps_.Add(var, zone()); + scope->AddTemporary(var); return var; } +bool Scope::RemoveTemporary(Variable* var) { + // Most likely (always?) any temporary variable we want to remove + // was just added before, so we search backwards. + for (int i = temps_.length(); i-- > 0;) { + if (temps_[i] == var) { + temps_.Remove(i); + return true; + } + } + return false; +} + + void Scope::AddDeclaration(Declaration* declaration) { decls_.Add(declaration, zone()); } diff --git a/src/ast/scopes.h b/src/ast/scopes.h index 0e7d209d46..6c261f63c3 100644 --- a/src/ast/scopes.h +++ b/src/ast/scopes.h @@ -209,6 +209,15 @@ class Scope: public ZoneObject { // names. Variable* NewTemporary(const AstRawString* name); + // Remove a temporary variable. This is for adjusting the scope of + // temporaries used when desugaring parameter initializers. + bool RemoveTemporary(Variable* var); + + // Adds a temporary variable in this scope's TemporaryScope. This is for + // adjusting the scope of temporaries used when desugaring parameter + // initializers. + void AddTemporary(Variable* var) { temps_.Add(var, zone()); } + // Adds the specific declaration node to the list of declarations in // this scope. The declarations are processed as part of entering // the scope; see codegen.cc:ProcessDeclarations. diff --git a/src/ast/variables.h b/src/ast/variables.h index 4057d2b5f6..ca5d1cdd40 100644 --- a/src/ast/variables.h +++ b/src/ast/variables.h @@ -37,6 +37,10 @@ class Variable: public ZoneObject { // scope is only used to follow the context chain length. Scope* scope() const { return scope_; } + // This is for adjusting the scope of temporaries used when desugaring + // parameter initializers. + void set_scope(Scope* scope) { scope_ = scope; } + Handle name() const { return name_->string(); } const AstRawString* raw_name() const { return name_; } VariableMode mode() const { return mode_; } diff --git a/src/parsing/parameter-initializer-rewriter.cc b/src/parsing/parameter-initializer-rewriter.cc index eaf1889392..003bbebae0 100644 --- a/src/parsing/parameter-initializer-rewriter.cc +++ b/src/parsing/parameter-initializer-rewriter.cc @@ -60,8 +60,14 @@ void Rewriter::VisitClassLiteral(ClassLiteral* class_literal) { void Rewriter::VisitVariableProxy(VariableProxy* proxy) { - DCHECK(!proxy->is_resolved()); - if (old_scope_->RemoveUnresolved(proxy)) { + if (proxy->is_resolved()) { + Variable* var = proxy->var(); + DCHECK_EQ(var->mode(), TEMPORARY); + if (old_scope_->RemoveTemporary(var)) { + var->set_scope(new_scope_); + new_scope_->AddTemporary(var); + } + } else if (old_scope_->RemoveUnresolved(proxy)) { new_scope_->AddUnresolved(proxy); } } diff --git a/test/mjsunit/harmony/regress/regress-4658.js b/test/mjsunit/harmony/regress/regress-4658.js new file mode 100644 index 0000000000..35bea12adc --- /dev/null +++ b/test/mjsunit/harmony/regress/regress-4658.js @@ -0,0 +1,21 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --harmony-do-expressions + +(function testWithSimpleLoopVariable() { + var f = (x, y = (do { var s=0; for (var e of x) s += e; s; })) => y*(y+1); + var result = f([1,2,3]); // core dump here, if not fixed. + assertEquals(result, 42); +})(); + +(function testWithComplexLoopVariable() { + var f = (x, i=x[0]-1, a=[], + y = (do { var s=0; + for (a[i] of x) s += a[i++]; + s; + })) => y*(a[0]+a[1]*a[2]); + var result = f([1,2,3]); // core dump here, if not fixed. + assertEquals(result, 42); +})();