Preparsing inner funcs: be less pessimistic about maybe_assigned.

src/parsing/preparser.h

 // Copyright 2012 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.
 
 #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"
 
 namespace v8 {
 namespace internal {
 
 // Whereas the Parser generates AST during the recursive descent,
 // the PreParser doesn't create a tree. Instead, it passes around minimal
 // data objects (PreParserExpression, PreParserIdentifier etc.) which contain
@@ skipping 101 matching lines @@
   const AstRawString* string_;
   friend class PreParserExpression;
   friend class PreParser;
   friend class PreParserFactory;
 };
 
 
 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;
   }
 
   static PreParserExpression BinaryOperation(PreParserExpression left,
                                              Token::Value op,
                                              PreParserExpression right) {
     return PreParserExpression(TypeField::encode(kExpression));
   }
 
   static PreParserExpression Assignment(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
+      ZoneList<VariableProxy*>* variables = nullptr) {

[neis, 2016/12/12 13:01:36]

Can we remove the default value here and below? I think it's not used.

[marja, 2016/12/12 13:36:46]

Done.

     return PreParserExpression(TypeField::encode(kExpression) |
                                    ExpressionTypeField::encode(kAssignment),
-                               identifiers);
+                               variables);
   }
 
   static PreParserExpression ObjectLiteral(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
+      ZoneList<VariableProxy*>* variables = nullptr) {
     return PreParserExpression(TypeField::encode(kObjectLiteralExpression),
-                               identifiers);
+                               variables);
   }
 
   static PreParserExpression ArrayLiteral(
-      ZoneList<const AstRawString*>* identifiers = nullptr) {
+      ZoneList<VariableProxy*>* variables = nullptr) {
     return PreParserExpression(TypeField::encode(kArrayLiteralExpression),
-                               identifiers);
+                               variables);
   }
 
   static PreParserExpression StringLiteral() {
     return PreParserExpression(TypeField::encode(kStringLiteralExpression));
   }
 
   static PreParserExpression UseStrictStringLiteral() {
     return PreParserExpression(TypeField::encode(kStringLiteralExpression) |
                                IsUseStrictField::encode(true));
   }
@@ skipping 156 matching lines @@
     kThisExpression,
     kThisPropertyExpression,
     kPropertyExpression,
     kCallExpression,
     kCallEvalExpression,
     kSuperCallReference,
     kNoTemplateTagExpression,
     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.
   typedef BitField<Type, 0, 3> TypeField;
 
   // The high order bit applies only to nodes which would inherit from the
   // Expression ASTNode --- This is by necessity, due to the fact that
   // Expression nodes may be represented as multiple Types, not exclusively
   // through kExpression.
   // TODO(caitp, adamk): clean up PreParserExpression bitfields.
   typedef BitField<bool, 31, 1> ParenthesizedField;
 
   // The rest of the bits are interpreted depending on the value
   // of the Type field, so they can share the storage.
   typedef BitField<ExpressionType, TypeField::kNext, 3> ExpressionTypeField;
   typedef BitField<bool, TypeField::kNext, 1> IsUseStrictField;
   typedef BitField<bool, IsUseStrictField::kNext, 1> IsUseAsmField;
   typedef BitField<PreParserIdentifier::Type, TypeField::kNext, 10>
       IdentifierTypeField;
   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;
   template <typename T>
   friend class PreParserList;
 };
 
 
 // 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_; }
   static PreParserList Null() { return PreParserList(-1); }
   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;
 };
 
 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_;
 }
 
 template <typename T>
 void PreParserList<T>::Add(T, Zone* zone) {
   ++length_;
 }
 
@@ skipping 78 matching lines @@
   };
 
   explicit PreParserStatement(Type code) : code_(code) {}
   Type code_;
 };
 
 
 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; }
 
   PreParserExpression NewStringLiteral(PreParserIdentifier identifier,
                                        int pos) {
     // 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_);
+    }

[neis, 2016/12/12 13:01:36]

Why do we create a variable proxy for a string literal?

[marja, 2016/12/12 13:36:46]

