[parser] Don't create proxies for vars without initialisers

Vars without initialisers don't need to allocate a VariableProxy, as the
proxy expression is not really needed for anything. So, we can special
case declaration parsing to look ahead for a '=' (plus a few other
cases), and skip the variable proxy allocation if it isn't there.

As a side-effect, variables that are only declared but never used are
no longer marked is_used, and thus not allocated. This saves on
generating dead code.

Change-Id: Ie4f04c6b5c1138df4c2e17acf1f0150459b3b571
Reviewed-on: https://chromium-review.googlesource.com/c/1434376
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59129}

src/ast/ast.h

@@ -3165,6 +3165,8 @@ class AstNodeFactory final {
                             Expression* value,
                             int pos) {
     DCHECK(Token::IsAssignmentOp(op));
+    DCHECK_NOT_NULL(target);
+    DCHECK_NOT_NULL(value);
 
     if (op != Token::INIT && target->IsVariableProxy()) {
       target->AsVariableProxy()->set_is_assigned();

src/parsing/expression-scope.h

@@ -49,14 +49,27 @@ class ExpressionScope {
     VariableProxy* result = parser_->NewRawVariable(name, pos);
     if (CanBeExpression()) {
       AsExpressionParsingScope()->TrackVariable(result);
-    } else if (type_ == kParameterDeclaration) {
-      AsParameterDeclarationParsingScope()->Declare(result);
     } else {
-      return AsVariableDeclarationParsingScope()->Declare(result);
+      Variable* var = Declare(name, pos);
+      if (IsVarDeclaration() && !parser()->scope()->is_declaration_scope()) {
+        // Make sure we'll properly resolve the variable since we might be in a
+        // with or catch scope. In those cases the proxy isn't guaranteed to
+        // refer to the declared variable, so consider it unresolved.
+        parser()->scope()->AddUnresolved(result);
+      } else if (var) {
+        result->BindTo(var);
+      }
     }
     return result;
   }
 
+  Variable* Declare(const AstRawString* name, int pos = kNoSourcePosition) {
+    if (type_ == kParameterDeclaration) {
+      return AsParameterDeclarationParsingScope()->Declare(name, pos);
+    }
+    return AsVariableDeclarationParsingScope()->Declare(name, pos);
+  }
+
   void MarkIdentifierAsAssigned() {
     if (!CanBeExpression()) return;
     AsExpressionParsingScope()->MarkIdentifierAsAssigned();
@@ -214,6 +227,7 @@ class ExpressionScope {
   bool IsAsyncArrowHeadParsingScope() const {
     return type_ == kMaybeAsyncArrowParameterDeclaration;
   }
+  bool IsVarDeclaration() const { return type_ == kVarDeclaration; }
 
  private:
   friend class AccumulationScope<Types>;
@@ -272,21 +286,22 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
         mode_(mode),
         names_(names) {}
 
-  VariableProxy* Declare(VariableProxy* proxy) {
+  Variable* Declare(const AstRawString* name, int pos) {
     VariableKind kind = NORMAL_VARIABLE;
     bool was_added;
-    this->parser()->DeclareAndBindVariable(
-        proxy, kind, mode_, Variable::DefaultInitializationFlag(mode_),
-        this->parser()->scope(), &was_added, proxy->position());
+    Variable* var = this->parser()->DeclareVariable(
+        name, kind, mode_, Variable::DefaultInitializationFlag(mode_),
+        this->parser()->scope(), &was_added, pos);
     if (was_added &&
         this->parser()->scope()->num_var() > kMaxNumFunctionLocals) {
       this->parser()->ReportMessage(MessageTemplate::kTooManyVariables);
     }
-    if (names_) names_->Add(proxy->raw_name(), this->parser()->zone());
+    if (names_) names_->Add(name, this->parser()->zone());
     if (this->IsLexicalDeclaration()) {
-      if (this->parser()->IsLet(proxy->raw_name())) {
-        this->parser()->ReportMessageAt(proxy->location(),
-                                        MessageTemplate::kLetInLexicalBinding);
+      if (this->parser()->IsLet(name)) {
+        this->parser()->ReportMessageAt(
+            Scanner::Location(pos, pos + name->length()),
+            MessageTemplate::kLetInLexicalBinding);
       }
     } else {
       if (this->parser()->loop_nesting_depth() > 0) {
@@ -307,18 +322,12 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
         //
         // This also handles marking of loop variables in for-in and for-of
         // loops, as determined by loop-nesting-depth.
-        proxy->set_is_assigned();
-      }
-
-      // Make sure we'll properly resolve the variable since we might be in a
-      // with or catch scope. In those cases the assignment isn't guaranteed to
-      // write to the variable declared above.
-      if (!this->parser()->scope()->is_declaration_scope()) {
-        proxy =
-            this->parser()->NewUnresolved(proxy->raw_name(), proxy->position());
+        if (V8_LIKELY(var)) {
+          var->set_maybe_assigned();
+        }
       }
     }
-    return proxy;
+    return var;
   }
 
  private:
@@ -343,16 +352,17 @@ class ParameterDeclarationParsingScope : public ExpressionScope<Types> {
   explicit ParameterDeclarationParsingScope(ParserT* parser)
       : ExpressionScopeT(parser, ExpressionScopeT::kParameterDeclaration) {}
 
-  void Declare(VariableProxy* proxy) {
+  Variable* Declare(const AstRawString* name, int pos) {
     VariableKind kind = PARAMETER_VARIABLE;
     VariableMode mode = VariableMode::kVar;
     bool was_added;
-    this->parser()->DeclareAndBindVariable(
-        proxy, kind, mode, Variable::DefaultInitializationFlag(mode),
-        this->parser()->scope(), &was_added, proxy->position());
+    Variable* var = this->parser()->DeclareVariable(
+        name, kind, mode, Variable::DefaultInitializationFlag(mode),
+        this->parser()->scope(), &was_added, pos);
     if (!has_duplicate() && !was_added) {
-      duplicate_loc_ = proxy->location();
+      duplicate_loc_ = Scanner::Location(pos, pos + name->length());
     }
+    return var;
   }
 
   bool has_duplicate() const { return duplicate_loc_.IsValid(); }

src/parsing/parser-base.h

@@ -482,7 +482,9 @@ class ParserBase {
   struct DeclarationParsingResult {
     struct Declaration {
       Declaration(ExpressionT pattern, ExpressionT initializer)
-          : pattern(pattern), initializer(initializer) {}
+          : pattern(pattern), initializer(initializer) {
+        DCHECK_IMPLIES(Impl::IsNull(pattern), Impl::IsNull(initializer));
+      }
 
       ExpressionT pattern;
       ExpressionT initializer;
@@ -3515,13 +3517,42 @@ void ParserBase<Impl>::ParseVariableDeclarations(
     FuncNameInferrerState fni_state(&fni_);
 
     int decl_pos = peek_position();
-    ExpressionT pattern = ParseBindingPattern();
+
+    IdentifierT name;
+    ExpressionT pattern;
+    // Check for an identifier first, so that we can elide the pattern in cases
+    // where there is no initializer (and so no proxy needs to be created).
+    if (V8_LIKELY(Token::IsAnyIdentifier(peek()))) {
+      name = ParseAndClassifyIdentifier(Next());
+      if (V8_UNLIKELY(is_strict(language_mode()) &&
+                      impl()->IsEvalOrArguments(name))) {
+        impl()->ReportMessageAt(scanner()->location(),
+                                MessageTemplate::kStrictEvalArguments);
+        return;
+      }
+      if (peek() == Token::ASSIGN ||
+          (var_context == kForStatement && PeekInOrOf()) ||
+          parsing_result->descriptor.mode == VariableMode::kLet) {
+        // Assignments need the variable expression for the assignment LHS, and
+        // for of/in will need it later, so create the expression now.
+        pattern = impl()->ExpressionFromIdentifier(name, decl_pos);
+      } else {
+        // Otherwise, elide the variable expression and just declare it.
+        impl()->DeclareIdentifier(name, decl_pos);
+        pattern = impl()->NullExpression();
+      }
+    } else {
+      name = impl()->NullIdentifier();
+      pattern = ParseBindingPattern();
+      DCHECK(!impl()->IsIdentifier(pattern));
+    }
 
     Scanner::Location variable_loc = scanner()->location();
 
     ExpressionT value = impl()->NullExpression();
     int value_beg_pos = kNoSourcePosition;
     if (Check(Token::ASSIGN)) {
+      DCHECK(!impl()->IsNull(pattern));
       {
         value_beg_pos = peek_position();
         AcceptINScope scope(this, var_context != kForStatement);
@@ -3544,14 +3575,35 @@ void ParserBase<Impl>::ParseVariableDeclarations(
 
       impl()->SetFunctionNameFromIdentifierRef(value, pattern);
     } else {
+#ifdef DEBUG
+      // We can fall through into here on error paths, so don't DCHECK those.
+      if (!has_error()) {
+        // We should never get identifier patterns for the non-initializer path,
+        // as those expressions should be elided.
+        DCHECK_EQ(!impl()->IsNull(name),
+                  Token::IsAnyIdentifier(scanner()->current_token()));
+        DCHECK_IMPLIES(impl()->IsNull(pattern), !impl()->IsNull(name));
+        // The only times we have a non-null pattern are:
+        //   1. This is a destructuring declaration (with no initializer, which
+        //      is immediately an error),
+        //   2. This is a declaration in a for in/of loop, or
+        //   3. This is a let (which has an implicit undefined initializer)
+        DCHECK_IMPLIES(
+            !impl()->IsNull(pattern),
+            !impl()->IsIdentifier(pattern) ||
+                (var_context == kForStatement && PeekInOrOf()) ||
+                parsing_result->descriptor.mode == VariableMode::kLet);
+      }
+#endif
+
       if (var_context != kForStatement || !PeekInOrOf()) {
         // ES6 'const' and binding patterns require initializers.
         if (parsing_result->descriptor.mode == VariableMode::kConst ||
-            !impl()->IsIdentifier(pattern)) {
+            impl()->IsNull(name)) {
           impl()->ReportMessageAt(
               Scanner::Location(decl_pos, end_position()),
               MessageTemplate::kDeclarationMissingInitializer,
-              !impl()->IsIdentifier(pattern) ? "destructuring" : "const");
+              impl()->IsNull(name) ? "destructuring" : "const");
           return;
         }
         // 'let x' initializes 'x' to undefined.
@@ -3567,8 +3619,14 @@ void ParserBase<Impl>::ParseVariableDeclarations(
       declaration_it->var()->set_initializer_position(initializer_position);
     }
 
+    // Patterns should be elided iff. they don't have an initializer.
+    DCHECK_IMPLIES(impl()->IsNull(pattern),
+                   impl()->IsNull(value) ||
+                       (var_context == kForStatement && PeekInOrOf()));
+
     typename DeclarationParsingResult::Declaration decl(pattern, value);
     decl.value_beg_pos = value_beg_pos;
+
     parsing_result->declarations.push_back(decl);
   } while (Check(Token::COMMA));
 

src/parsing/parser.cc

@@ -1817,7 +1817,7 @@ Block* Parser::RewriteForVarInLegacy(const ForInfo& for_info) {
   const DeclarationParsingResult::Declaration& decl =
       for_info.parsing_result.declarations[0];
   if (!IsLexicalVariableMode(for_info.parsing_result.descriptor.mode) &&
-      decl.pattern->IsVariableProxy() && decl.initializer != nullptr) {
+      decl.initializer != nullptr && decl.pattern->IsVariableProxy()) {
     ++use_counts_[v8::Isolate::kForInInitializer];
     const AstRawString* name = decl.pattern->AsVariableProxy()->raw_name();
     VariableProxy* single_var = NewUnresolved(name);
@@ -1855,6 +1855,7 @@ void Parser::DesugarBindingInForEachStatement(ForInfo* for_info,
       for_info->parsing_result.declarations[0];
   Variable* temp = NewTemporary(ast_value_factory()->dot_for_string());
   ScopedPtrList<Statement> each_initialization_statements(pointer_buffer());
+  DCHECK_NOT_NULL(decl.pattern);
   decl.initializer = factory()->NewVariableProxy(temp, for_info->position);
   InitializeVariables(&each_initialization_statements, NORMAL_VARIABLE, &decl);
 

src/parsing/parser.h

@@ -818,6 +818,11 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     return expression_scope()->NewVariable(name, start_position);
   }
 
+  V8_INLINE void DeclareIdentifier(const AstRawString* name,
+                                   int start_position) {
+    expression_scope()->Declare(name, start_position);
+  }
+
   V8_INLINE Variable* DeclareCatchVariableName(Scope* scope,
                                                const AstRawString* name) {
     return scope->DeclareCatchVariableName(name);

src/parsing/preparser.h

@@ -1089,10 +1089,10 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserStatement::Default();
   }
 
-  void DeclareVariable(const AstRawString* name, VariableKind kind,
-                       VariableMode mode, InitializationFlag init, Scope* scope,
-                       bool* was_added, int position) {
-    DeclareVariableName(name, mode, scope, was_added, position, kind);
+  Variable* DeclareVariable(const AstRawString* name, VariableKind kind,
+                            VariableMode mode, InitializationFlag init,
+                            Scope* scope, bool* was_added, int position) {
+    return DeclareVariableName(name, mode, scope, was_added, position, kind);
   }
 
   void DeclareAndBindVariable(const VariableProxy* proxy, VariableKind kind,
@@ -1590,6 +1590,13 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserExpression::FromIdentifier(name);
   }
 
+  V8_INLINE void DeclareIdentifier(const PreParserIdentifier& name,
+                                   int start_position) {
+    if (name.string_ != nullptr) {
+      expression_scope()->Declare(name.string_, start_position);
+    }
+  }
+
   V8_INLINE Variable* DeclareCatchVariableName(
       Scope* scope, const PreParserIdentifier& identifier) {
     return scope->DeclareCatchVariableName(identifier.string_);

test/cctest/interpreter/test-bytecode-generator.cc

@@ -2420,7 +2420,7 @@ TEST(WideRegisters) {
   // Prepare prologue that creates frame for lots of registers.
   std::ostringstream os;
   for (size_t i = 0; i < 157; ++i) {
-    os << "var x" << i << ";\n";
+    os << "var x" << i << " = 0;\n";
   }
   std::string prologue(os.str());
 

test/cctest/test-parsing.cc

@@ -3473,7 +3473,6 @@ TEST(InnerAssignment) {
         i::Variable* var = scope->LookupForTesting(var_name);
         bool expected = outers[i].assigned || inners[j].assigned;
         CHECK_NOT_NULL(var);
-        CHECK(var->is_used() || !expected);
         bool is_maybe_assigned = var->maybe_assigned() == i::kMaybeAssigned;
         CHECK(is_maybe_assigned == expected ||
               (is_maybe_assigned && inners[j].allow_error_in_inner_function));

test/debugger/debug/regress/regress-1170187.js

@@ -73,7 +73,13 @@ Debug.setListener(listener);
 
 // Call a function with local variables passing a different number parameters
 // that the number of arguments.
-(function(x,y){var a,b,c; debugger; return 3})()
+(function(x,y){
+  var a,b,c;
+  // Make sure a, b, and c are used.
+  a,b,c;
+  debugger;
+  return 3
+})()
 
 // Make sure that the debug event listener vas invoked (again).
 assertTrue(listenerCalled);