[ES6] Namespace object re-export should be handled as local export
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 May 2016 17:46:09 +0000 (17:46 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 May 2016 17:46:09 +0000 (17:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157806

Reviewed by Mark Lam.

We align the implementation of ExportEntry to the spec; remove Type::Namespace.
This Type::Namespace is used for re-exported namespace object binding. For example,

    import * as namespace from "namespace.js"
    export { namespace }

In the above case, we used ExportEntry(Type::Namespace). In this patch, we drop this
and use normal local export (Type::Local) instead because namespace object actually has
the local binding in the above module environment. And this handling strictly meets the
spec (Sec 15.2.1.16.1 step 11-a-ii-2-b).

And we also clean up the ExportEntry implementation; dropping unnecessary information.
This change fixes the test262/test/language/module-code/instn-star-equality.js crash.

* parser/ModuleAnalyzer.cpp:
(JSC::ModuleAnalyzer::exportVariable):
* runtime/JSModuleRecord.cpp:
(JSC::getExportedNames):
(JSC::JSModuleRecord::dump): Deleted.
* runtime/JSModuleRecord.h:
* tests/modules/namespace-re-export.js: Added.
* tests/modules/namespace-re-export/namespace-re-export-fixture.js: Added.
* tests/modules/namespace-re-export/namespace-re-export.js: Added.
* tests/modules/resources/assert.js:
(export.shouldNotBe):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@201085 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/parser/ModuleAnalyzer.cpp
Source/JavaScriptCore/runtime/JSModuleRecord.cpp
Source/JavaScriptCore/runtime/JSModuleRecord.h
Source/JavaScriptCore/tests/modules/namespace-re-export.js [new file with mode: 0644]
Source/JavaScriptCore/tests/modules/namespace-re-export/namespace-re-export-fixture.js [new file with mode: 0644]
Source/JavaScriptCore/tests/modules/namespace-re-export/namespace-re-export.js [new file with mode: 0644]
Source/JavaScriptCore/tests/modules/resources/assert.js

index c74fce4..eaf37d4 100644 (file)
@@ -1,3 +1,36 @@
+2016-05-18  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [ES6] Namespace object re-export should be handled as local export
+        https://bugs.webkit.org/show_bug.cgi?id=157806
+
+        Reviewed by Mark Lam.
+
+        We align the implementation of ExportEntry to the spec; remove Type::Namespace.
+        This Type::Namespace is used for re-exported namespace object binding. For example,
+
+            import * as namespace from "namespace.js"
+            export { namespace }
+
+        In the above case, we used ExportEntry(Type::Namespace). In this patch, we drop this
+        and use normal local export (Type::Local) instead because namespace object actually has
+        the local binding in the above module environment. And this handling strictly meets the
+        spec (Sec 15.2.1.16.1 step 11-a-ii-2-b).
+
+        And we also clean up the ExportEntry implementation; dropping unnecessary information.
+        This change fixes the test262/test/language/module-code/instn-star-equality.js crash.
+
+        * parser/ModuleAnalyzer.cpp:
+        (JSC::ModuleAnalyzer::exportVariable):
+        * runtime/JSModuleRecord.cpp:
+        (JSC::getExportedNames):
+        (JSC::JSModuleRecord::dump): Deleted.
+        * runtime/JSModuleRecord.h:
+        * tests/modules/namespace-re-export.js: Added.
+        * tests/modules/namespace-re-export/namespace-re-export-fixture.js: Added.
+        * tests/modules/namespace-re-export/namespace-re-export.js: Added.
+        * tests/modules/resources/assert.js:
+        (export.shouldNotBe):
+
 2016-05-17  Filip Pizlo  <fpizlo@apple.com>
 
         JSC should detect the right default locale even when it's not embedded in WebCore
index c1a3cc2..29668e9 100644 (file)
@@ -78,24 +78,27 @@ void ModuleAnalyzer::exportVariable(const RefPtr<UniquedStringImpl>& localName,
 
     // Exported module local variable.
     if (!variable.isImported()) {
-        moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createLocal(exportName, Identifier::fromUid(m_vm, localName.get()), variable));
+        moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createLocal(exportName, Identifier::fromUid(m_vm, localName.get())));
         return;
     }
 
-    Optional<JSModuleRecord::ImportEntry> optionalImportEntry = moduleRecord()->tryGetImportEntry(localName.get());
-    ASSERT(optionalImportEntry);
-    const JSModuleRecord::ImportEntry& importEntry = *optionalImportEntry;
     if (variable.isImportedNamespace()) {
         // Exported namespace binding.
         // import * as namespace from "mod"
         // export { namespace }
-        moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createNamespace(exportName, importEntry.moduleRequest));
+        //
+        // Sec 15.2.1.16.1 step 11-a-ii-2-b https://tc39.github.io/ecma262/#sec-parsemodule
+        // Namespace export is handled as local export since a namespace object binding itself is implemented as a local binding.
+        moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createLocal(exportName, Identifier::fromUid(m_vm, localName.get())));
         return;
     }
 
     // Indirectly exported binding.
     // import a from "mod"
     // export { a }
