[es6] Implement proper TDZ for parameters

Previously, examples like (({a = x}, x) => {})({}, 0) did not throw a ReferenceError like they should. This CL

- Splits up DeclareFormalParameters such that the formals can be recorded first and declared later.

- Declaration then takes the complete parameter list into account. If it is not simple, temporaries are introduced for all parameters.

- BuildParameterInitializationBlock desugars all parameters from non-simple lists into let-bindings.

- Refactored Pre/ParserFormalParameters, so that the arity information is no longer duplicated in Parser.

- Rest is currently handled specially, until rest-via-destructuring has landed.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:811
LOG=N

Review URL: https://codereview.chromium.org/1259283002

Cr-Commit-Position: refs/heads/master@{#30025}

diff --git a/src/parser.cc b/src/parser.cc
index 311d192..9e3d672 100644 (file)
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -1203,6 +1203,11 @@ FunctionLiteral* Parser::ParseLazy(Isolate* isolate, ParseInfo* info,
           // BindingIdentifier
           const bool is_rest = false;
           ParseFormalParameter(is_rest, &formals, &formals_classifier, &ok);
+          if (ok) {
+            DeclareFormalParameter(
+                formals.scope, formals.at(0), formals.is_simple,
+                &formals_classifier);
+          }
         }
       }
 
