[parser] Remove hoist_scope from DeclarationDescriptor

src/parsing/pattern-rewriter.cc

Index: src/parsing/pattern-rewriter.cc
diff --git a/src/parsing/pattern-rewriter.cc b/src/parsing/pattern-rewriter.cc
index 7f38053e549c6cd7f5eb673b88e5b7bc10739fc9..9ed69646ef5a229929eade51f31ec49991a8c2ed 100644
--- a/src/parsing/pattern-rewriter.cc
+++ b/src/parsing/pattern-rewriter.cc
@@ -137,22 +137,31 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
       factory()->NewVariableProxy(name, NORMAL_VARIABLE, pattern->position());
   Declaration* declaration = factory()->NewVariableDeclaration(
       proxy, descriptor_->scope, descriptor_->declaration_pos);
+
+  // When an extra declaration scope needs to be inserted to account for
+  // a sloppy eval in a default parameter or function body, the parameter
+  // needs to be declared in the function's scope, not in the varblock
+  // scope which will be used for the initializer expression.
+  Scope* outer_function_scope = nullptr;
+  if (DeclaresParameterContainingSloppyEval()) {
+    outer_function_scope = descriptor_->scope->outer_scope();

[neis, 2017/02/01 08:55:24]

Can you add a DCHECK here saying that it's a function scope?

[adamk, 2017/02/01 16:22:15]

I could add another one, but it does feel kind of redundant. Let me know what
you think of my suggestion below.

+  }
   Variable* var = parser_->Declare(
       declaration, descriptor_->declaration_kind, descriptor_->mode,
       Variable::DefaultInitializationFlag(descriptor_->mode), ok_,
-      descriptor_->hoist_scope);
+      outer_function_scope);
   if (!*ok_) return;
   DCHECK_NOT_NULL(var);
   DCHECK(proxy->is_resolved());
   DCHECK(initializer_position_ != kNoSourcePosition);
   var->set_initializer_position(initializer_position_);
 
-  // TODO(adamk): This should probably be checking hoist_scope.
-  // Move it to Parser::Declare() to make it easier to test
-  // the right scope.
-  Scope* declaration_scope = IsLexicalVariableMode(descriptor_->mode)
-                                 ? descriptor_->scope
-                                 : descriptor_->scope->GetDeclarationScope();
+  Scope* declaration_scope =
+      outer_function_scope != nullptr
+          ? outer_function_scope
+          : (IsLexicalVariableMode(descriptor_->mode)
+                 ? descriptor_->scope
+                 : descriptor_->scope->GetDeclarationScope());
   if (declaration_scope->num_var() > kMaxNumFunctionLocals) {
     parser_->ReportMessage(MessageTemplate::kTooManyVariables);
     *ok_ = false;
@@ -321,20 +330,31 @@ void Parser::PatternRewriter::VisitRewritableExpression(
   set_context(old_context);
 }
 
+bool Parser::PatternRewriter::DeclaresParameterContainingSloppyEval() const {
+  // Need to check for a binding context to make sure we have a descriptor.
+  if (IsBindingContext() &&
+      // Only relevant for parameters.
+      descriptor_->declaration_kind == DeclarationDescriptor::PARAMETER &&
+      // And only when scope is a block scope;
+      // without eval, it is a function scope.
+      scope()->is_block_scope()) {
+    DCHECK(scope()->calls_sloppy_eval());
+    DCHECK(scope()->is_declaration_scope());
+    DCHECK(scope()->outer_scope()->is_function_scope());

[neis, 2017/02/01 08:55:24]

Hmm I guess it's already here because scope() is always descriptor_->scope?

[adamk, 2017/02/01 16:22:15]

Another option would be to call this function something like
GetScopeForParameterContainingEval() and have it return the scope.

[neis, 2017/02/01 16:26:41]

I prefer the current version. It's okay with me if you don't add the DCHECK.

+    return true;
+  }
+
+  return false;
+}
+
 // When an extra declaration scope needs to be inserted to account for
 // a sloppy eval in a default parameter or function body, the expressions
 // needs to be in that new inner scope which was added after initial
 // parsing.
 void Parser::PatternRewriter::RewriteParameterScopes(Expression* expr) {
-  if (!IsBindingContext()) return;
-  if (descriptor_->declaration_kind != DeclarationDescriptor::PARAMETER) return;
-  if (!scope()->is_block_scope()) return;
-
-  DCHECK(scope()->is_declaration_scope());
-  DCHECK(scope()->outer_scope()->is_function_scope());
-  DCHECK(scope()->calls_sloppy_eval());
-
-  ReparentParameterExpressionScope(parser_->stack_limit(), expr, scope());
+  if (DeclaresParameterContainingSloppyEval()) {
+    ReparentParameterExpressionScope(parser_->stack_limit(), expr, scope());
+  }
 }
 
 void Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* pattern,