[es6] Implement proper TDZ for parameters

src/parser.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_PARSER_H_
 #define V8_PARSER_H_
 
 #include "src/allocation.h"
 #include "src/ast.h"
 #include "src/compiler.h"  // TODO(titzer): remove this include dependency
@@ skipping 521 matching lines @@
   bool failed_;
 };
 
 // ----------------------------------------------------------------------------
 // JAVASCRIPT PARSING
 
 class Parser;
 class SingletonLogger;
 
 
-struct ParserFormalParameters : public PreParserFormalParameters {
+struct ParserFormalParameters : FormalParametersBase {
   struct Parameter {
     Parameter(const AstRawString* name, Expression* pattern, bool is_rest)
         : name(name), pattern(pattern), is_rest(is_rest) {}
     const AstRawString* name;
     Expression* pattern;
     bool is_rest;
   };
 
   explicit ParserFormalParameters(Scope* scope)
-      : PreParserFormalParameters(scope), params(4, scope->zone()) {}
-
+      : FormalParametersBase(scope), params(4, scope->zone()) {}
   ZoneList<Parameter> params;
 
-  void AddParameter(
-      const AstRawString* name, Expression* pattern, bool is_rest) {
-    params.Add(Parameter(name, pattern, is_rest), scope->zone());
-    DCHECK_EQ(arity, params.length());
-  }
+  int Arity() const { return params.length(); }
+  const Parameter& at(int i) const { return params[i]; }

[caitp (gmail), 2015/08/04 16:46:25]

minor question: Is there a sort of system in place for deciding whether to name
methods with PascalCase vs camelCase vs snake_case? All three seem to be used in
V8, sometimes within the same classes (as here). The style-guide doesn't seem
super clear on this.

[adamk, 2015/08/04 17:25:04]

camelCase shouldn't be used in C++ (and I can't think of a place where it is),
though we do use it in JS. For the other two, see this bit of the Google C++
style guide:

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

Short answer: trivial inlined members (such as getters/setters, or this at()
case) may have snake_case names (the idea being to signal that they are
trivial). Any non-trivial method uses PascalCase.

[rossberg, 2015/08/04 19:09:21]

What Adam said. And yes, V8 is rather inconsistent with this convention. OTOH,
I'm not even convinced the convention makes a lot of sense.

 };
 
 
 class ParserTraits {
  public:
   struct Type {
     // TODO(marja): To be removed. The Traits object should contain all the data
     // it needs.
     typedef v8::internal::Parser* Parser;
 
@@ skipping 204 matching lines @@
     return new(zone) ZoneList<v8::internal::Statement*>(size, zone);
   }
 
   V8_INLINE void AddParameterInitializationBlock(
       const ParserFormalParameters& parameters,
       ZoneList<v8::internal::Statement*>* body, bool* ok);
 
   V8_INLINE Scope* NewScope(Scope* parent_scope, ScopeType scope_type,
                             FunctionKind kind = kNormalFunction);
 
+  V8_INLINE void AddFormalParameter(
+      ParserFormalParameters* parameters, Expression* pattern, bool is_rest);
   V8_INLINE void DeclareFormalParameter(
-      ParserFormalParameters* parameters, Expression* pattern, bool is_rest,
-      ExpressionClassifier* classifier);
+      Scope* scope, const ParserFormalParameters::Parameter& parameter,
+      bool is_simple, ExpressionClassifier* classifier);
   void ParseArrowFunctionFormalParameters(
       ParserFormalParameters* parameters, Expression* params,
       const Scanner::Location& params_loc,
       Scanner::Location* duplicate_loc, bool* ok);
