From a538d945e39cb653674ef244973ab21d128563d7 Mon Sep 17 00:00:00 2001 From: adamk Date: Thu, 19 Feb 2015 12:14:55 -0800 Subject: [PATCH] Teach ModuleDescriptor about basic local exports Add() becomes AddLocalExport, which takes an export_name and a local_name. New parsing tests exercise this. Also start generating exports for default exports (though this doesn't yet handle anonymous default exports). BUG=v8:1569 LOG=n Review URL: https://codereview.chromium.org/934323004 Cr-Commit-Position: refs/heads/master@{#26758} --- src/ast-value-factory.h | 1 + src/messages.js | 2 +- src/modules.cc | 17 ++- src/modules.h | 9 +- src/parser.cc | 123 +++++++++++++--------- src/parser.h | 4 +- src/scopeinfo.cc | 4 +- test/cctest/cctest.status | 3 - test/cctest/test-parsing.cc | 69 +----------- test/message/export-duplicate-as.js | 9 ++ test/message/export-duplicate-as.out | 7 ++ test/message/export-duplicate-default.js | 8 ++ test/message/export-duplicate-default.out | 7 ++ test/message/export-duplicate.js | 9 ++ test/message/export-duplicate.out | 7 ++ test/message/testcfg.py | 3 + 16 files changed, 148 insertions(+), 134 deletions(-) create mode 100644 test/message/export-duplicate-as.js create mode 100644 test/message/export-duplicate-as.out create mode 100644 test/message/export-duplicate-default.js create mode 100644 test/message/export-duplicate-default.out create mode 100644 test/message/export-duplicate.js create mode 100644 test/message/export-duplicate.out diff --git a/src/ast-value-factory.h b/src/ast-value-factory.h index fe333254bb..39a1732f1c 100644 --- a/src/ast-value-factory.h +++ b/src/ast-value-factory.h @@ -234,6 +234,7 @@ class AstValue : public ZoneObject { F(anonymous_function, "(anonymous function)") \ F(arguments, "arguments") \ F(constructor, "constructor") \ + F(default, "default") \ F(done, "done") \ F(dot, ".") \ F(dot_for, ".for") \ diff --git a/src/messages.js b/src/messages.js index 049f7f602b..7d98cea192 100644 --- a/src/messages.js +++ b/src/messages.js @@ -176,8 +176,8 @@ var kMessages = { symbol_to_string: ["Cannot convert a Symbol value to a string"], symbol_to_primitive: ["Cannot convert a Symbol wrapper object to a primitive value"], symbol_to_number: ["Cannot convert a Symbol value to a number"], - invalid_module_path: ["Module does not export '", "%0", "', or export is not itself a module"], module_export_undefined: ["Export '", "%0", "' is not defined in module"], + duplicate_export: ["Duplicate export of '", "%0", "'"], unexpected_super: ["'super' keyword unexpected here"], extends_value_not_a_function: ["Class extends value ", "%0", " is not a function or null"], prototype_parent_not_an_object: ["Class extends value does not have valid prototype property ", "%0"], diff --git a/src/modules.cc b/src/modules.cc index eb01cf08e4..5fab34dcbd 100644 --- a/src/modules.cc +++ b/src/modules.cc @@ -11,28 +11,27 @@ namespace v8 { namespace internal { -// --------------------------------------------------------------------------- -// Addition. -void ModuleDescriptor::Add(const AstRawString* name, Zone* zone, bool* ok) { - void* key = const_cast(name); +void ModuleDescriptor::AddLocalExport(const AstRawString* export_name, + const AstRawString* local_name, + Zone* zone, bool* ok) { + void* key = const_cast(export_name); - ZoneHashMap** map = &exports_; ZoneAllocationPolicy allocator(zone); - if (*map == nullptr) { - *map = new (zone->New(sizeof(ZoneHashMap))) + if (exports_ == nullptr) { + exports_ = new (zone->New(sizeof(ZoneHashMap))) ZoneHashMap(ZoneHashMap::PointersMatch, ZoneHashMap::kDefaultHashMapCapacity, allocator); } ZoneHashMap::Entry* p = - (*map)->Lookup(key, name->hash(), !IsFrozen(), allocator); + exports_->Lookup(key, export_name->hash(), !IsFrozen(), allocator); if (p == nullptr || p->value != nullptr) { *ok = false; } - p->value = key; + p->value = const_cast(local_name); } } } // namespace v8::internal diff --git a/src/modules.h b/src/modules.h index ac04e47c4d..b7f5d19abc 100644 --- a/src/modules.h +++ b/src/modules.h @@ -28,7 +28,8 @@ class ModuleDescriptor : public ZoneObject { // Add a name to the list of exports. If it already exists, or this descriptor // is frozen, that's an error. - void Add(const AstRawString* name, Zone* zone, bool* ok); + void AddLocalExport(const AstRawString* export_name, + const AstRawString* local_name, Zone* zone, bool* ok); // Do not allow any further refinements, directly or through unification. void Freeze() { frozen_ = true; } @@ -67,10 +68,14 @@ class ModuleDescriptor : public ZoneObject { class Iterator { public: bool done() const { return entry_ == NULL; } - const AstRawString* name() const { + const AstRawString* export_name() const { DCHECK(!done()); return static_cast(entry_->key); } + const AstRawString* local_name() const { + DCHECK(!done()); + return static_cast(entry_->value); + } void Advance() { entry_ = exports_->Next(entry_); } private: diff --git a/src/parser.cc b/src/parser.cc index 985a90f8dc..336afa9a9f 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -1290,8 +1290,11 @@ Statement* Parser::ParseModule(bool* ok) { ModuleDescriptor* descriptor = scope->module(); for (ModuleDescriptor::Iterator it = descriptor->iterator(); !it.done(); it.Advance()) { - if (scope->LookupLocal(it.name()) == NULL) { - ParserTraits::ReportMessage("module_export_undefined", it.name()); + 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("module_export_undefined", it.local_name()); *ok = false; return NULL; } @@ -1312,7 +1315,9 @@ Literal* Parser::ParseModuleSpecifier(bool* ok) { } -void* Parser::ParseExportClause(ZoneList* names, +void* Parser::ParseExportClause(ZoneList* export_names, + ZoneList* export_locations, + ZoneList* local_names, Scanner::Location* reserved_loc, bool* ok) { // ExportClause : // '{' '}' @@ -1337,14 +1342,17 @@ void* Parser::ParseExportClause(ZoneList* names, !Token::IsIdentifier(name_tok, STRICT, false)) { *reserved_loc = scanner()->location(); } - const AstRawString* name = ParseIdentifierName(CHECK_OK); - names->Add(name, zone()); + const AstRawString* local_name = ParseIdentifierName(CHECK_OK); const AstRawString* export_name = NULL; if (CheckContextualKeyword(CStrVector("as"))) { export_name = ParseIdentifierName(CHECK_OK); } - // TODO(ES6): Return the export_name as well as the name. - USE(export_name); + if (export_name == NULL) { + export_name = local_name; + } + export_names->Add(export_name, zone()); + local_names->Add(local_name, zone()); + export_locations->Add(scanner()->location(), zone()); if (peek() == Token::RBRACE) break; Expect(Token::COMMA, CHECK_OK); } @@ -1488,16 +1496,20 @@ Statement* Parser::ParseExportDefault(bool* ok) { // 'export' 'default' ClassDeclaration // 'export' 'default' AssignmentExpression[In] ';' + Expect(Token::DEFAULT, CHECK_OK); + Scanner::Location default_loc = scanner()->location(); + + ZoneList names(1, zone()); Statement* result = NULL; switch (peek()) { case Token::FUNCTION: // TODO(ES6): Support parsing anonymous function declarations here. - result = ParseFunctionDeclaration(NULL, CHECK_OK); + result = ParseFunctionDeclaration(&names, CHECK_OK); break; case Token::CLASS: // TODO(ES6): Support parsing anonymous class declarations here. - result = ParseClassDeclaration(NULL, CHECK_OK); + result = ParseClassDeclaration(&names, CHECK_OK); break; default: { @@ -1509,7 +1521,20 @@ Statement* Parser::ParseExportDefault(bool* ok) { } } - // TODO(ES6): Add default export to scope_->module() + const AstRawString* default_string = ast_value_factory()->default_string(); + + DCHECK_LE(names.length(), 1); + if (names.length() == 1) { + scope_->module()->AddLocalExport(default_string, names.first(), zone(), ok); + if (!*ok) { + ParserTraits::ReportMessageAt(default_loc, "duplicate_export", + default_string); + return NULL; + } + } else { + // TODO(ES6): Assign result to a const binding with the name "*default*" + // and add an export entry with "*default*" as the local name. + } return result; } @@ -1528,10 +1553,8 @@ Statement* Parser::ParseExportDeclaration(bool* ok) { Statement* result = NULL; ZoneList names(1, zone()); - bool is_export_from = false; switch (peek()) { case Token::DEFAULT: - Consume(Token::DEFAULT); return ParseExportDefault(ok); case Token::MUL: { @@ -1539,12 +1562,9 @@ Statement* Parser::ParseExportDeclaration(bool* ok) { ExpectContextualKeyword(CStrVector("from"), CHECK_OK); Literal* module = ParseModuleSpecifier(CHECK_OK); ExpectSemicolon(CHECK_OK); - // TODO(ES6): Do something with the return value - // of ParseModuleSpecifier. + // TODO(ES6): scope_->module()->AddStarExport(...) USE(module); - is_export_from = true; - result = factory()->NewEmptyStatement(pos); - break; + return factory()->NewEmptyStatement(pos); } case Token::LBRACE: { @@ -1560,13 +1580,14 @@ Statement* Parser::ParseExportDeclaration(bool* ok) { // encountered, and then throw a SyntaxError if we are in the // non-FromClause case. Scanner::Location reserved_loc = Scanner::Location::invalid(); - ParseExportClause(&names, &reserved_loc, CHECK_OK); + ZoneList export_names(1, zone()); + ZoneList export_locations(1, zone()); + ZoneList local_names(1, zone()); + ParseExportClause(&export_names, &export_locations, &local_names, + &reserved_loc, CHECK_OK); + Literal* indirect_export_module_specifier = NULL; if (CheckContextualKeyword(CStrVector("from"))) { - Literal* module = ParseModuleSpecifier(CHECK_OK); - // TODO(ES6): Do something with the return value - // of ParseModuleSpecifier. - USE(module); - is_export_from = true; + indirect_export_module_specifier = ParseModuleSpecifier(CHECK_OK); } else if (reserved_loc.IsValid()) { // No FromClause, so reserved words are invalid in ExportClause. *ok = false; @@ -1574,8 +1595,25 @@ Statement* Parser::ParseExportDeclaration(bool* ok) { return NULL; } ExpectSemicolon(CHECK_OK); - result = factory()->NewEmptyStatement(pos); - break; + const int length = export_names.length(); + DCHECK_EQ(length, local_names.length()); + DCHECK_EQ(length, export_locations.length()); + if (indirect_export_module_specifier == NULL) { + 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], + "duplicate_export", export_names[i]); + return NULL; + } + } + } else { + for (int i = 0; i < length; ++i) { + // TODO(ES6): scope_->module()->AddIndirectExport(...);( + } + } + return factory()->NewEmptyStatement(pos); } case Token::FUNCTION: @@ -1598,37 +1636,18 @@ Statement* Parser::ParseExportDeclaration(bool* ok) { return NULL; } - // Every export of a module may be assigned. + // Extract declared names into export declarations. + ModuleDescriptor* descriptor = scope_->module(); for (int i = 0; i < names.length(); ++i) { - Variable* var = scope_->Lookup(names[i]); - if (var == NULL) { - // TODO(sigurds) This is an export that has no definition yet, - // not clear what to do in this case. - continue; - } - if (!IsImmutableVariableMode(var->mode())) { - var->set_maybe_assigned(); + descriptor->AddLocalExport(names[i], names[i], zone(), ok); + if (!*ok) { + // TODO(adamk): Possibly report this error at the right place. + ParserTraits::ReportMessage("duplicate_export", names[i]); + return NULL; } } - // TODO(ES6): Handle 'export from' once imports are properly implemented. - // For now we just drop such exports on the floor. - if (!is_export_from) { - // Extract declared names into export declarations and module descriptor. - ModuleDescriptor* descriptor = scope_->module(); - for (int i = 0; i < names.length(); ++i) { - // TODO(adamk): Make early errors here provide the right error message - // (duplicate exported names). - descriptor->Add(names[i], zone(), CHECK_OK); - // TODO(rossberg): Rethink whether we actually need to store export - // declarations (for compilation?). - // ExportDeclaration* declaration = - // factory()->NewExportDeclaration(proxy, scope_, position); - // scope_->AddDeclaration(declaration); - } - } - - DCHECK(result != NULL); + DCHECK_NOT_NULL(result); return result; } diff --git a/src/parser.h b/src/parser.h index 713133a679..4b0e77f606 100644 --- a/src/parser.h +++ b/src/parser.h @@ -713,7 +713,9 @@ class Parser : public ParserBase { Statement* ParseImportDeclaration(bool* ok); Statement* ParseExportDeclaration(bool* ok); Statement* ParseExportDefault(bool* ok); - void* ParseExportClause(ZoneList* names, + void* ParseExportClause(ZoneList* export_names, + ZoneList* export_locations, + ZoneList* local_names, Scanner::Location* reserved_loc, bool* ok); void* ParseNamedImports(ZoneList* names, bool* ok); Statement* ParseStatement(ZoneList* labels, bool* ok); diff --git a/src/scopeinfo.cc b/src/scopeinfo.cc index 74aefdb954..10a493f5d6 100644 --- a/src/scopeinfo.cc +++ b/src/scopeinfo.cc @@ -560,8 +560,8 @@ Handle ModuleInfo::Create(Isolate* isolate, int i = 0; for (ModuleDescriptor::Iterator it = descriptor->iterator(); !it.done(); it.Advance(), ++i) { - Variable* var = scope->LookupLocal(it.name()); - info->set_name(i, *(it.name()->string())); + Variable* var = scope->LookupLocal(it.local_name()); + info->set_name(i, *(it.export_name()->string())); info->set_mode(i, var->mode()); DCHECK(var->index() >= 0); info->set_index(i, var->index()); diff --git a/test/cctest/cctest.status b/test/cctest/cctest.status index ef452a6159..1b43b93116 100644 --- a/test/cctest/cctest.status +++ b/test/cctest/cctest.status @@ -63,9 +63,6 @@ # are actually 13 * 38 * 5 * 128 = 316160 individual tests hidden here. 'test-parsing/ParserSync': [PASS, NO_VARIANTS], - # Modules are busted - 'test-parsing/ExportsMaybeAssigned': [SKIP], - # This tests only the type system, so there is no point in running several # variants. 'test-hydrogen-types/*': [PASS, NO_VARIANTS], diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc index 1a17475ada..a62504f23c 100644 --- a/test/cctest/test-parsing.cc +++ b/test/cctest/test-parsing.cc @@ -3246,70 +3246,6 @@ TEST(IfArgumentsArrayAccessedThenParametersMaybeAssigned) { } -TEST(ExportsMaybeAssigned) { - i::FLAG_use_strict = true; - i::FLAG_harmony_scoping = true; - i::FLAG_harmony_modules = true; - - i::Isolate* isolate = CcTest::i_isolate(); - i::Factory* factory = isolate->factory(); - i::HandleScope scope(isolate); - LocalContext env; - - const char* src = - "module A {" - " export var x = 1;" - " export function f() { return x };" - " export const y = 2;" - " module B {}" - " export module C {}" - "};" - "A.f"; - - i::ScopedVector program(Utf8LengthHelper(src) + 1); - i::SNPrintF(program, "%s", src); - i::Handle source = factory->InternalizeUtf8String(program.start()); - source->PrintOn(stdout); - printf("\n"); - i::Zone zone; - v8::Local v = CompileRun(src); - i::Handle o = v8::Utils::OpenHandle(*v); - i::Handle f = i::Handle::cast(o); - i::Context* context = f->context(); - i::AstValueFactory avf(&zone, isolate->heap()->HashSeed()); - avf.Internalize(isolate); - - i::Scope* script_scope = - new (&zone) i::Scope(&zone, NULL, i::SCRIPT_SCOPE, &avf); - script_scope->Initialize(); - i::Scope* s = - i::Scope::DeserializeScopeChain(isolate, &zone, context, script_scope); - DCHECK(s != script_scope); - const i::AstRawString* name_x = avf.GetOneByteString("x"); - const i::AstRawString* name_f = avf.GetOneByteString("f"); - const i::AstRawString* name_y = avf.GetOneByteString("y"); - const i::AstRawString* name_B = avf.GetOneByteString("B"); - const i::AstRawString* name_C = avf.GetOneByteString("C"); - - // Get result from h's function context (that is f's context) - i::Variable* var_x = s->Lookup(name_x); - CHECK(var_x != NULL); - CHECK(var_x->maybe_assigned() == i::kMaybeAssigned); - i::Variable* var_f = s->Lookup(name_f); - CHECK(var_f != NULL); - CHECK(var_f->maybe_assigned() == i::kMaybeAssigned); - i::Variable* var_y = s->Lookup(name_y); - CHECK(var_y != NULL); - CHECK(var_y->maybe_assigned() == i::kNotAssigned); - i::Variable* var_B = s->Lookup(name_B); - CHECK(var_B != NULL); - CHECK(var_B->maybe_assigned() == i::kNotAssigned); - i::Variable* var_C = s->Lookup(name_C); - CHECK(var_C != NULL); - CHECK(var_C->maybe_assigned() == i::kNotAssigned); -} - - TEST(InnerAssignment) { i::Isolate* isolate = CcTest::i_isolate(); i::Factory* factory = isolate->factory(); @@ -5108,6 +5044,7 @@ TEST(BasicImportExportParsing) { "export { yield } from 'm.js'", "export { static } from 'm.js'", "export { let } from 'm.js'", + "var a; export { a as b, a as c };", "import 'somemodule.js';", "import { } from 'm.js';", @@ -5211,6 +5148,10 @@ TEST(ImportExportParsingErrors) { "export { arguments }", "export { arguments as foo }", "var a; export { a, a };", + "var a, b; export { a as b, b };", + "var a, b; export { a as c, b as c };", + "export default function f(){}; export default class C {};", + "export default function f(){}; var a; export { a as default };", "import from;", "import from 'm.js';", diff --git a/test/message/export-duplicate-as.js b/test/message/export-duplicate-as.js new file mode 100644 index 0000000000..49b52d4b17 --- /dev/null +++ b/test/message/export-duplicate-as.js @@ -0,0 +1,9 @@ +// Copyright 2015 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. +// +// MODULE + +var a, b; +export { a as c }; +export { a, b as c }; diff --git a/test/message/export-duplicate-as.out b/test/message/export-duplicate-as.out new file mode 100644 index 0000000000..1726d9491a --- /dev/null +++ b/test/message/export-duplicate-as.out @@ -0,0 +1,7 @@ +# Copyright 2015 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. +*%(basename)s:9: SyntaxError: Duplicate export of 'c' +export { a, b as c }; + ^ +SyntaxError: Duplicate export of 'c' diff --git a/test/message/export-duplicate-default.js b/test/message/export-duplicate-default.js new file mode 100644 index 0000000000..72a54a45f4 --- /dev/null +++ b/test/message/export-duplicate-default.js @@ -0,0 +1,8 @@ +// Copyright 2015 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. +// +// MODULE + +export default function f() {}; +export default class C {}; diff --git a/test/message/export-duplicate-default.out b/test/message/export-duplicate-default.out new file mode 100644 index 0000000000..4c6b97a7a1 --- /dev/null +++ b/test/message/export-duplicate-default.out @@ -0,0 +1,7 @@ +# Copyright 2015 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. +*%(basename)s:8: SyntaxError: Duplicate export of 'default' +export default class C {}; + ^^^^^^^ +SyntaxError: Duplicate export of 'default' diff --git a/test/message/export-duplicate.js b/test/message/export-duplicate.js new file mode 100644 index 0000000000..f45aefe13f --- /dev/null +++ b/test/message/export-duplicate.js @@ -0,0 +1,9 @@ +// Copyright 2015 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. +// +// MODULE + +var a, b; +export { a }; +export { a, b }; diff --git a/test/message/export-duplicate.out b/test/message/export-duplicate.out new file mode 100644 index 0000000000..e88779f580 --- /dev/null +++ b/test/message/export-duplicate.out @@ -0,0 +1,7 @@ +# Copyright 2015 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. +*%(basename)s:9: SyntaxError: Duplicate export of 'a' +export { a, b }; + ^ +SyntaxError: Duplicate export of 'a' diff --git a/test/message/testcfg.py b/test/message/testcfg.py index 5d6ab84663..cfe22f15d7 100644 --- a/test/message/testcfg.py +++ b/test/message/testcfg.py @@ -36,6 +36,7 @@ from testrunner.objects import testcase FLAGS_PATTERN = re.compile(r"//\s+Flags:(.*)") INVALID_FLAGS = ["--enable-slow-asserts"] +MODULE_PATTERN = re.compile(r"^// MODULE$", flags=re.MULTILINE) class MessageTestSuite(testsuite.TestSuite): @@ -63,6 +64,8 @@ class MessageTestSuite(testsuite.TestSuite): for match in flags_match: result += match.strip().split() result += context.mode_flags + if MODULE_PATTERN.search(source): + result.append("--module") result = [x for x in result if x not in INVALID_FLAGS] result.append(os.path.join(self.root, testcase.path + ".js")) return testcase.flags + result