Preparsing inner funcs: be less pessimistic about maybe_assigned.

BUG=v8:5501, v8:5678

Review-Url: https://codereview.chromium.org/2539123002
Cr-Commit-Position: refs/heads/master@{#41645}

src/ast/scopes.cc

@@ -1093,26 +1093,6 @@ bool Scope::RemoveUnresolved(VariableProxy* var) {
   return false;
 }
 
-bool Scope::RemoveUnresolved(const AstRawString* name) {
-  if (unresolved_ != nullptr && unresolved_->raw_name() == name) {
-    VariableProxy* removed = unresolved_;
-    unresolved_ = unresolved_->next_unresolved();
-    removed->set_next_unresolved(nullptr);
-    return true;
-  }
-  VariableProxy* current = unresolved_;
-  while (current != nullptr) {
-    VariableProxy* next = current->next_unresolved();
-    if (next != nullptr && next->raw_name() == name) {
-      current->set_next_unresolved(next->next_unresolved());
-      next->set_next_unresolved(nullptr);
-      return true;
-    }
-    current = next;
-  }
-  return false;
-}
-
 Variable* Scope::NewTemporary(const AstRawString* name) {
   DeclarationScope* scope = GetClosureScope();
   Variable* var = new (zone())

src/ast/scopes.h

@@ -183,7 +183,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // allocated globally as a "ghost" variable. RemoveUnresolved removes
   // such a variable again if it was added; otherwise this is a no-op.
   bool RemoveUnresolved(VariableProxy* var);
-  bool RemoveUnresolved(const AstRawString* name);
 
   // Creates a new temporary variable in this scope's TemporaryScope.  The
   // name is only used for printing and cannot be used to find the variable.

src/parsing/preparser.cc

@@ -269,19 +269,17 @@ PreParser::LazyParsingResult PreParser::ParseStatementListAndLogFunction(
 
 PreParserExpression PreParser::ExpressionFromIdentifier(
     PreParserIdentifier name, int start_position, InferName infer) {
+  VariableProxy* proxy = nullptr;
   if (track_unresolved_variables_) {
     AstNodeFactory factory(ast_value_factory());
     // Setting the Zone is necessary because zone_ might be the temp Zone, and
     // AstValueFactory doesn't know about it.
     factory.set_zone(zone());
     DCHECK_NOT_NULL(name.string_);
-    VariableProxy* proxy = scope()->NewUnresolved(
-        &factory, name.string_, start_position, NORMAL_VARIABLE);
-    // We don't know whether the preparsed function assigns or not, so we set
-    // is_assigned pessimistically.
-    proxy->set_is_assigned();
+    proxy = scope()->NewUnresolved(&factory, name.string_, start_position,
+                                   NORMAL_VARIABLE);
   }
-  return PreParserExpression::FromIdentifier(name, zone());
+  return PreParserExpression::FromIdentifier(name, proxy, zone());
 }
 
 void PreParser::DeclareAndInitializeVariables(
@@ -289,7 +287,7 @@ void PreParser::DeclareAndInitializeVariables(
     const DeclarationDescriptor* declaration_descriptor,
     const DeclarationParsingResult::Declaration* declaration,
     ZoneList<const AstRawString*>* names, bool* ok) {
-  if (declaration->pattern.identifiers_ != nullptr) {
+  if (declaration->pattern.variables_ != nullptr) {
     DCHECK(FLAG_lazy_inner_functions);
     /* Mimic what Parser does when declaring variables (see
        Parser::PatternRewriter::VisitVariableProxy).
@@ -306,12 +304,13 @@ void PreParser::DeclareAndInitializeVariables(
     if (declaration->initializer.IsEmpty() ||
         (declaration_descriptor->mode == VariableMode::LET ||
          declaration_descriptor->mode == VariableMode::CONST)) {
-      for (auto identifier : *(declaration->pattern.identifiers_)) {
-        declaration_descriptor->scope->RemoveUnresolved(identifier);
+      for (auto variable : *(declaration->pattern.variables_)) {
+        declaration_descriptor->scope->RemoveUnresolved(variable);
       }
     }
-    for (auto identifier : *(declaration->pattern.identifiers_)) {
-      scope->DeclareVariableName(identifier, declaration_descriptor->mode);
+    for (auto variable : *(declaration->pattern.variables_)) {
+      scope->DeclareVariableName(variable->raw_name(),
+                                 declaration_descriptor->mode);
     }
   }
 }

src/parsing/preparser.h

@@ -5,6 +5,7 @@
 #ifndef V8_PARSING_PREPARSER_H
 #define V8_PARSING_PREPARSER_H
 
+#include "src/ast/ast.h"
 #include "src/ast/scopes.h"
 #include "src/parsing/parser-base.h"
 #include "src/parsing/preparse-data.h"
@@ -126,25 +127,26 @@ class PreParserIdentifier {
 class PreParserExpression {
  public:
   PreParserExpression()
-      : code_(TypeField::encode(kEmpty)), identifiers_(nullptr) {}
+      : code_(TypeField::encode(kEmpty)), variables_(nullptr) {}
 
   static PreParserExpression Empty() { return PreParserExpression(); }
 
   static PreParserExpression Default(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
-    return PreParserExpression(TypeField::encode(kExpression), identifiers);
+      ZoneList<VariableProxy*>* variables = nullptr) {
+    return PreParserExpression(TypeField::encode(kExpression), variables);
   }
 
   static PreParserExpression Spread(PreParserExpression expression) {
     return PreParserExpression(TypeField::encode(kSpreadExpression),
-                               expression.identifiers_);
+                               expression.variables_);
   }
 
   static PreParserExpression FromIdentifier(PreParserIdentifier id,
+                                            VariableProxy* variable,
                                             Zone* zone) {
     PreParserExpression expression(TypeField::encode(kIdentifierExpression) |
                                    IdentifierTypeField::encode(id.type_));
-    expression.AddIdentifier(id.string_, zone);
+    expression.AddVariable(variable, zone);
     return expression;
   }
 
@@ -154,23 +156,21 @@ class PreParserExpression {
     return PreParserExpression(TypeField::encode(kExpression));
   }
 
-  static PreParserExpression Assignment(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
+  static PreParserExpression Assignment(ZoneList<VariableProxy*>* variables) {
     return PreParserExpression(TypeField::encode(kExpression) |
                                    ExpressionTypeField::encode(kAssignment),
-                               identifiers);
+                               variables);
   }
 
   static PreParserExpression ObjectLiteral(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
+      ZoneList<VariableProxy*>* variables) {
     return PreParserExpression(TypeField::encode(kObjectLiteralExpression),
-                               identifiers);
+                               variables);
   }
 
-  static PreParserExpression ArrayLiteral(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
+  static PreParserExpression ArrayLiteral(ZoneList<VariableProxy*>* variables) {
     return PreParserExpression(TypeField::encode(kArrayLiteralExpression),
-                               identifiers);
+                               variables);
   }
 
   static PreParserExpression StringLiteral() {
@@ -347,19 +347,18 @@ class PreParserExpression {
     kAssignment
   };
 
-  explicit PreParserExpression(
-      uint32_t expression_code,
-      ZoneList<const AstRawString*>* identifiers = nullptr)
-      : code_(expression_code), identifiers_(identifiers) {}
+  explicit PreParserExpression(uint32_t expression_code,
+                               ZoneList<VariableProxy*>* variables = nullptr)
+      : code_(expression_code), variables_(variables) {}
 
-  void AddIdentifier(const AstRawString* identifier, Zone* zone) {
-    if (identifier == nullptr) {
+  void AddVariable(VariableProxy* variable, Zone* zone) {
+    if (variable == nullptr) {
       return;
     }
-    if (identifiers_ == nullptr) {
-      identifiers_ = new (zone) ZoneList<const AstRawString*>(1, zone);
+    if (variables_ == nullptr) {
+      variables_ = new (zone) ZoneList<VariableProxy*>(1, zone);
     }
-    identifiers_->Add(identifier, zone);
+    variables_->Add(variable, zone);
   }
 
   // The first three bits are for the Type.
@@ -382,9 +381,9 @@ class PreParserExpression {
   typedef BitField<bool, TypeField::kNext, 1> HasCoverInitializedNameField;
 
   uint32_t code_;
-  // If the PreParser is used in the identifier tracking mode,
-  // PreParserExpression accumulates identifiers in that expression.
-  ZoneList<const AstRawString*>* identifiers_;
+  // If the PreParser is used in the variable tracking mode, PreParserExpression
+  // accumulates variables in that expression.
+  ZoneList<VariableProxy*>* variables_;
 
   friend class PreParser;
   friend class PreParserFactory;
@@ -394,13 +393,13 @@ class PreParserExpression {
 
 
 // The pre-parser doesn't need to build lists of expressions, identifiers, or
-// the like. If the PreParser is used in identifier tracking mode, it needs to
-// build lists of identifiers though.
+// the like. If the PreParser is used in variable tracking mode, it needs to
+// build lists of variables though.
 template <typename T>
 class PreParserList {
  public:
   // These functions make list->Add(some_expression) work (and do nothing).
-  PreParserList() : length_(0), identifiers_(nullptr) {}
+  PreParserList() : length_(0), variables_(nullptr) {}
   PreParserList* operator->() { return this; }
   void Add(T, Zone* zone);
   int length() const { return length_; }
@@ -408,9 +407,9 @@ class PreParserList {
   bool IsNull() const { return length_ == -1; }
 
  private:
-  explicit PreParserList(int n) : length_(n), identifiers_(nullptr) {}
+  explicit PreParserList(int n) : length_(n), variables_(nullptr) {}
   int length_;
-  ZoneList<const AstRawString*>* identifiers_;
+  ZoneList<VariableProxy*>* variables_;
 
   friend class PreParser;
   friend class PreParserFactory;
@@ -419,14 +418,14 @@ class PreParserList {
 template <>
 inline void PreParserList<PreParserExpression>::Add(
     PreParserExpression expression, Zone* zone) {
-  if (expression.identifiers_ != nullptr) {
+  if (expression.variables_ != nullptr) {
     DCHECK(FLAG_lazy_inner_functions);
     DCHECK(zone != nullptr);
-    if (identifiers_ == nullptr) {
-      identifiers_ = new (zone) ZoneList<const AstRawString*>(1, zone);
+    if (variables_ == nullptr) {
+      variables_ = new (zone) ZoneList<VariableProxy*>(1, zone);
     }
-    for (auto identifier : (*expression.identifiers_)) {
-      identifiers_->Add(identifier, zone);
+    for (auto identifier : (*expression.variables_)) {
+      variables_->Add(identifier, zone);
     }
   }
   ++length_;
@@ -525,7 +524,8 @@ class PreParserStatement {
 class PreParserFactory {
  public:
   explicit PreParserFactory(AstValueFactory* ast_value_factory)
-      : zone_(ast_value_factory->zone()) {}
+      : ast_value_factory_(ast_value_factory),
+        zone_(ast_value_factory->zone()) {}
 
   void set_zone(Zone* zone) { zone_ = zone; }
 
@@ -534,7 +534,14 @@ class PreParserFactory {
     // This is needed for object literal property names. Property names are
     // normalized to string literals during object literal parsing.
     PreParserExpression expression = PreParserExpression::Default();
-    expression.AddIdentifier(identifier.string_, zone_);
+    if (identifier.string_ != nullptr) {
+      DCHECK(FLAG_lazy_inner_functions);
+      AstNodeFactory factory(ast_value_factory_);
+      factory.set_zone(zone_);
+      VariableProxy* variable =
+          factory.NewVariableProxy(identifier.string_, NORMAL_VARIABLE);
+      expression.AddVariable(variable, zone_);
+    }
     return expression;
   }
   PreParserExpression NewNumberLiteral(double number,
@@ -552,7 +559,7 @@ class PreParserFactory {
   PreParserExpression NewArrayLiteral(PreParserExpressionList values,
                                       int first_spread_index, int literal_index,
                                       int pos) {
-    return PreParserExpression::ArrayLiteral(values.identifiers_);
+    return PreParserExpression::ArrayLiteral(values.variables_);
   }
   PreParserExpression NewClassLiteralProperty(PreParserExpression key,
                                               PreParserExpression value,
@@ -565,18 +572,18 @@ class PreParserFactory {
                                                PreParserExpression value,
                                                ObjectLiteralProperty::Kind kind,
                                                bool is_computed_name) {
-    return PreParserExpression::Default(value.identifiers_);
+    return PreParserExpression::Default(value.variables_);
   }
   PreParserExpression NewObjectLiteralProperty(PreParserExpression key,
                                                PreParserExpression value,
                                                bool is_computed_name) {
-    return PreParserExpression::Default(value.identifiers_);
+    return PreParserExpression::Default(value.variables_);
   }
   PreParserExpression NewObjectLiteral(PreParserExpressionList properties,
                                        int literal_index,
                                        int boilerplate_properties,
                                        int pos) {
-    return PreParserExpression::ObjectLiteral(properties.identifiers_);
+    return PreParserExpression::ObjectLiteral(properties.variables_);
   }
   PreParserExpression NewVariableProxy(void* variable) {
     return PreParserExpression::Default();
@@ -611,8 +618,8 @@ class PreParserFactory {
                                     PreParserExpression left,
                                     PreParserExpression right,
                                     int pos) {
-    // For tracking identifiers for parameters with a default value.
-    return PreParserExpression::Assignment(left.identifiers_);
+    // For tracking variables for parameters with a default value.
+    return PreParserExpression::Assignment(left.variables_);
   }
   PreParserExpression NewYield(PreParserExpression generator_object,
                                PreParserExpression expression, int pos,
@@ -749,6 +756,7 @@ class PreParserFactory {
   }
 
  private:
+  AstValueFactory* ast_value_factory_;
   Zone* zone_;
 };
 
@@ -1211,10 +1219,16 @@ class PreParser : public ParserBase<PreParser> {
   V8_INLINE static void CheckAssigningFunctionLiteralToProperty(
       PreParserExpression left, PreParserExpression right) {}
 
-  V8_INLINE static void MarkExpressionAsAssigned(
-      PreParserExpression expression) {
+  V8_INLINE void MarkExpressionAsAssigned(PreParserExpression expression) {
     // TODO(marja): To be able to produce the same errors, the preparser needs
     // to start tracking which expressions are variables and which are assigned.
+    if (expression.variables_ != nullptr) {
+      DCHECK(FLAG_lazy_inner_functions);
+      DCHECK(track_unresolved_variables_);
+      for (auto variable : *expression.variables_) {
+        variable->set_is_assigned();
+      }
+    }
   }
 
   V8_INLINE bool ShortcutNumericLiteralBinaryExpression(PreParserExpression* x,
@@ -1244,6 +1258,7 @@ class PreParser : public ParserBase<PreParser> {
   InitializeForEachStatement(PreParserStatement stmt, PreParserExpression each,
                              PreParserExpression subject,
                              PreParserStatement body, int each_keyword_pos) {
+    MarkExpressionAsAssigned(each);
     return stmt;
   }
 
@@ -1474,9 +1489,9 @@ class PreParser : public ParserBase<PreParser> {
     if (track_unresolved_variables_) {
       DCHECK(FLAG_lazy_inner_functions);
       for (auto parameter : parameters) {
-        if (parameter->pattern.identifiers_ != nullptr) {
-          for (auto i : *parameter->pattern.identifiers_) {
-            scope->DeclareVariableName(i, VAR);
+        if (parameter->pattern.variables_ != nullptr) {
+          for (auto variable : *parameter->pattern.variables_) {
+            scope->DeclareVariableName(variable->raw_name(), VAR);
           }
         }
       }
@@ -1511,7 +1526,7 @@ class PreParser : public ParserBase<PreParser> {
 
   V8_INLINE PreParserExpression
   ExpressionListToExpression(PreParserExpressionList args) {
-    return PreParserExpression::Default(args.identifiers_);
+    return PreParserExpression::Default(args.variables_);
   }
 
   V8_INLINE void AddAccessorPrefixToFunctionName(bool is_get,

test/cctest/test-parsing.cc

@@ -3302,63 +3302,68 @@ TEST(InnerAssignment) {
       {"function x() {}; var x;", true, false},
       {"var x; try {} catch (x) { var x = 5; }", true, false},
   };
+
+  // We set allow_error_in_inner_function to true in cases where our handling of
+  // assigned variables in lazy inner functions is currently overly pessimistic.
+  // FIXME(marja): remove it when no longer needed.
   struct {
     const char* source;
     bool assigned;
     bool with;
+    bool allow_error_in_inner_function;
   } inners[] = {
       // Actual assignments.
-      {"x = 1;", true, false},
-      {"x++;", true, false},
-      {"++x;", true, false},
-      {"x--;", true, false},
-      {"--x;", true, false},
-      {"{ x = 1; }", true, false},
-      {"'use strict'; { let x; }; x = 0;", true, false},
-      {"'use strict'; { const x = 1; }; x = 0;", true, false},
-      {"'use strict'; { function x() {} }; x = 0;", true, false},
-      {"with ({}) { x = 1; }", true, true},
-      {"eval('');", true, false},
-      {"'use strict'; { let y; eval('') }", true, false},
-      {"function h() { x = 0; }", true, false},
-      {"(function() { x = 0; })", true, false},
-      {"(function() { x = 0; })", true, false},
-      {"with ({}) (function() { x = 0; })", true, true},
-      {"for (x of [1,2,3]) {}", true, false},
-      {"for (x in {a: 1}) {}", true, false},
-      {"for ([x] of [[1],[2],[3]]) {}", true, false},
-      {"for ([x] in {ab: 1}) {}", true, false},
-      {"for ([...x] in {ab: 1}) {}", true, false},
-      {"[x] = [1]", true, false},
+      {"x = 1;", true, false, false},
+      {"x++;", true, false, false},
+      {"++x;", true, false, false},
+      {"x--;", true, false, false},
+      {"--x;", true, false, false},
+      {"{ x = 1; }", true, false, false},
+      {"'use strict'; { let x; }; x = 0;", true, false, false},
+      {"'use strict'; { const x = 1; }; x = 0;", true, false, false},
+      {"'use strict'; { function x() {} }; x = 0;", true, false, false},
+      {"with ({}) { x = 1; }", true, true, false},
+      {"eval('');", true, false, false},
+      {"'use strict'; { let y; eval('') }", true, false, false},
+      {"function h() { x = 0; }", true, false, false},
+      {"(function() { x = 0; })", true, false, false},
+      {"(function() { x = 0; })", true, false, false},
+      {"with ({}) (function() { x = 0; })", true, true, false},
+      {"for (x of [1,2,3]) {}", true, false, false},
+      {"for (x in {a: 1}) {}", true, false, false},
+      {"for ([x] of [[1],[2],[3]]) {}", true, false, false},
+      {"for ([x] in {ab: 1}) {}", true, false, false},
+      {"for ([...x] in {ab: 1}) {}", true, false, false},
+      {"[x] = [1]", true, false, false},
       // Actual non-assignments.
-      {"", false, false},
-      {"x;", false, false},
-      {"var x;", false, false},
-      {"var x = 8;", false, false},
-      {"var x; x = 8;", false, false},
-      {"'use strict'; let x;", false, false},
-      {"'use strict'; let x = 8;", false, false},
-      {"'use strict'; let x; x = 8;", false, false},
-      {"'use strict'; const x = 8;", false, false},
-      {"function x() {}", false, false},
-      {"function x() { x = 0; }", false, false},
-      {"function h(x) { x = 0; }", false, false},
-      {"'use strict'; { let x; x = 0; }", false, false},
-      {"{ var x; }; x = 0;", false, false},
-      {"with ({}) {}", false, true},
-      {"var x; { with ({}) { x = 1; } }", false, true},
-      {"try {} catch(x) { x = 0; }", false, false},
-      {"try {} catch(x) { with ({}) { x = 1; } }", false, true},
+      {"", false, false, false},
+      {"x;", false, false, false},
+      {"var x;", false, false, false},
+      {"var x = 8;", false, false, false},
+      {"var x; x = 8;", false, false, false},
+      {"'use strict'; let x;", false, false, false},
+      {"'use strict'; let x = 8;", false, false, false},
+      {"'use strict'; let x; x = 8;", false, false, false},
+      {"'use strict'; const x = 8;", false, false, false},
+      {"function x() {}", false, false, false},
+      {"function x() { x = 0; }", false, false, true},
+      {"function h(x) { x = 0; }", false, false, false},
+      {"'use strict'; { let x; x = 0; }", false, false, false},
+      {"{ var x; }; x = 0;", false, false, false},
+      {"with ({}) {}", false, true, false},
+      {"var x; { with ({}) { x = 1; } }", false, true, false},
+      {"try {} catch(x) { x = 0; }", false, false, true},
+      {"try {} catch(x) { with ({}) { x = 1; } }", false, true, true},
       // Eval approximation.
-      {"eval('');", true, false},
-      {"function h() { eval(''); }", true, false},
-      {"(function() { eval(''); })", true, false},
+      {"eval('');", true, false, false},
+      {"function h() { eval(''); }", true, false, false},
+      {"(function() { eval(''); })", true, false, false},
       // Shadowing not recognized because of eval approximation.
-      {"var x; eval('');", true, false},
-      {"'use strict'; let x; eval('');", true, false},
-      {"try {} catch(x) { eval(''); }", true, false},
-      {"function x() { eval(''); }", true, false},
-      {"(function(x) { eval(''); })", true, false},
+      {"var x; eval('');", true, false, false},
+      {"'use strict'; let x; eval('');", true, false, false},
+      {"try {} catch(x) { eval(''); }", true, false, false},
+      {"function x() { eval(''); }", true, false, false},
+      {"(function(x) { eval(''); })", true, false, false},
   };
 
   int prefix_len = Utf8LengthHelper(prefix);
@@ -3417,9 +3422,8 @@ TEST(InnerAssignment) {
         CHECK(var->is_used() || !expected);
         bool is_maybe_assigned = var->maybe_assigned() == i::kMaybeAssigned;
         if (i::FLAG_lazy_inner_functions) {
-          // If we parse inner functions lazily, allow being pessimistic about
-          // maybe_assigned.
-          CHECK(is_maybe_assigned || (is_maybe_assigned == expected));
+          CHECK(is_maybe_assigned == expected ||
+                (is_maybe_assigned && inners[j].allow_error_in_inner_function));
         } else {
           CHECK_EQ(is_maybe_assigned, expected);
         }