[ast] Save one pointer in most Function and Variable declaration node

Currently, Declaration stores a Scope pointer to whichever Scope the
declaration appeared in. This is used to disallow var declarations
being hoisted over lexical declarations. For example:

  {
    let x;
    { var x; }
  }

But in fact this is the only sort of case where storing the scope
is required: for lexical declarations (including function declarations
appearing in blocks), Declaration::scope() was always identical to
Declaration::proxy()->var()->scope(). That is, only var declarations
end up "nested" in this way.

This patch adds a subclass of VariableDeclaration to store the Scope.
Since the only thing that cares about that data is Scope analysis,
this isn't treated as a distinct AstNode::NodeType from VariableDeclaration,
leaving all AstVisitors untouched in the process.

Also reworked the logic in Scope::CheckConflictingVarDeclarations() for
clarity after making changes to accomodate the new code.

Change-Id: I6ee4298700508ab9e28a76ddb8504bae68bc473f
Reviewed-on: https://chromium-review.googlesource.com/619595
Commit-Queue: Adam Klein <adamk@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47441}

src/ast/ast.h

@@ -121,6 +121,7 @@ class BreakableStatement;
 class Expression;
 class IterationStatement;
 class MaterializedLiteral;
+class NestedVariableDeclaration;
 class ProducedPreParsedScopeData;
 class Statement;
 
@@ -418,31 +419,60 @@ class Declaration : public AstNode {
   typedef ThreadedList<Declaration> List;
 
   VariableProxy* proxy() const { return proxy_; }
-  Scope* scope() const { return scope_; }
 
  protected:
-  Declaration(VariableProxy* proxy, Scope* scope, int pos, NodeType type)
-      : AstNode(pos, type), proxy_(proxy), scope_(scope), next_(nullptr) {}
+  Declaration(VariableProxy* proxy, int pos, NodeType type)
+      : AstNode(pos, type), proxy_(proxy), next_(nullptr) {}
 
  private:
   VariableProxy* proxy_;
-  // Nested scope from which the declaration originated.
-  Scope* scope_;
   // Declarations list threaded through the declarations.
   Declaration** next() { return &next_; }
   Declaration* next_;
   friend List;
 };
 