+    Optional<JSModuleRecord::ImportEntry> optionalImportEntry = moduleRecord()->tryGetImportEntry(localName.get());
+    ASSERT(optionalImportEntry);
+    const JSModuleRecord::ImportEntry& importEntry = *optionalImportEntry;
     moduleRecord()->addExportEntry(JSModuleRecord::ExportEntry::createIndirect(exportName, importEntry.importName, importEntry.moduleRequest));
 }
 
index 7b7b323..0da9520 100644 (file)
@@ -106,19 +106,14 @@ auto JSModuleRecord::tryGetExportEntry(UniquedStringImpl* exportName) -> Optiona
     return Optional<ExportEntry>(iterator->value);
 }
 
-auto JSModuleRecord::ExportEntry::createLocal(const Identifier& exportName, const Identifier& localName, const VariableEnvironmentEntry& variable) -> ExportEntry
+auto JSModuleRecord::ExportEntry::createLocal(const Identifier& exportName, const Identifier& localName) -> ExportEntry
 {
-    return ExportEntry { Type::Local, exportName, Identifier(), Identifier(), localName, variable };
-}
-
-auto JSModuleRecord::ExportEntry::createNamespace(const Identifier& exportName, const Identifier& moduleName) -> ExportEntry
-{
-    return ExportEntry { Type::Namespace, exportName, moduleName, Identifier(), Identifier(), VariableEnvironmentEntry() };
+    return ExportEntry { Type::Local, exportName, Identifier(), Identifier(), localName };
 }
 
 auto JSModuleRecord::ExportEntry::createIndirect(const Identifier& exportName, const Identifier& importName, const Identifier& moduleName) -> ExportEntry
 {
-    return ExportEntry { Type::Indirect, exportName, moduleName, importName, Identifier(), VariableEnvironmentEntry() };
+    return ExportEntry { Type::Indirect, exportName, moduleName, importName, Identifier() };
 }
 
 auto JSModuleRecord::Resolution::notFound() -> Resolution
