From 252df0a1db54ea87695bfc74fc50ea21ebbd6537 Mon Sep 17 00:00:00 2001 From: phosit Date: Wed, 15 Jan 2025 21:34:58 +0100 Subject: [PATCH] Return the namespace of a module To make it easy for the engine to access the exported values the namespace is returned from the future. --- .../_test.scriptinterface/module/export.js | 6 + .../export_default/does_not_work_around.js | 3 + .../module/export_default/immutable.js | 1 + .../module/export_default/invalid.js | 2 + .../module/export_default/works_around.js | 3 + .../_test.scriptinterface/module/indirect.js | 1 + eslint.config.mjs | 1 + source/scriptinterface/ModuleLoader.cpp | 26 ++- source/scriptinterface/ModuleLoader.h | 10 +- source/scriptinterface/tests/test_Module.h | 161 +++++++++++++++++- 10 files changed, 206 insertions(+), 8 deletions(-) create mode 100644 binaries/data/mods/_test.scriptinterface/module/export.js create mode 100644 binaries/data/mods/_test.scriptinterface/module/export_default/does_not_work_around.js create mode 100644 binaries/data/mods/_test.scriptinterface/module/export_default/immutable.js create mode 100644 binaries/data/mods/_test.scriptinterface/module/export_default/invalid.js create mode 100644 binaries/data/mods/_test.scriptinterface/module/export_default/works_around.js create mode 100644 binaries/data/mods/_test.scriptinterface/module/indirect.js diff --git a/binaries/data/mods/_test.scriptinterface/module/export.js b/binaries/data/mods/_test.scriptinterface/module/export.js new file mode 100644 index 0000000000..dbcbcfefc7 --- /dev/null +++ b/binaries/data/mods/_test.scriptinterface/module/export.js @@ -0,0 +1,6 @@ +export let value = 6; + +export function mutate(newValue) +{ + value = newValue; +} diff --git a/binaries/data/mods/_test.scriptinterface/module/export_default/does_not_work_around.js b/binaries/data/mods/_test.scriptinterface/module/export_default/does_not_work_around.js new file mode 100644 index 0000000000..e0cf6134d5 --- /dev/null +++ b/binaries/data/mods/_test.scriptinterface/module/export_default/does_not_work_around.js @@ -0,0 +1,3 @@ +let value = 6; +export default value; +value = 36; diff --git a/binaries/data/mods/_test.scriptinterface/module/export_default/immutable.js b/binaries/data/mods/_test.scriptinterface/module/export_default/immutable.js new file mode 100644 index 0000000000..0d9d3bea7e --- /dev/null +++ b/binaries/data/mods/_test.scriptinterface/module/export_default/immutable.js @@ -0,0 +1 @@ +export default 36; diff --git a/binaries/data/mods/_test.scriptinterface/module/export_default/invalid.js b/binaries/data/mods/_test.scriptinterface/module/export_default/invalid.js new file mode 100644 index 0000000000..dc9dbaa02d --- /dev/null +++ b/binaries/data/mods/_test.scriptinterface/module/export_default/invalid.js @@ -0,0 +1,2 @@ +export default let value = 6; +value = 36; diff --git a/binaries/data/mods/_test.scriptinterface/module/export_default/works_around.js b/binaries/data/mods/_test.scriptinterface/module/export_default/works_around.js new file mode 100644 index 0000000000..028909f8fd --- /dev/null +++ b/binaries/data/mods/_test.scriptinterface/module/export_default/works_around.js @@ -0,0 +1,3 @@ +let value = 6; +export { value as default }; +value = 36; diff --git a/binaries/data/mods/_test.scriptinterface/module/indirect.js b/binaries/data/mods/_test.scriptinterface/module/indirect.js new file mode 100644 index 0000000000..42be15c741 --- /dev/null +++ b/binaries/data/mods/_test.scriptinterface/module/indirect.js @@ -0,0 +1 @@ +export * from "export.js"; diff --git a/eslint.config.mjs b/eslint.config.mjs index 84661a4863..6517d58865 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -15,6 +15,7 @@ const configIgnores = { // This files deliberately contain errors "binaries/data/mods/_test.scriptinterface/module/import_inside_function.js", + "binaries/data/mods/_test.scriptinterface/module/export_default/invalid.js", ], }; diff --git a/source/scriptinterface/ModuleLoader.cpp b/source/scriptinterface/ModuleLoader.cpp index f5f41fe568..0d80bd038e 100644 --- a/source/scriptinterface/ModuleLoader.cpp +++ b/source/scriptinterface/ModuleLoader.cpp @@ -108,7 +108,14 @@ bool Call(JSContext* cx, const unsigned argc, JS::Value* vp) return true; } - status = ModuleLoader::Future::Fulfilled{}; + const auto evaluatingStatus{std::get_if(&status)}; + if (!evaluatingStatus) + { + status = ModuleLoader::Future::Rejected{std::make_exception_ptr(std::runtime_error{ + "Future is not Pending."})}; + return true; + } + status = ModuleLoader::Future::Fulfilled{evaluatingStatus->moduleNamespace}; return true; } @@ -144,15 +151,24 @@ ModuleLoader::CompiledModule::CompiledModule(const ScriptRequest& rq, const VfsP } ModuleLoader::Future::Future(const ScriptRequest& rq, ModuleLoader& loader, const VfsPath& modulePath): - m_Status{Evaluating{{rq.cx, JS_NewObject(rq.cx, &callbackClass)}, + m_Status{Evaluating{{rq.cx, nullptr}, {rq.cx, JS_NewObject(rq.cx, &callbackClass)}, {rq.cx, JS_NewObject(rq.cx, &callbackClass)}}} { + // It's possible to access exported values before the complete module is evaluated (whenever + // something is `export`-ed before a top-level `await`). + // Those "partial" module namespaces are not exposed for the following reasons: + // - The use case for them is too limited. + // - JS developers are used to getting either a complete namespace or nothing. + // - Accessing values which are not yet exported results in an error. These errors might implicitly be + // dropped. + JS::RootedObject mod{rq.cx, CompileModule(rq, loader.m_Registry, modulePath)}; JS::RootedObject promise{rq.cx, Evaluate(rq, mod)}; + Evaluating& evaluatingStatus{std::get(m_Status)}; + evaluatingStatus.moduleNamespace = JS::GetModuleNamespace(rq.cx, mod); SetReservedSlot(JS::PrivateValue(static_cast(&m_Status))); - Evaluating& evaluatingStatus{std::get(m_Status)}; if (!JS::AddPromiseReactions(rq.cx, promise, evaluatingStatus.fulfill, evaluatingStatus.reject)) throw std::runtime_error{"Failed adding promise reaction."}; } @@ -182,10 +198,10 @@ ModuleLoader::Future::~Future() return std::holds_alternative(m_Status) || std::holds_alternative(m_Status); } -void ModuleLoader::Future::Get() +[[nodiscard]] JSObject* ModuleLoader::Future::Get() { if (std::holds_alternative(m_Status)) - return; + return std::get(std::exchange(m_Status, Invalid{})).moduleNamespace; std::exception_ptr error{std::move(std::get(m_Status).error)}; m_Status = Invalid{}; std::rethrow_exception(std::move(error)); diff --git a/source/scriptinterface/ModuleLoader.h b/source/scriptinterface/ModuleLoader.h index 9b8c0fe2a5..19a9ac10cd 100644 --- a/source/scriptinterface/ModuleLoader.h +++ b/source/scriptinterface/ModuleLoader.h @@ -49,10 +49,14 @@ public: public: struct Evaluating { + JS::PersistentRootedObject moduleNamespace; JS::PersistentRootedObject fulfill; JS::PersistentRootedObject reject; }; - struct Fulfilled {}; + struct Fulfilled + { + JS::PersistentRootedObject moduleNamespace; + }; struct Rejected { std::exception_ptr error; @@ -72,8 +76,10 @@ public: /** * Throws if the evaluation of the module failed. + * @return The module namespace. All exported values are a property + * of this object. @c default is a property with name "default". */ - void Get(); + [[nodiscard]] JSObject* Get(); private: // It's save to not require a `JS::HandleValue` here. diff --git a/source/scriptinterface/tests/test_Module.h b/source/scriptinterface/tests/test_Module.h index d451bc8fb2..42b1a32c61 100644 --- a/source/scriptinterface/tests/test_Module.h +++ b/source/scriptinterface/tests/test_Module.h @@ -19,7 +19,9 @@ #include "ps/CLogger.h" #include "ps/Filesystem.h" +#include "scriptinterface/FunctionWrapper.h" #include "scriptinterface/ModuleLoader.h" +#include "scriptinterface/Object.h" #include "scriptinterface/ScriptContext.h" #include "scriptinterface/ScriptInterface.h" @@ -125,6 +127,7 @@ public: TS_ASSERT(!future.IsDone()); g_ScriptContext->RunJobs(); TS_ASSERT(future.IsDone()); + std::ignore = future.Get(); } void test_TopLevelAwaitInfinite() @@ -136,6 +139,7 @@ public: g_ScriptContext->RunJobs(); TS_ASSERT(!future.IsDone()); + TS_ASSERT_THROWS_ANYTHING(std::ignore = future.Get()); } void test_MoveFulfilledFuture() @@ -207,7 +211,162 @@ public: g_ScriptContext->RunJobs(); TS_ASSERT(future.IsDone()); - TS_ASSERT_THROWS_EQUALS(future.Get(), const std::runtime_error& e, e.what(), + TS_ASSERT_THROWS_EQUALS(std::ignore = future.Get(), const std::runtime_error& e, e.what(), "Error: Test reason\n@top_level_throw.js:1:7\n"); } + + void test_Export() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + auto future = script.GetModuleLoader().LoadModule(rq, "export.js"); + + g_ScriptContext->RunJobs(); + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + + { + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "value", value)); + TS_ASSERT_EQUALS(value, 6); + } + + TS_ASSERT(ScriptFunction::CallVoid(rq, moduleValue, "mutate", 12)); + + { + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "value", value)); + TS_ASSERT_EQUALS(value, 12); + } + } + + void test_ExportSame() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + { + auto future = script.GetModuleLoader().LoadModule(rq, "export.js"); + g_ScriptContext->RunJobs(); + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + TS_ASSERT(ScriptFunction::CallVoid(rq, moduleValue, "mutate", 12)); + } + + { + auto future = script.GetModuleLoader().LoadModule(rq, "include/../export.js"); + g_ScriptContext->RunJobs(); + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "value", value)); + TS_ASSERT_EQUALS(value, 12); + } + } + + void test_ExportIndirect() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + { + auto future = script.GetModuleLoader().LoadModule(rq, "export.js"); + g_ScriptContext->RunJobs(); + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + TS_ASSERT(ScriptFunction::CallVoid(rq, moduleValue, "mutate", 12)); + } + + { + auto future = script.GetModuleLoader().LoadModule(rq, "indirect.js"); + g_ScriptContext->RunJobs(); + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "value", value)); + TS_ASSERT_EQUALS(value, 12); + } + } + + void test_ExportDefaultImmutable() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + auto future = script.GetModuleLoader().LoadModule(rq, "export_default/immutable.js"); + + g_ScriptContext->RunJobs(); + + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "default", value)); + TS_ASSERT_EQUALS(value, 36); + } + + void test_ExportDefaultInvalid() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + TestLogger logger; + TS_ASSERT_THROWS(std::ignore = script.GetModuleLoader().LoadModule(rq, + "export_default/invalid.js"), const std::invalid_argument&); + const std::string log{logger.GetOutput()}; + TS_ASSERT_STR_CONTAINS(log, "export_default/invalid.js line 1"); + } + + void test_ExportDefaultDoesNotWorkAround() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + auto future = script.GetModuleLoader().LoadModule(rq, "export_default/does_not_work_around.js"); + + g_ScriptContext->RunJobs(); + + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "default", value)); + TS_ASSERT_DIFFERS(value, 36); + TS_ASSERT_EQUALS(value, 6); + } + + void test_ExportDefaultWorksAround() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + auto future = script.GetModuleLoader().LoadModule(rq, "export_default/works_around.js"); + + g_ScriptContext->RunJobs(); + + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "default", value)); + TS_ASSERT_EQUALS(value, 36); + } + + void test_ReplaceEvaluatingFuture() + { + ScriptInterface script{"Test", "Test", g_ScriptContext}; + const ScriptRequest rq{script}; + + auto future = script.GetModuleLoader().LoadModule(rq, "top_level_await_finite.js"); + future = script.GetModuleLoader().LoadModule(rq, "export.js"); + + g_ScriptContext->RunJobs(); + JS::RootedObject ns{rq.cx, future.Get()}; + JS::RootedValue moduleValue{rq.cx, JS::ObjectValue(*ns)}; + + int value{0}; + TS_ASSERT(Script::GetProperty(rq, moduleValue, "value", value)); + TS_ASSERT_EQUALS(value, 6); + } };