[parser] Remove hoist_scope from DeclarationDescriptor

src/parsing/pattern-rewriter.cc

 // 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.
 
 #include "src/ast/ast.h"
 #include "src/messages.h"
 #include "src/objects-inl.h"
 #include "src/parsing/parameter-initializer-rewriter.h"
 #include "src/parsing/parser.h"
 
@@ skipping 119 matching lines @@
   // when the variable is encountered in the source. But the variable/constant
   // is declared (and set to 'undefined') upon entering the function within
   // which the variable or constant is declared. Only function variables have
   // an initial value in the declaration (because they are initialized upon
   // entering the function).
   const AstRawString* name = pattern->raw_name();
   VariableProxy* proxy =
       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;
     return;
   }
   if (names_) {
     names_->Add(name, zone());
   }
 
   // If there's no initializer, we're done.
@@ skipping 148 matching lines @@
   Expression* expr = factory()->NewDoExpression(block_, temp, pos);
   node->Rewrite(expr);
   block_ = old_block;
   if (block_) {
     block_->statements()->Add(factory()->NewExpressionStatement(expr, pos),
                               zone());
   }
   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,
                                                  Variable** temp_var) {
   auto temp = *temp_var = CreateTempVar(current_value_);
 
   ZoneList<Expression*>* rest_runtime_callargs = nullptr;
   if (pattern->has_rest_property()) {
     // non_rest_properties_count = pattern->properties()->length - 1;
     // args_length = 1 + non_rest_properties_count because we need to
@@ skipping 398 matching lines @@
 NOT_A_PATTERN(TryFinallyStatement)
 NOT_A_PATTERN(UnaryOperation)
 NOT_A_PATTERN(VariableDeclaration)
 NOT_A_PATTERN(WhileStatement)
 NOT_A_PATTERN(WithStatement)
 NOT_A_PATTERN(Yield)
 
 #undef NOT_A_PATTERN
 }  // namespace internal
 }  // namespace v8