[modules] AST and parser rework.

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index 4308b333a0c9a9c48ed8054ae0afde5ac023f496..db0278cc9decc7f976f51bd6118a4a959e1c6c51 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -949,6 +949,8 @@ FunctionLiteral* Parser::DoParseProgram(ParseInfo* info) {
     parsing_module_ = info->is_module();
     if (parsing_module_) {
       ParseModuleItemList(body, &ok);
+      ok = ok &&
+           scope_->module()->Validate(scope_, &pending_error_handler_, zone());
     } else {
       // Don't count the mode in the use counters--give the program a chance
       // to enable script-wide strict mode below.
@@ -1319,7 +1321,7 @@ Statement* Parser::ParseStatementListItem(bool* ok) {
 
 
 Statement* Parser::ParseModuleItem(bool* ok) {
-  // (Ecma 262 6th Edition, 15.2):
+  // ecma262/#prod-ModuleItem
   // ModuleItem :
   //    ImportDeclaration
   //    ExportDeclaration
@@ -1327,7 +1329,8 @@ Statement* Parser::ParseModuleItem(bool* ok) {
 
   switch (peek()) {
     case Token::IMPORT:
-      return ParseImportDeclaration(ok);
+      ParseImportDeclaration(CHECK_OK);
+      return factory()->NewEmptyStatement(kNoSourcePosition);
     case Token::EXPORT:
       return ParseExportDeclaration(ok);
     default:
@@ -1337,37 +1340,21 @@ Statement* Parser::ParseModuleItem(bool* ok) {
 
 
 void* Parser::ParseModuleItemList(ZoneList<Statement*>* body, bool* ok) {

[adamk, 2016/07/13 18:38:21]

This doesn't seem to need a return value at all (could be "void" instead of
"void*"). ParseStatementList has other callers which is why it returns a void* I
think.

[neis, 2016/07/14 10:28:24]

void* is needed because of the beautiful CHECK_OK macro.

[adamk, 2016/07/14 18:26:33]

Except no one calls this with CHECK_OK. But I guess someone might in future,
doesn't matter much either way.

-  // (Ecma 262 6th Edition, 15.2):
+  // ecma262/#prod-Module
   // Module :
   //    ModuleBody?
   //
+  // ecma262/#prod-ModuleItemList
   // ModuleBody :
   //    ModuleItem*
 
   DCHECK(scope_->is_module_scope());
-
   while (peek() != Token::EOS) {
     Statement* stat = ParseModuleItem(CHECK_OK);
     if (stat && !stat->IsEmpty()) {
       body->Add(stat, zone());
     }
   }
-
-  // Check that all exports are bound.
-  ModuleDescriptor* descriptor = scope_->module();
-  for (ModuleDescriptor::Iterator it = descriptor->iterator(); !it.done();
-       it.Advance()) {
-    if (scope_->LookupLocal(it.local_name()) == NULL) {
-      // TODO(adamk): Pass both local_name and export_name once ParserTraits
-      // supports multiple arg error messages.
-      // Also try to report this at a better location.
-      ParserTraits::ReportMessage(MessageTemplate::kModuleExportUndefined,
-                                  it.local_name());
-      *ok = false;
-      return NULL;
-    }
-  }
-
   return NULL;
 }
 
@@ -1425,11 +1412,12 @@ void* Parser::ParseExportClause(ZoneList<const AstRawString*>* export_names,
 
   Expect(Token::RBRACE, CHECK_OK);
 
-  return 0;
+  return nullptr;
 }
 
 
-ZoneList<ImportDeclaration*>* Parser::ParseNamedImports(int pos, bool* ok) {
+ZoneList<const Parser::NamedImport*>* Parser::ParseNamedImports(
+    int pos, bool* ok) {
   // NamedImports :
   //   '{' '}'
   //   '{' ImportsList '}'
@@ -1445,8 +1433,7 @@ ZoneList<ImportDeclaration*>* Parser::ParseNamedImports(int pos, bool* ok) {
 
   Expect(Token::LBRACE, CHECK_OK);
 
-  ZoneList<ImportDeclaration*>* result =
-      new (zone()) ZoneList<ImportDeclaration*>(1, zone());
+  auto result = new (zone()) ZoneList<const NamedImport*>(1, zone());
   while (peek() != Token::RBRACE) {
     const AstRawString* import_name = ParseIdentifierName(CHECK_OK);
     const AstRawString* local_name = import_name;
@@ -1460,36 +1447,37 @@ ZoneList<ImportDeclaration*>* Parser::ParseNamedImports(int pos, bool* ok) {
                              parsing_module_)) {
       *ok = false;
       ReportMessage(MessageTemplate::kUnexpectedReserved);
-      return NULL;
+      return nullptr;
     } else if (IsEvalOrArguments(local_name)) {
       *ok = false;
       ReportMessage(MessageTemplate::kStrictEvalArguments);
-      return NULL;
+      return nullptr;
     }
-    VariableProxy* proxy = NewUnresolved(local_name, CONST);
-    ImportDeclaration* declaration =
-        factory()->NewImportDeclaration(proxy, import_name, NULL, scope_, pos);
-    Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
-    result->Add(declaration, zone());
+
+    DeclareImport(local_name, position(), CHECK_OK);  // XXX

[adamk, 2016/07/13 18:38:22]

What's this XXX about? Should it be replaced with a TODO or removed?

[neis, 2016/07/14 10:28:24]

Just an indicator for you (see below). Removed now.

+
+    NamedImport* import = new (zone()->New(sizeof(NamedImport)))

[adamk, 2016/07/13 18:38:22]

Same as elsewhere, if you make NamedImport a ZoneObject then this gets cleaner.

[neis, 2016/07/14 10:28:24]

Done.

+        NamedImport(import_name, local_name, scanner()->location());
+    result->Add(import, zone());
+
     if (peek() == Token::RBRACE) break;
     Expect(Token::COMMA, CHECK_OK);
   }
 
   Expect(Token::RBRACE, CHECK_OK);
-
   return result;
 }
 
 
-Statement* Parser::ParseImportDeclaration(bool* ok) {
+void* Parser::ParseImportDeclaration(bool* ok) {
   // ImportDeclaration :
   //   'import' ImportClause 'from' ModuleSpecifier ';'
   //   'import' ModuleSpecifier ';'
   //
   // ImportClause :
+  //   ImportedDefaultBinding
   //   NameSpaceImport
   //   NamedImports
-  //   ImportedDefaultBinding
   //   ImportedDefaultBinding ',' NameSpaceImport
   //   ImportedDefaultBinding ',' NamedImports
   //
@@ -1504,132 +1492,182 @@ Statement* Parser::ParseImportDeclaration(bool* ok) {
   // 'import' ModuleSpecifier ';'
   if (tok == Token::STRING) {
     const AstRawString* module_specifier = ParseModuleSpecifier(CHECK_OK);
-    scope_->module()->AddModuleRequest(module_specifier, zone());
     ExpectSemicolon(CHECK_OK);
-    return factory()->NewEmptyStatement(pos);
+    scope_->module()->AddEmptyImport(
+        module_specifier, scanner()->location(), zone());
+    return nullptr;
   }
 
   // Parse ImportedDefaultBinding if present.
-  ImportDeclaration* import_default_declaration = NULL;
+  const AstRawString* import_default_binding = nullptr;
+  Scanner::Location import_default_binding_loc;
   if (tok != Token::MUL && tok != Token::LBRACE) {
-    const AstRawString* local_name =
+    import_default_binding =
         ParseIdentifier(kDontAllowRestrictedIdentifiers, CHECK_OK);
-    VariableProxy* proxy = NewUnresolved(local_name, CONST);
-    import_default_declaration = factory()->NewImportDeclaration(
-        proxy, ast_value_factory()->default_string(), NULL, scope_, pos);
-    Declare(import_default_declaration, DeclarationDescriptor::NORMAL, true,
-            CHECK_OK);
+    import_default_binding_loc = scanner()->location();
+    DeclareImport(import_default_binding, pos, CHECK_OK);  // XXX

[adamk, 2016/07/13 18:38:22]

???

[neis, 2016/07/14 10:28:24]

Done.

   }
 
-  const AstRawString* module_instance_binding = NULL;
-  ZoneList<ImportDeclaration*>* named_declarations = NULL;
-  if (import_default_declaration == NULL || Check(Token::COMMA)) {
+  // Parse NameSpaceImport or NamedImports if present.
+  const AstRawString* module_namespace_binding = nullptr;
+  Scanner::Location module_namespace_binding_loc;
+  const ZoneList<const NamedImport*>* named_imports = nullptr;
+  if (import_default_binding == nullptr || Check(Token::COMMA)) {
     switch (peek()) {
       case Token::MUL: {
         Consume(Token::MUL);
         ExpectContextualKeyword(CStrVector("as"), CHECK_OK);
-        module_instance_binding =
+        module_namespace_binding =
             ParseIdentifier(kDontAllowRestrictedIdentifiers, CHECK_OK);
-        // TODO(ES6): Add an appropriate declaration.
+        module_namespace_binding_loc = scanner()->location();
         break;
       }
 
       case Token::LBRACE:
-        named_declarations = ParseNamedImports(pos, CHECK_OK);
+        named_imports = ParseNamedImports(pos, CHECK_OK);
         break;
 
       default:
         *ok = false;
         ReportUnexpectedToken(scanner()->current_token());
-        return NULL;
+        return nullptr;
     }
   }
 
   ExpectContextualKeyword(CStrVector("from"), CHECK_OK);
   const AstRawString* module_specifier = ParseModuleSpecifier(CHECK_OK);
-  scope_->module()->AddModuleRequest(module_specifier, zone());
+  ExpectSemicolon(CHECK_OK);
 
-  if (module_instance_binding != NULL) {
-    // TODO(ES6): Set the module specifier for the module namespace binding.
+  // Now that we have all the information, we can make the appropriate
+  // declarations.
+
+  if (module_namespace_binding != nullptr) {
+    scope_->module()->AddStarImport(
+        module_namespace_binding, module_specifier,
+        module_namespace_binding_loc, zone());
+    // TODO(neis): Create special immutable binding for the namespace object.
   }
 
-  if (import_default_declaration != NULL) {
-    import_default_declaration->set_module_specifier(module_specifier);
+  // TODO(neis): Would prefer to call DeclareImport below rather than above and
+  // in ParseNamedImports, but then a possible error message would point to the
+  // wrong location.  Maybe have a DeclareAt version of Declare that takes a
+  // location?
+
+  if (import_default_binding != nullptr) {
+    scope_->module()->AddNonStarImport(
+        ast_value_factory()->default_string(), import_default_binding,
+        module_specifier, import_default_binding_loc, zone());
+    // DeclareImport(import_default_binding, pos, CHECK_OK);

[adamk, 2016/07/13 18:38:22]

Are these what the XXXs are about?

[neis, 2016/07/14 10:28:24]

Exactly.

   }
 
-  if (named_declarations != NULL) {
-    for (int i = 0; i < named_declarations->length(); ++i) {
-      named_declarations->at(i)->set_module_specifier(module_specifier);
+  if (named_imports != nullptr) {
+    if (named_imports->length() == 0) {
+      scope_->module()->AddEmptyImport(
+          module_specifier, scanner()->location(), zone());
+    } else {
+      for (int i = 0; i < named_imports->length(); ++i) {
+        const NamedImport* import = named_imports->at(i);
+        scope_->module()->AddNonStarImport(
+            import->import_name, import->local_name,
+            module_specifier, import->location, zone());
+        // DeclareImport(import->local_name, pos, CHECK_OK);
+      }
     }
   }
 
-  ExpectSemicolon(CHECK_OK);
-  return factory()->NewEmptyStatement(pos);
+  return nullptr;
 }
 
 
 Statement* Parser::ParseExportDefault(bool* ok) {
   //  Supports the following productions, starting after the 'default' token:
-  //    'export' 'default' FunctionDeclaration
+  //    'export' 'default' HoistableDeclaration
   //    'export' 'default' ClassDeclaration
   //    'export' 'default' AssignmentExpression[In] ';'
 
   Expect(Token::DEFAULT, CHECK_OK);
   Scanner::Location default_loc = scanner()->location();
 
-  const AstRawString* default_string = ast_value_factory()->default_string();
-  ZoneList<const AstRawString*> names(1, zone());
+  ZoneList<const AstRawString*> local_names(1, zone());
   Statement* result = nullptr;
-  Expression* default_export = nullptr;
   switch (peek()) {
     case Token::FUNCTION:
-      result = ParseHoistableDeclaration(&names, true, CHECK_OK);
+      result = ParseHoistableDeclaration(&local_names, true, CHECK_OK);
       break;
 
     case Token::CLASS:
       Consume(Token::CLASS);
-      result = ParseClassDeclaration(&names, true, CHECK_OK);
+      result = ParseClassDeclaration(&local_names, true, CHECK_OK);
       break;
 
     case Token::ASYNC:
       if (allow_harmony_async_await() && PeekAhead() == Token::FUNCTION &&
           !scanner()->HasAnyLineTerminatorAfterNext()) {
         Consume(Token::ASYNC);
-        result = ParseAsyncFunctionDeclaration(&names, true, CHECK_OK);
+        result = ParseAsyncFunctionDeclaration(&local_names, true, CHECK_OK);
         break;
       }
     /* falls through */
 
     default: {
-      int pos = peek_position();
+      int pos = position();
       ExpressionClassifier classifier(this);
-      Expression* expr = ParseAssignmentExpression(true, &classifier, CHECK_OK);
+      Expression* value =
+          ParseAssignmentExpression(true, &classifier, CHECK_OK);
       RewriteNonPattern(&classifier, CHECK_OK);
 
+      if (value->IsAnonymousFunctionDefinition()) {
+        // Ensure that the function's "name" property gets set to "default", by
+        // rewriting e.g.
+        //   function () {}
+        // to
+        //   %SetFunctionName(function () {}, "default")
+        // Note that %SetFunctionName returns the function.
+        //
+        // TODO(neis): Make sure this only happens when the function value does
+        // not already have an own "name" property, as can happen with class
+        // literals:
+        //   class { static name() {} }
+        //
+        ZoneList<Expression*>* args =
+            new (zone()) ZoneList<Expression*>(1, zone());
+        args->Add(value, zone());
+        args->Add(factory()->NewStringLiteral(
+            ast_value_factory()->default_string(), kNoSourcePosition), zone());
+        value = factory()->NewCallRuntime(
+            Runtime::kFunctionSetName, args, kNoSourcePosition);

[adamk, 2016/07/13 18:38:22]

Oh, I didn't understand your question this morning. You want the logic that's
currently in ParserTraits::SetFunctionNameFromIdentifierRef(), rather than this
runtime call. That'll set the name in the FunctionLiteral or ClassLiteral AST
node, which will then be properly overridden.

[neis, 2016/07/14 10:28:23]

Ah, I see.  Done (with some refactoring of the existing SetFunctionNameFromFoo
functions).

+      }
+
+      const AstRawString* local_name =
+          ast_value_factory()->star_default_star_string();
+      local_names.Add(local_name, zone());
+
+      // It's fine to declare this as CONST because the user has no way of
+      // writing to it.
+      VariableProxy* proxy = NewUnresolved(local_name, CONST);
+      Declaration* declaration =
+          factory()->NewVariableDeclaration(proxy, CONST, scope_, pos);
+      Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+      proxy->var()->set_initializer_position(position());
+
+      Assignment* assignment = factory()->NewAssignment(
+          Token::INIT, proxy, value, kNoSourcePosition);
+      result = factory()->NewExpressionStatement(assignment, kNoSourcePosition);
+
       ExpectSemicolon(CHECK_OK);
-      result = factory()->NewExpressionStatement(expr, pos);
       break;
     }
   }
 
-  DCHECK_LE(names.length(), 1);
-  if (names.length() == 1) {
-    scope_->module()->AddLocalExport(default_string, names.first(), zone(), ok);
-    if (!*ok) {
-      ParserTraits::ReportMessageAt(
-          default_loc, MessageTemplate::kDuplicateExport, default_string);
-      return nullptr;
-    }
-  } else {
-    // TODO(ES6): Assign result to a const binding with the name "*default*"
-    // and add an export entry with "*default*" as the local name.
-    USE(default_export);
-  }
+  DCHECK_EQ(local_names.length(), 1);
+  scope_->module()->AddNonStarExport(
+      local_names.first(), ast_value_factory()->default_string(), default_loc,
+      zone());
 
+  DCHECK_NOT_NULL(result);
   return result;
 }
 
-
 Statement* Parser::ParseExportDeclaration(bool* ok) {
   // ExportDeclaration:
   //    'export' '*' 'from' ModuleSpecifier ';'
@@ -1641,7 +1679,7 @@ Statement* Parser::ParseExportDeclaration(bool* ok) {
   int pos = peek_position();
   Expect(Token::EXPORT, CHECK_OK);
 
-  Statement* result = NULL;
+  Statement* result = nullptr;
   ZoneList<const AstRawString*> names(1, zone());
   switch (peek()) {
     case Token::DEFAULT:
@@ -1651,9 +1689,9 @@ Statement* Parser::ParseExportDeclaration(bool* ok) {
       Consume(Token::MUL);
       ExpectContextualKeyword(CStrVector("from"), CHECK_OK);
       const AstRawString* module_specifier = ParseModuleSpecifier(CHECK_OK);
-      scope_->module()->AddModuleRequest(module_specifier, zone());
-      // TODO(ES6): scope_->module()->AddStarExport(...)
       ExpectSemicolon(CHECK_OK);
+      scope_->module()->AddStarExport(
+          module_specifier, scanner()->location(), zone());
       return factory()->NewEmptyStatement(pos);
     }
 
@@ -1675,35 +1713,36 @@ Statement* Parser::ParseExportDeclaration(bool* ok) {
       ZoneList<const AstRawString*> local_names(1, zone());
       ParseExportClause(&export_names, &export_locations, &local_names,
                         &reserved_loc, CHECK_OK);
-      const AstRawString* indirect_export_module_specifier = NULL;
+      const AstRawString* module_specifier = nullptr;
       if (CheckContextualKeyword(CStrVector("from"))) {
-        indirect_export_module_specifier = ParseModuleSpecifier(CHECK_OK);
+        module_specifier = ParseModuleSpecifier(CHECK_OK);
       } else if (reserved_loc.IsValid()) {
         // No FromClause, so reserved words are invalid in ExportClause.
         *ok = false;
         ReportMessageAt(reserved_loc, MessageTemplate::kUnexpectedReserved);
-        return NULL;
+        return nullptr;
       }
       ExpectSemicolon(CHECK_OK);
       const int length = export_names.length();
       DCHECK_EQ(length, local_names.length());
       DCHECK_EQ(length, export_locations.length());
-      if (indirect_export_module_specifier == NULL) {
+      if (module_specifier == nullptr) {
         for (int i = 0; i < length; ++i) {
-          scope_->module()->AddLocalExport(export_names[i], local_names[i],
-                                           zone(), ok);
-          if (!*ok) {
-            ParserTraits::ReportMessageAt(export_locations[i],
-                                          MessageTemplate::kDuplicateExport,
-                                          export_names[i]);
-            return NULL;
-          }
+          scope_->module()->AddNonStarExport(local_names[i], export_names[i],
+                                             export_locations[i], zone());
         }
       } else {
-        scope_->module()->AddModuleRequest(indirect_export_module_specifier,
-                                           zone());
-        for (int i = 0; i < length; ++i) {
-          // TODO(ES6): scope_->module()->AddIndirectExport(...);(
+        if (length == 0) {

[adamk, 2016/07/13 18:38:22]

You can collapse this up into the outer if statement (I'd find it easier to
grok):

if (module_specifier == nullptr) {
} else if (length == 0) {
} else {
}

[neis, 2016/07/14 10:28:23]

Done. Also removed the TODO since the exact location doesn't really matter for
an empty import.

+          // TODO(neis): Provide better location.
+          scope_->module()->AddEmptyImport(
+              module_specifier, scanner()->location(), zone());
+        } else {
+          auto& import_names = local_names;

[adamk, 2016/07/13 18:38:22]

Is this just for clarity? I'd leave it out.

[neis, 2016/07/14 10:28:23]

Yes. Done. Instead, I renamed local_names to original_names.

+          for (int i = 0; i < length; ++i) {
+            scope_->module()->AddNonStarExport(
+                import_names[i], export_names[i], module_specifier,
+                export_locations[i], zone());
+          }
         }
       }
       return factory()->NewEmptyStatement(pos);
@@ -1726,6 +1765,8 @@ Statement* Parser::ParseExportDeclaration(bool* ok) {
 
     case Token::ASYNC:
       if (allow_harmony_async_await()) {
+        // TODO(neis): Why don't we have the same check here as in
+        // ParseStatementListItem?
         Consume(Token::ASYNC);
         result = ParseAsyncFunctionDeclaration(&names, false, CHECK_OK);
         break;
@@ -1735,18 +1776,14 @@ Statement* Parser::ParseExportDeclaration(bool* ok) {
     default:
       *ok = false;
       ReportUnexpectedToken(scanner()->current_token());
-      return NULL;
+      return nullptr;
   }
 
-  // Extract declared names into export declarations.
   ModuleDescriptor* descriptor = scope_->module();
   for (int i = 0; i < names.length(); ++i) {
-    descriptor->AddLocalExport(names[i], names[i], zone(), ok);
-    if (!*ok) {
-      // TODO(adamk): Possibly report this error at the right place.
-      ParserTraits::ReportMessage(MessageTemplate::kDuplicateExport, names[i]);
-      return NULL;
-    }
+    // TODO(neis): Provide better location.
+    descriptor->AddNonStarExport(
+        names[i], names[i], scanner()->location(), zone());
   }
 
   DCHECK_NOT_NULL(result);
@@ -1902,6 +1939,16 @@ VariableProxy* Parser::NewUnresolved(const AstRawString* name,
 }
 
 
+void* Parser::DeclareImport(const AstRawString* local_name, int pos, bool* ok) {
+  DCHECK_NOT_NULL(local_name);
+  VariableProxy* proxy = NewUnresolved(local_name, IMPORT);
+  Declaration* declaration =
+      factory()->NewVariableDeclaration(proxy, IMPORT, scope_, pos);
+  Declare(declaration, DeclarationDescriptor::NORMAL, true, CHECK_OK);
+  return nullptr;
+}
+
+
 Variable* Parser::Declare(Declaration* declaration,
                           DeclarationDescriptor::Kind declaration_kind,
                           bool resolve, bool* ok, Scope* scope) {
@@ -2173,9 +2220,6 @@ Statement* Parser::ParseHoistableDeclaration(
                               : FunctionKind::kNormalFunction,
       pos, FunctionLiteral::kDeclaration, language_mode(), CHECK_OK);
 
-  // Even if we're not at the top-level of the global or a function
-  // scope, we treat it as such and introduce the function with its
-  // initial value upon entering the corresponding scope.
   // In ES6, a function behaves as a lexical binding, except in
   // a script scope, or the initial scope of eval or another function.
   VariableMode mode =
@@ -2495,6 +2539,7 @@ static bool ContainsLabel(ZoneList<const AstRawString*>* labels,
   return false;
 }
 
+// TODO(neis): Better name.

[adamk, 2016/07/13 18:38:22]

Please expand on this TODO (why does it need a better name?) or remove it.

[neis, 2016/07/14 10:28:23]

This doesn't belong into this CL, removed.

 Statement* Parser::ParseFunctionDeclaration(bool* ok) {
   Consume(Token::FUNCTION);
   int pos = position();