Discussed offline: for object literal properties which through this code path
(see comment above).

     return expression;
   }
   PreParserExpression NewNumberLiteral(double number,
                                        int pos) {
     return PreParserExpression::Default();
   }
   PreParserExpression NewUndefinedLiteral(int pos) {
     return PreParserExpression::Default();
   }
   PreParserExpression NewRegExpLiteral(PreParserIdentifier js_pattern,
                                        int js_flags, int literal_index,
                                        int pos) {
     return PreParserExpression::Default();
   }
   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,
                                               ClassLiteralProperty::Kind kind,
                                               bool is_static,
                                               bool is_computed_name) {
     return PreParserExpression::Default();
   }
   PreParserExpression NewObjectLiteralProperty(PreParserExpression key,
                                                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();
   }
   PreParserExpression NewProperty(PreParserExpression obj,
                                   PreParserExpression key,
                                   int pos) {
     if (obj.IsThis()) {
       return PreParserExpression::ThisProperty();
     }
@@ skipping 14 matching lines @@
                                           PreParserExpression right, int pos) {
     return PreParserExpression::Default();
   }
   PreParserExpression NewRewritableExpression(PreParserExpression expression) {
     return expression;
   }
   PreParserExpression NewAssignment(Token::Value op,
                                     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,
                                Yield::OnException on_exception) {
     return PreParserExpression::Default();
   }
   PreParserExpression NewConditional(PreParserExpression condition,
                                      PreParserExpression then_expression,
                                      PreParserExpression else_expression,
                                      int pos) {
@@ skipping 116 matching lines @@
 
   // Return the object itself as AstVisitor and implement the needed
   // dummy method right in this class.
   PreParserFactory* visitor() { return this; }
   int* ast_properties() {
     static int dummy = 42;
     return &dummy;
   }
 
  private:
+  AstValueFactory* ast_value_factory_;
   Zone* zone_;
 };
 
 
 struct PreParserFormalParameters : FormalParametersBase {
   struct Parameter : public ZoneObject {
     explicit Parameter(PreParserExpression pattern) : pattern(pattern) {}
     Parameter** next() { return &next_parameter; }
     Parameter* const* next() const { return &next_parameter; }
     PreParserExpression pattern;
@@ skipping 442 matching lines @@
   V8_INLINE static void PushVariableName(PreParserIdentifier id) {}
   V8_INLINE void PushPropertyName(PreParserExpression expression) {}
   V8_INLINE void PushEnclosingName(PreParserIdentifier name) {}
   V8_INLINE static void AddFunctionForNameInference(
       PreParserExpression expression) {}
   V8_INLINE static void InferFunctionName() {}
 
   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.

[neis, 2016/12/12 13:01:36]

Is this TODO now resolved?

[marja, 2016/12/12 13:36:46]

Sadly, no. It's orthogonal-ish. We still only track the variables in the
track_unresolved_variables_ = true mode, and don't track them in a way that
would allow us to produce all the errors.. (and error producing is not
implemented).

+    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,
                                                         PreParserExpression y,
                                                         Token::Value op,
                                                         int pos) {
     return false;
   }
 
   V8_INLINE PreParserExpression BuildUnaryExpression(
       PreParserExpression expression, Token::Value op, int pos) {
     return PreParserExpression::Default();
   }
 
   V8_INLINE PreParserExpression BuildIteratorResult(PreParserExpression value,
                                                     bool done) {
     return PreParserExpression::Default();
   }
 
   V8_INLINE PreParserStatement
   BuildInitializationBlock(DeclarationParsingResult* parsing_result,
                            ZoneList<const AstRawString*>* names, bool* ok) {
     return PreParserStatement::Default();
   }
 
   V8_INLINE PreParserStatement
   InitializeForEachStatement(PreParserStatement stmt, PreParserExpression each,
                              PreParserExpression subject,
                              PreParserStatement body, int each_keyword_pos) {
+    MarkExpressionAsAssigned(each);

[neis, 2016/12/12 13:01:36]

Maybe reflect this change in the CL description.

[marja, 2016/12/12 13:36:46]

Offline discussion: this falls under the description.

     return stmt;
   }
 
   V8_INLINE PreParserStatement RewriteForVarInLegacy(const ForInfo& for_info) {
     return PreParserStatement::Null();
   }
   V8_INLINE void DesugarBindingInForEachStatement(
       ForInfo* for_info, PreParserStatement* body_block,
       PreParserExpression* each_variable, bool* ok) {}
   V8_INLINE PreParserStatement CreateForEachStatementTDZ(
@@ skipping 210 matching lines @@
 
   V8_INLINE void DeclareFormalParameters(
       DeclarationScope* scope,
       const ThreadedList<PreParserFormalParameters::Parameter>& parameters) {
     if (!classifier()->is_simple_parameter_list()) {
       scope->SetHasNonSimpleParameters();
     }
     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);
           }
         }
       }
     }
   }
 
   V8_INLINE void DeclareArrowFunctionFormalParameters(
       PreParserFormalParameters* parameters, PreParserExpression params,
       const Scanner::Location& params_loc, Scanner::Location* duplicate_loc,
       bool* ok) {
@@ skipping 14 matching lines @@
   }
 
   V8_INLINE void MaterializeUnspreadArgumentsLiterals(int count) {
     for (int i = 0; i < count; ++i) {
       function_state_->NextMaterializedLiteralIndex();
     }
   }
 
   V8_INLINE PreParserExpression
   ExpressionListToExpression(PreParserExpressionList args) {
-    return PreParserExpression::Default(args.identifiers_);
+    return PreParserExpression::Default(args.variables_);
   }
 
   V8_INLINE void AddAccessorPrefixToFunctionName(bool is_get,
                                                  PreParserExpression function,
                                                  PreParserIdentifier name) {}
   V8_INLINE void SetFunctionNameFromPropertyName(PreParserExpression property,
                                                  PreParserIdentifier name) {}
   V8_INLINE void SetFunctionNameFromIdentifierRef(
       PreParserExpression value, PreParserExpression identifier) {}
 
@@ skipping 58 matching lines @@
     function_state_->NextMaterializedLiteralIndex();
     function_state_->NextMaterializedLiteralIndex();
   }
   return EmptyExpression();
 }
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_PARSING_PREPARSER_H