@@ -2223,10 +2228,7 @@ Statement* Parser::ParseFunctionDeclaration(
       is_strong(language_mode())
           ? CONST
           : (is_strict(language_mode()) || allow_harmony_sloppy()) &&
-                    !(scope_->is_script_scope() || scope_->is_eval_scope() ||
-                      scope_->is_function_scope())
-                ? LET
-                : VAR;
+              !scope_->is_declaration_scope() ? LET : VAR;
   VariableProxy* proxy = NewUnresolved(name, mode);
   Declaration* declaration =
       factory()->NewFunctionDeclaration(proxy, mode, fun, scope_, pos);
@@ -3854,7 +3856,7 @@ void ParserTraits::ParseArrowFunctionFormalParameters(
     ParserFormalParameters* parameters, Expression* expr,
     const Scanner::Location& params_loc,
     Scanner::Location* duplicate_loc, bool* ok) {
-  if (parameters->arity >= Code::kMaxArguments) {
+  if (parameters->Arity() >= Code::kMaxArguments) {
     ReportMessageAt(params_loc, MessageTemplate::kMalformedArrowFunParamList);
     *ok = false;
     return;
@@ -3908,11 +3910,26 @@ void ParserTraits::ParseArrowFunctionFormalParameters(
     parser_->scope_->RemoveUnresolved(expr->AsVariableProxy());
   }
 
-  ++parameters->arity;
-  ExpressionClassifier classifier;
-  DeclareFormalParameter(parameters, expr, is_rest, &classifier);
-  if (!duplicate_loc->IsValid()) {
-    *duplicate_loc = classifier.duplicate_formal_parameter_error().location;
+  AddFormalParameter(parameters, expr, is_rest);
+}
+
+
+void ParserTraits::ParseArrowFunctionFormalParameterList(
+    ParserFormalParameters* parameters, Expression* expr,
+    const Scanner::Location& params_loc,
+    Scanner::Location* duplicate_loc, bool* ok) {
+  ParseArrowFunctionFormalParameters(parameters, expr, params_loc,
+                                     duplicate_loc, ok);
+  if (!*ok) return;
+
+  for (int i = 0; i < parameters->Arity(); ++i) {
+    auto parameter = parameters->at(i);
+    ExpressionClassifier classifier;
+    DeclareFormalParameter(
+        parameters->scope, parameter, parameters->is_simple, &classifier);
+    if (!duplicate_loc->IsValid()) {
+      *duplicate_loc = classifier.duplicate_formal_parameter_error().location;
+    }
   }
 }
 
@@ -4032,8 +4049,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     scope_->set_start_position(start_position);
     ParserFormalParameters formals(scope);
     ParseFormalParameterList(&formals, &formals_classifier, CHECK_OK);
-    arity = formals.arity;
-    DCHECK(arity == formals.params.length());
+    arity = formals.Arity();
     Expect(Token::RPAREN, CHECK_OK);
     int formals_end_position = scanner()->location().end_pos;
 
@@ -4299,7 +4315,8 @@ Block* Parser::BuildParameterInitializationBlock(
       factory()->NewBlock(NULL, 1, true, RelocInfo::kNoPosition);
   for (int i = 0; i < parameters.params.length(); ++i) {
     auto parameter = parameters.params[i];
-    if (parameter.pattern == nullptr) continue;
+    // TODO(caitp,rossberg): Remove special handling for rest once desugared.
+    if (parameter.is_rest) break;
     DeclarationDescriptor descriptor;
     descriptor.declaration_kind = DeclarationDescriptor::PARAMETER;
     descriptor.parser = this;

diff --git a/src/parser.h b/src/parser.h
index 2086ee9..4a77d91 100644 (file)
--- a/src/parser.h
+++ b/src/parser.h
@@ -539,7 +539,7 @@ 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) {}
@@ -549,15 +549,11 @@ struct ParserFormalParameters : public PreParserFormalParameters {
   };
 
   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]; }
 };
 
 
@@ -782,13 +778,19 @@ class ParserTraits {
   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);
 
@@ -1310,25 +1312,35 @@ Expression* ParserTraits::SpreadCallNew(
 }
 
 
-void ParserTraits::DeclareFormalParameter(
-    ParserFormalParameters* parameters, Expression* pattern, bool is_rest,
-    ExpressionClassifier* classifier) {
-  bool is_duplicate = false;
+void ParserTraits::AddFormalParameter(
+    ParserFormalParameters* parameters, Expression* pattern, bool is_rest) {
   bool is_simple = pattern->IsVariableProxy();
-  DCHECK(parser_->allow_harmony_destructuring() || is_simple);
-
+  DCHECK(parser_->allow_harmony_destructuring() ||
+         parser_->allow_harmony_rest_parameters() || is_simple);
   const AstRawString* name = is_simple
                                  ? pattern->AsVariableProxy()->raw_name()
                                  : parser_->ast_value_factory()->empty_string();
-  VariableMode mode = is_simple ? VAR : TEMPORARY;
+  parameters->params.Add(
+      ParserFormalParameters::Parameter(name, pattern, is_rest),
+      parameters->scope->zone());
+}
+
+
+void ParserTraits::DeclareFormalParameter(
+    Scope* scope, const ParserFormalParameters::Parameter& parameter,
+    bool is_simple, ExpressionClassifier* classifier) {
+  bool is_duplicate = false;
+  // 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.

diff --git a/src/preparser.h b/src/preparser.h
index 3bd0953..d240db1 100644 (file)
--- a/src/preparser.h
+++ b/src/preparser.h
@@ -27,6 +27,15 @@ enum FunctionNameValidity {
 };
 
 
+struct FormalParametersBase {
+  explicit FormalParametersBase(Scope* scope) : scope(scope) {}
+  Scope* scope;
+  bool has_rest = false;
+  bool is_simple = true;
+  int materialized_literals_count = 0;
+};
+
+
 // Common base class shared between parser and pre-parser. Traits encapsulate
 // the differences between Parser and PreParser:
 
@@ -1312,18 +1321,13 @@ class PreParserFactory {
 };
 
 
-struct PreParserFormalParameters {
+struct PreParserFormalParameters : FormalParametersBase {
   explicit PreParserFormalParameters(Scope* scope)
-      : scope(scope),
-        arity(0),
-        has_rest(false),
-        is_simple(true),
-        materialized_literals_count(0) {}
-  Scope* scope;
-  int arity;
-  bool has_rest;
-  bool is_simple;
-  int materialized_literals_count;
+      : FormalParametersBase(scope) {}
+  int arity = 0;
+
+  int Arity() const { return arity; }
+  PreParserIdentifier at(int i) { return PreParserIdentifier(); }  // Dummy
 };
 
 
@@ -1606,7 +1610,7 @@ class PreParserTraits {
       const PreParserFormalParameters& parameters, FunctionKind kind,
       FunctionLiteral::FunctionType function_type, bool* ok);
 
-  V8_INLINE void ParseArrowFunctionFormalParameters(
+  V8_INLINE void ParseArrowFunctionFormalParameterList(
       PreParserFormalParameters* parameters,
       PreParserExpression expression, const Scanner::Location& params_loc,
       Scanner::Location* duplicate_loc, bool* ok);
@@ -1637,8 +1641,13 @@ class PreParserTraits {
     return !tag.IsNoTemplateTag();
   }
 
-  void DeclareFormalParameter(PreParserFormalParameters* parameters,
-                              PreParserExpression pattern, bool is_rest,
+  void AddFormalParameter(
+      PreParserFormalParameters* parameters, PreParserExpression pattern,
+      bool is_rest) {
+    ++parameters->arity;
+  }
+  void DeclareFormalParameter(Scope* scope, PreParserIdentifier parameter,
+                              bool is_simple,
                               ExpressionClassifier* classifier) {}
 
   void CheckConflictingVarDeclarations(Scope* scope, bool* ok) {}
@@ -1835,7 +1844,7 @@ PreParserExpression PreParserTraits::SpreadCallNew(PreParserExpression function,
 }
 
 
-void PreParserTraits::ParseArrowFunctionFormalParameters(
+void PreParserTraits::ParseArrowFunctionFormalParameterList(
     PreParserFormalParameters* parameters,
     PreParserExpression params, const Scanner::Location& params_loc,
     Scanner::Location* duplicate_loc, bool* ok) {
@@ -2280,12 +2289,15 @@ ParserBase<Traits>::ParsePrimaryExpression(ExpressionClassifier* classifier,
         // (...x) => y
         Scope* scope =
             this->NewScope(scope_, ARROW_SCOPE, FunctionKind::kArrowFunction);
-        FormalParametersT parameters(scope);
+        FormalParametersT formals(scope);
         scope->set_start_position(beg_pos);
-        ExpressionClassifier args_classifier;
+        ExpressionClassifier formals_classifier;
         const bool is_rest = true;
-        this->ParseFormalParameter(is_rest, &parameters, &args_classifier,
+        this->ParseFormalParameter(is_rest, &formals, &formals_classifier,
                                    CHECK_OK);
+        Traits::DeclareFormalParameter(
+            formals.scope, formals.at(0), formals.is_simple,
+            &formals_classifier);
         if (peek() == Token::COMMA) {
           ReportMessageAt(scanner()->peek_location(),
                           MessageTemplate::kParamAfterRest);
@@ -2293,7 +2305,7 @@ ParserBase<Traits>::ParsePrimaryExpression(ExpressionClassifier* classifier,
           return this->EmptyExpression();
         }
         Expect(Token::RPAREN, CHECK_OK);
-        result = this->ParseArrowFunctionLiteral(parameters, args_classifier,
+        result = this->ParseArrowFunctionLiteral(formals, formals_classifier,
                                                  CHECK_OK);
       } else {
         // Heuristically try to detect immediately called functions before
@@ -2845,8 +2857,8 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
 
     scope->set_start_position(lhs_location.beg_pos);
     Scanner::Location duplicate_loc = Scanner::Location::invalid();
-    this->ParseArrowFunctionFormalParameters(&parameters, expression, loc,
-                                             &duplicate_loc, CHECK_OK);
+    this->ParseArrowFunctionFormalParameterList(&parameters, expression, loc,
+                                                &duplicate_loc, CHECK_OK);
     if (duplicate_loc.IsValid()) {
       arrow_formals_classifier.RecordDuplicateFormalParameterError(
           duplicate_loc);
@@ -3647,8 +3659,7 @@ void ParserBase<Traits>::ParseFormalParameter(
     *ok = false;
     return;
   }
-  ++parameters->arity;
-  Traits::DeclareFormalParameter(parameters, pattern, is_rest, classifier);
+  Traits::AddFormalParameter(parameters, pattern, is_rest);
 }
 
 
@@ -3669,11 +3680,11 @@ void ParserBase<Traits>::ParseFormalParameterList(
   //   FormalsList[?Yield, ?GeneratorParameter] ,
   //     FormalParameter[?Yield,?GeneratorParameter]
 
-  DCHECK_EQ(0, parameters->arity);
+  DCHECK_EQ(0, parameters->Arity());
 
   if (peek() != Token::RPAREN) {
     do {
-      if (parameters->arity > Code::kMaxArguments) {
+      if (parameters->Arity() > Code::kMaxArguments) {
         ReportMessage(MessageTemplate::kTooManyParameters);
         *ok = false;
         return;
@@ -3687,8 +3698,15 @@ void ParserBase<Traits>::ParseFormalParameterList(
       ReportMessageAt(scanner()->peek_location(),
                       MessageTemplate::kParamAfterRest);
       *ok = false;
+      return;
     }
   }
+
+  for (int i = 0; i < parameters->Arity(); ++i) {
+    auto parameter = parameters->at(i);
+    Traits::DeclareFormalParameter(
+        parameters->scope, parameter, parameters->is_simple, classifier);
+  }
 }
 
 

diff --git a/test/mjsunit/harmony/destructuring.js b/test/mjsunit/harmony/destructuring.js
index f9b6203..41b366a 100644 (file)
--- a/test/mjsunit/harmony/destructuring.js
+++ b/test/mjsunit/harmony/destructuring.js
@@ -806,16 +806,70 @@
   assertEquals(1, g20({a: 1, b: 2}));
   // var g21 = ({[eval('y')]: x}) => { var y = 'b'; return x; };
   // assertEquals(1, g21({a: 1, b: 2}));
+})();
+
+
+(function TestParameterTDZ() {
+  function f1({a = x}, x) { return a }
+  assertThrows(() => f1({}, 4), ReferenceError);
+  assertEquals(4, f1({a: 4}, 5));
+  // TODO(rossberg): eval in default expressions is not working yet.
+  // function f2({a = eval("x")}, x) { return a }
+  // assertThrows(() => f2({}, 4), ReferenceError);
+  // assertEquals(4, f2({a: 4}, 5));
+  // function f3({a = eval("x")}, x) { 'use strict'; return a }
+  // assertThrows(() => f3({}, 4), ReferenceError);
+  // assertEquals(4, f3({a: 4}, 5));
+  // function f4({a = eval("'use strict'; x")}, x) { return a }
+  // assertThrows(() => f4({}, 4), ReferenceError);
+  // assertEquals(4, f4({a: 4}, 5));
+
+  function f5({a = () => x}, x) { return a() }
+  assertEquals(4, f5({a: () => 4}, 5));
+  // TODO(rossberg): eval in default expressions is not working yet.
+  // function f6({a = () => eval("x")}, x) { return a() }
+  // assertEquals(4, f6({a: () => 4}, 5));
+  // function f7({a = () => eval("x")}, x) { 'use strict'; return a() }
+  // assertEquals(4, f7({a: () => 4}, 5));
+  // function f8({a = () => eval("'use strict'; x")}, x) { return a() }
+  // assertEquals(4, f8({a: () => 4}, 5));
+
+  function f11({a = b}, {b}) { return a }
+  assertThrows(() => f11({}, {b: 4}), ReferenceError);
+  assertEquals(4, f11({a: 4}, {b: 5}));
+  // function f12({a = eval("b")}, {b}) { return a }
+  // assertThrows(() => f12({}, {b: 4}), ReferenceError);
+  // assertEquals(4, f12({a: 4}, {b: 5}));
+  // function f13({a = eval("b")}, {b}) { 'use strict'; return a }
+  // assertThrows(() => f13({}, {b: 4}), ReferenceError);
+  // assertEquals(4, f13({a: 4}, {b: 5}));
+  // function f14({a = eval("'use strict'; b")}, {b}) { return a }
+  // assertThrows(() => f14({}, {b: 4}), ReferenceError);
+  // assertEquals(4, f14({a: 4}, {b: 5}));
+
+  function f15({a = () => b}, {b}) { return a() }
+  assertEquals(4, f15({a: () => 4}, {b: 5}));
+  // function f16({a = () => eval("b")}, {b}) { return a() }
+  // assertEquals(4, f16({a: () => 4}, {b: 5}));
+  // function f17({a = () => eval("b")}, {b}) { 'use strict'; return a() }
+  // assertEquals(4, f17({a: () => 4}, {b: 5}));
+  // function f18({a = () => eval("'use strict'; b")}, {b}) { return a() }
+  // assertEquals(4, f18({a: () => 4}, {b: 5}));
 
   // TODO(caitp): TDZ for rest parameters is not working yet.
-  // function f30({x = a}, ...a) {}
+  // function f30({x = a}, ...a) { return x[0] }
   // assertThrows(() => f30({}), ReferenceError);
-  // function f31({x = eval("a")}, ...a) {}
+  // assertEquals(4, f30({a: [4]}, 5));
+  // function f31({x = eval("a")}, ...a) { return x[0] }
   // assertThrows(() => f31({}), ReferenceError);
-  // function f32({x = eval("a")}, ...a) { 'use strict'; }
+  // assertEquals(4, f31({a: [4]}, 5));
+  // function f32({x = eval("a")}, ...a) { 'use strict'; return x[0] }
   // assertThrows(() => f32({}), ReferenceError);
-  // function f33({x = eval("'use strict'; a")}, ...a) {}
+  // assertEquals(4, f32({a: [4]}, 5));
+  // function f33({x = eval("'use strict'; a")}, ...a) { return x[0] }
   // assertThrows(() => f33({}), ReferenceError);
+  // assertEquals(4, f33({a: [4]}, 5));
+
   function f34({x = function() { return a }}, ...a) { return x()[0] }
   assertEquals(4, f34({}, 4));
   function f35({x = () => a}, ...a) { return x()[0] }

diff --git a/test/test262-es6/test262-es6.status b/test/test262-es6/test262-es6.status
index 29ecd6c..8696f96 100644 (file)
--- a/test/test262-es6/test262-es6.status
+++ b/test/test262-es6/test262-es6.status
@@ -436,6 +436,9 @@
   'language/computed-property-names/class/static/method-symbol': [FAIL, FAIL_SLOPPY],
   'language/computed-property-names/class/static/method-string': [FAIL, FAIL_SLOPPY],
 
+  # This should work as soon as rest parameters are re-implemented via desaguring.
+  'language/expressions/arrow-function/syntax/early-errors/arrowparameters-cover-no-duplicates-rest': [PASS, FAIL],
+
   # https://code.google.com/p/v8/issues/detail?id=2160
   'language/expressions/arrow-function/syntax/arrowparameters-cover-initialize-1': [FAIL],
   'language/expressions/arrow-function/syntax/arrowparameters-cover-initialize-2': [FAIL],