+  void ParseArrowFunctionFormalParameterList(
+      ParserFormalParameters* parameters, Expression* params,
+      const Scanner::Location& params_loc,
+      Scanner::Location* duplicate_loc, bool* ok);
 
   void ReindexLiterals(const ParserFormalParameters& parameters);
 
   // Temporary glue; these functions will move to ParserBase.
   Expression* ParseV8Intrinsic(bool* ok);
   FunctionLiteral* ParseFunctionLiteral(
       const AstRawString* name, Scanner::Location function_name_location,
       FunctionNameValidity function_name_validity, FunctionKind kind,
       int function_token_position, FunctionLiteral::FunctionType type,
       FunctionLiteral::ArityRestriction arity_restriction,
@@ skipping 501 matching lines @@
   return parser_->SpreadCall(function, args, pos);
 }
 
 
 Expression* ParserTraits::SpreadCallNew(
     Expression* function, ZoneList<v8::internal::Expression*>* args, int pos) {
   return parser_->SpreadCallNew(function, args, pos);
 }
 
 
+void ParserTraits::AddFormalParameter(
+    ParserFormalParameters* parameters, Expression* pattern, bool is_rest) {
+  bool is_simple = pattern->IsVariableProxy();
+  DCHECK(parser_->allow_harmony_destructuring() ||
+         parser_->allow_harmony_rest_parameters() || is_simple);

[caitp (gmail), 2015/08/04 16:46:25]

since these are just DCHECKs, why not make these more robust? Ensure the rest
flag is used for rest, destructuring flag used for patterns. Might avoid some
false DCHECK successes?

[rossberg, 2015/08/04 19:09:21]

Good idea. Will incorporate that into the next CL.

+  const AstRawString* name = is_simple
+                                 ? pattern->AsVariableProxy()->raw_name()
+                                 : parser_->ast_value_factory()->empty_string();
+  parameters->params.Add(
+      ParserFormalParameters::Parameter(name, pattern, is_rest),
+      parameters->scope->zone());
+}
+
+
 void ParserTraits::DeclareFormalParameter(
-    ParserFormalParameters* parameters, Expression* pattern, bool is_rest,
-    ExpressionClassifier* classifier) {
+    Scope* scope, const ParserFormalParameters::Parameter& parameter,
+    bool is_simple, ExpressionClassifier* classifier) {
   bool is_duplicate = false;
-  bool is_simple = pattern->IsVariableProxy();
-  DCHECK(parser_->allow_harmony_destructuring() || is_simple);
-
-  const AstRawString* name = is_simple
-                                 ? pattern->AsVariableProxy()->raw_name()
-                                 : parser_->ast_value_factory()->empty_string();
-  VariableMode mode = is_simple ? VAR : TEMPORARY;
+  // TODO(caitp): Remove special handling for rest once desugaring is in.
+  auto name = is_simple || parameter.is_rest
+      ? parameter.name : parser_->ast_value_factory()->empty_string();
+  auto mode = is_simple || parameter.is_rest ? VAR : TEMPORARY;
   Variable* var =
-      parameters->scope->DeclareParameter(name, mode, is_rest, &is_duplicate);
-  parameters->AddParameter(name, is_simple ? nullptr : pattern, is_rest);
+      scope->DeclareParameter(name, mode, parameter.is_rest, &is_duplicate);
   if (is_duplicate) {
     classifier->RecordDuplicateFormalParameterError(
         parser_->scanner()->location());
   }
-  if (is_sloppy(parameters->scope->language_mode())) {
+  if (is_sloppy(scope->language_mode())) {
     // TODO(sigurds) Mark every parameter as maybe assigned. This is a
     // conservative approximation necessary to account for parameters
     // that are assigned via the arguments array.
     var->set_maybe_assigned();
   }
 }
 
 
 void ParserTraits::AddParameterInitializationBlock(
     const ParserFormalParameters& parameters,
     ZoneList<v8::internal::Statement*>* body, bool* ok) {
   if (!parameters.is_simple) {
     auto* init_block =
         parser_->BuildParameterInitializationBlock(parameters, ok);
     if (!*ok) return;
     if (init_block != nullptr) {
       body->Add(init_block, parser_->zone());
     }
   }
 }
 } }  // namespace v8::internal
 
 #endif  // V8_PARSER_H_