@@ -581,8 +576,8 @@ auto JSModuleRecord::resolveExportImpl(ExecState* exec, const ResolveQuery& root
 
             const ExportEntry& exportEntry = *optionalExportEntry;
             switch (exportEntry.type) {
-            case ExportEntry::Type::Local:
-            case ExportEntry::Type::Namespace: {
+            case ExportEntry::Type::Local: {
+                ASSERT(!exportEntry.localName.isNull());
                 Resolution resolution { Resolution::Type::Resolved, moduleRecord, exportEntry.localName };
                 //  2. A module that has resolved a local binding is always cacheable.
                 cacheResolutionForQuery(query, resolution);
@@ -673,16 +668,8 @@ static void getExportedNames(ExecState* exec, JSModuleRecord* root, IdentifierSe
 
         for (const auto& pair : moduleRecord->exportEntries()) {
             const JSModuleRecord::ExportEntry& exportEntry = pair.value;
-            switch (exportEntry.type) {
-            case JSModuleRecord::ExportEntry::Type::Local:
-            case JSModuleRecord::ExportEntry::Type::Indirect:
-                if (moduleRecord == root || exec->propertyNames().defaultKeyword != exportEntry.exportName)
-                    exportedNames.add(exportEntry.exportName.impl());
-                break;
-
-            case JSModuleRecord::ExportEntry::Type::Namespace:
-                break;
-            }
+            if (moduleRecord == root || exec->propertyNames().defaultKeyword != exportEntry.exportName)
+                exportedNames.add(exportEntry.exportName.impl());
         }
 
         for (const auto& starModuleName : moduleRecord->starExportEntries()) {
@@ -891,10 +878,6 @@ void JSModuleRecord::dump()
             dataLog("      [Local] ", "export(", printableName(exportEntry.exportName), "), local(", printableName(exportEntry.localName), ")\n");
             break;
 
-        case ExportEntry::Type::Namespace:
-            dataLog("      [Namespace] ", "export(", printableName(exportEntry.exportName), "), module(", printableName(exportEntry.moduleName), ")\n");
-            break;
-
         case ExportEntry::Type::Indirect:
             dataLog("      [Indirect] ", "export(", printableName(exportEntry.exportName), "), import(", printableName(exportEntry.importName), "), module(", printableName(exportEntry.moduleName), ")\n");
             break;
index b82a9e1..9313409 100644 (file)
@@ -48,15 +48,14 @@ class JSModuleRecord : public JSDestructibleObject {
 public:
     typedef JSDestructibleObject Base;
 
+    // https://tc39.github.io/ecma262/#sec-source-text-module-records
     struct ExportEntry {
         enum class Type {
             Local,
-            Namespace,
             Indirect
         };
 
-        static ExportEntry createLocal(const Identifier& exportName, const Identifier& localName, const VariableEnvironmentEntry&);
-        static ExportEntry createNamespace(const Identifier& exportName, const Identifier& moduleName);
+        static ExportEntry createLocal(const Identifier& exportName, const Identifier& localName);
         static ExportEntry createIndirect(const Identifier& exportName, const Identifier& importName, const Identifier& moduleName);
 
         Type type;
@@ -64,7 +63,6 @@ public:
         Identifier moduleName;
         Identifier importName;
         Identifier localName;
-        VariableEnvironmentEntry variable;
     };
 
     struct ImportEntry {
diff --git a/Source/JavaScriptCore/tests/modules/namespace-re-export.js b/Source/JavaScriptCore/tests/modules/namespace-re-export.js
new file mode 100644 (file)
index 0000000..9c74e61
--- /dev/null
@@ -0,0 +1 @@
+import "namespace-re-export/namespace-re-export.js";
diff --git a/Source/JavaScriptCore/tests/modules/namespace-re-export/namespace-re-export-fixture.js b/Source/JavaScriptCore/tests/modules/namespace-re-export/namespace-re-export-fixture.js
new file mode 100644 (file)
index 0000000..9c71bcc
--- /dev/null
@@ -0,0 +1,2 @@
+import * as namespace from './namespace-re-export.js';
+export { namespace };
diff --git a/Source/JavaScriptCore/tests/modules/namespace-re-export/namespace-re-export.js b/Source/JavaScriptCore/tests/modules/namespace-re-export/namespace-re-export.js
new file mode 100644 (file)
index 0000000..4e24963
--- /dev/null
@@ -0,0 +1,11 @@
+import { shouldBe, shouldNotBe } from "../resources/assert.js";
+import * as self1 from './namespace-re-export.js';
+import * as other1 from './namespace-re-export-fixture.js';
+import { namespace } from './namespace-re-export-fixture.js';
+
+// Re-exported namespace objects
+shouldBe(self1, namespace);
+shouldNotBe(self1, other1);
+
+// Re-exported namespace binding should reside in the namespace-re-export-fixture's namespace object.
+shouldBe('namespace' in other1, true);
index a006f84..fbf3bd5 100644 (file)
@@ -3,6 +3,11 @@ export function shouldBe(actual, expected) {
         throw new Error(`bad value: ${String(actual)}`);
 }
 
+export function shouldNotBe(actual, expected) {
+    if (actual === expected)
+        throw new Error(`bad value: ${String(actual)}`);
+}
+
 export function shouldThrow(func, errorMessage) {
     var errorThrown = false;
     var error = null;