+class VariableDeclaration : public Declaration {
+ public:
+  inline NestedVariableDeclaration* AsNested();
 
-class VariableDeclaration final : public Declaration {
  private:
   friend class AstNodeFactory;
 
-  VariableDeclaration(VariableProxy* proxy, Scope* scope, int pos)
-      : Declaration(proxy, scope, pos, kVariableDeclaration) {}
+  class IsNestedField
+      : public BitField<bool, Declaration::kNextBitFieldIndex, 1> {};
+
+ protected:
+  VariableDeclaration(VariableProxy* proxy, int pos, bool is_nested = false)
+      : Declaration(proxy, pos, kVariableDeclaration) {
+    bit_field_ = IsNestedField::update(bit_field_, is_nested);
+  }
+
+  static const uint8_t kNextBitFieldIndex = IsNestedField::kNext;
 };
 
+// For var declarations that appear in a block scope.
+// Only distinguished from VariableDeclaration during Scope analysis,
+// so it doesn't get its own NodeType.
+class NestedVariableDeclaration final : public VariableDeclaration {
+ public:
+  Scope* scope() const { return scope_; }
+
+ private:
+  friend class AstNodeFactory;
+
+  NestedVariableDeclaration(VariableProxy* proxy, Scope* scope, int pos)
+      : VariableDeclaration(proxy, pos, true), scope_(scope) {}
+
+  // Nested scope from which the declaration originated.
+  Scope* scope_;
+};
+
+inline NestedVariableDeclaration* VariableDeclaration::AsNested() {
+  return IsNestedField::decode(bit_field_)
+             ? static_cast<NestedVariableDeclaration*>(this)
+             : nullptr;
+}
 
 class FunctionDeclaration final : public Declaration {
  public:
@@ -452,9 +482,8 @@ class FunctionDeclaration final : public Declaration {
  private:
   friend class AstNodeFactory;
 
-  FunctionDeclaration(VariableProxy* proxy, FunctionLiteral* fun, Scope* scope,
-                      int pos)
-      : Declaration(proxy, scope, pos, kFunctionDeclaration), fun_(fun) {
+  FunctionDeclaration(VariableProxy* proxy, FunctionLiteral* fun, int pos)
+      : Declaration(proxy, pos, kFunctionDeclaration), fun_(fun) {
     DCHECK(fun != NULL);
   }
 
@@ -3105,15 +3134,19 @@ class AstNodeFactory final BASE_EMBEDDED {
 
   AstValueFactory* ast_value_factory() const { return ast_value_factory_; }
 
-  VariableDeclaration* NewVariableDeclaration(VariableProxy* proxy,
-                                              Scope* scope, int pos) {
-    return new (zone_) VariableDeclaration(proxy, scope, pos);
+  VariableDeclaration* NewVariableDeclaration(VariableProxy* proxy, int pos) {
+    return new (zone_) VariableDeclaration(proxy, pos);
+  }
+
+  NestedVariableDeclaration* NewNestedVariableDeclaration(VariableProxy* proxy,
+                                                          Scope* scope,
+                                                          int pos) {
+    return new (zone_) NestedVariableDeclaration(proxy, scope, pos);
   }
 
   FunctionDeclaration* NewFunctionDeclaration(VariableProxy* proxy,
-                                              FunctionLiteral* fun,
-                                              Scope* scope, int pos) {
-    return new (zone_) FunctionDeclaration(proxy, fun, scope, pos);
+                                              FunctionLiteral* fun, int pos) {
+    return new (zone_) FunctionDeclaration(proxy, fun, pos);
   }
 
   Block* NewBlock(ZoneList<const AstRawString*>* labels, int capacity,

src/ast/scopes.cc

@@ -602,7 +602,7 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
       DCHECK(!is_being_lazily_parsed_);
       VariableProxy* proxy = factory->NewVariableProxy(name, NORMAL_VARIABLE);
       auto declaration =
-          factory->NewVariableDeclaration(proxy, this, kNoSourcePosition);
+          factory->NewVariableDeclaration(proxy, kNoSourcePosition);
       // Based on the preceding checks, it doesn't matter what we pass as
       // allow_harmony_restrictive_generators and
       // sloppy_mode_block_scope_function_redefinition.
@@ -1289,28 +1289,36 @@ Variable* Scope::NewTemporary(const AstRawString* name,
 Declaration* Scope::CheckConflictingVarDeclarations() {
   for (Declaration* decl : decls_) {
     VariableMode mode = decl->proxy()->var()->mode();
-    if (IsLexicalVariableMode(mode) && !is_block_scope()) continue;
 
-    // Iterate through all scopes until and including the declaration scope.
-    Scope* previous = NULL;
-    Scope* current = decl->scope();
     // Lexical vs lexical conflicts within the same scope have already been
     // captured in Parser::Declare. The only conflicts we still need to check
-    // are lexical vs VAR, or any declarations within a declaration block scope
-    // vs lexical declarations in its surrounding (function) scope.
-    if (IsLexicalVariableMode(mode)) current = current->outer_scope_;
-    do {
+    // are lexical vs nested var, or any declarations within a declaration
+    // block scope vs lexical declarations in its surrounding (function) scope.
+    Scope* current = this;
+    if (decl->IsVariableDeclaration() &&
+        decl->AsVariableDeclaration()->AsNested() != nullptr) {
+      DCHECK_EQ(mode, VAR);
+      current = decl->AsVariableDeclaration()->AsNested()->scope();
+    } else if (IsLexicalVariableMode(mode)) {
+      if (!is_block_scope()) continue;
+      DCHECK(is_declaration_scope());
+      DCHECK_EQ(outer_scope()->scope_type(), FUNCTION_SCOPE);
+      current = outer_scope();
+    }
+
+    // Iterate through all scopes until and including the declaration scope.
+    while (true) {
       // There is a conflict if there exists a non-VAR binding.
       Variable* other_var =
           current->variables_.Lookup(decl->proxy()->raw_name());
-      if (other_var != NULL && IsLexicalVariableMode(other_var->mode())) {
+      if (other_var != nullptr && IsLexicalVariableMode(other_var->mode())) {
         return decl;
       }
-      previous = current;
-      current = current->outer_scope_;
-    } while (!previous->is_declaration_scope());
+      if (current->is_declaration_scope()) break;
+      current = current->outer_scope();
+    }
   }
-  return NULL;
+  return nullptr;
 }
 
 Declaration* Scope::CheckLexDeclarationsConflictingWith(

src/parsing/parser.cc

@@ -1429,8 +1429,13 @@ Declaration* Parser::DeclareVariable(const AstRawString* name,
   DCHECK_NOT_NULL(name);
   VariableProxy* proxy = factory()->NewVariableProxy(
       name, NORMAL_VARIABLE, scanner()->location().beg_pos);
-  Declaration* declaration =
-      factory()->NewVariableDeclaration(proxy, this->scope(), pos);
+  Declaration* declaration;
+  if (mode == VAR && !scope()->is_declaration_scope()) {
+    DCHECK(scope()->is_block_scope() || scope()->is_with_scope());
+    declaration = factory()->NewNestedVariableDeclaration(proxy, scope(), pos);
+  } else {
+    declaration = factory()->NewVariableDeclaration(proxy, pos);
+  }
   Declare(declaration, DeclarationDescriptor::NORMAL, mode, init, ok, nullptr,
           scanner()->location().end_pos);
   if (!*ok) return nullptr;
@@ -1491,7 +1496,7 @@ Statement* Parser::DeclareFunction(const AstRawString* variable_name,
       factory()->NewVariableProxy(variable_name, NORMAL_VARIABLE);
 
   Declaration* declaration =
-      factory()->NewFunctionDeclaration(proxy, function, scope(), pos);
+      factory()->NewFunctionDeclaration(proxy, function, pos);
   Declare(declaration, DeclarationDescriptor::NORMAL, mode, kCreatedInitialized,
           CHECK_OK);
   if (names) names->Add(variable_name, zone());
@@ -3234,8 +3239,8 @@ void Parser::DeclareClassVariable(const AstRawString* name,
 
   if (name != nullptr) {
     class_info->proxy = factory()->NewVariableProxy(name, NORMAL_VARIABLE);
-    Declaration* declaration = factory()->NewVariableDeclaration(
-        class_info->proxy, scope(), class_token_pos);
+    Declaration* declaration =
+        factory()->NewVariableDeclaration(class_info->proxy, class_token_pos);
     Declare(declaration, DeclarationDescriptor::NORMAL, CONST,
             Variable::DefaultInitializationFlag(CONST), ok);
   }

src/parsing/pattern-rewriter.cc

@@ -195,8 +195,16 @@ void PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
   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);
+  Declaration* declaration;
+  if (descriptor_->mode == VAR && !descriptor_->scope->is_declaration_scope()) {
+    DCHECK(descriptor_->scope->is_block_scope() ||
+           descriptor_->scope->is_with_scope());
+    declaration = factory()->NewNestedVariableDeclaration(
+        proxy, descriptor_->scope, descriptor_->declaration_pos);
+  } else {
+    declaration =
+        factory()->NewVariableDeclaration(proxy, 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

test/message/var-conflict-in-with.js

@@ -0,0 +1,5 @@
+// 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.
+
+{ { with ({}) var x; } let x; }

test/message/var-conflict-in-with.out

@@ -0,0 +1,4 @@
+*%(basename)s:5: SyntaxError: Identifier 'x' has already been declared
+{ { with ({}) var x; } let x; }
+                  ^
+SyntaxError: Identifier 'x' has already been declared