From 0fc67ffd623d62797fe81da215045b599b0f5661 Mon Sep 17 00:00:00 2001 From: davidot Date: Thu, 1 Sep 2022 23:48:57 +0200 Subject: [PATCH] LibJS: Make indirect bindings of module behave like normal bindings Before this we attempted to hack around this by only overriding has_binding. However this did not cover all cases, for example when assigning to variables before their declaration it didn't throw. By using the new find_binding_and_index virtual method we can just pretend the indirect bindings are real. Since indirect binding do come from a normal environment we need to ensure you cannot modify the binding and that properties like mutable are false as expected by the spec for such an indirect binding. --- .../LibJS/Runtime/ModuleEnvironment.cpp | 46 ++++++++++++++----- .../LibJS/Runtime/ModuleEnvironment.h | 8 +--- .../accessing-lex-import-before-decl.mjs | 23 ++++++++++ .../accessing-var-import-before-decl.mjs | 17 +++++++ .../LibJS/Tests/modules/basic-modules.js | 8 ++++ 5 files changed, 85 insertions(+), 17 deletions(-) create mode 100644 Userland/Libraries/LibJS/Tests/modules/accessing-lex-import-before-decl.mjs create mode 100644 Userland/Libraries/LibJS/Tests/modules/accessing-var-import-before-decl.mjs diff --git a/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.cpp b/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.cpp index 47d4d1ddb8..8843c8e71d 100644 --- a/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.cpp +++ b/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.cpp @@ -48,17 +48,6 @@ ThrowCompletionOr ModuleEnvironment::get_binding_value(VM& vm, FlyString return DeclarativeEnvironment::get_binding_value(vm, name, strict); } -// Not defined in the spec, see comment in the header. -ThrowCompletionOr ModuleEnvironment::has_binding(FlyString const& name, Optional* out_index) const -{ - // 1. If envRec has a binding for the name that is the value of N, return true. - if (get_indirect_binding(name)) - return true; - return DeclarativeEnvironment::has_binding(name, out_index); - - // 2. Return false. -} - // 9.1.1.5.2 DeleteBinding ( N ), https://tc39.es/ecma262/#sec-module-environment-records-deletebinding-n ThrowCompletionOr ModuleEnvironment::delete_binding(VM&, FlyString const&) { @@ -102,4 +91,39 @@ ModuleEnvironment::IndirectBinding const* ModuleEnvironment::get_indirect_bindin return &(*binding_or_end); } +Optional ModuleEnvironment::find_binding_and_index(FlyString const& name) const +{ + auto* indirect_binding = get_indirect_binding(name); + if (indirect_binding != nullptr) { + auto* target_env = indirect_binding->module->environment(); + if (!target_env) + return {}; + + VERIFY(is(target_env)); + auto& target_module_environment = static_cast(*target_env); + auto result = target_module_environment.find_binding_and_index(indirect_binding->binding_name); + if (!result.has_value()) + return {}; + + // NOTE: We must pretend this binding is actually from this environment + // so as specified by + // 9.1.1.5.5 CreateImportBinding ( N, M, N2 ), https://tc39.es/ecma262/#sec-createimportbinding + // It creates a new initialized immutable indirect binding for the + // name N. A binding must not already exist in this Environment + // Record for N. N2 is the name of a binding that exists in M's + // Module Environment Record. Accesses to the value of the new + // binding will indirectly access the bound value of the target + // binding. + // We don't alter the name of the binding as the name is only used + // for lookup. + Binding copy_binding = result->binding(); + copy_binding.mutable_ = false; + copy_binding.can_be_deleted = false; + copy_binding.initialized = true; + return BindingAndIndex { copy_binding }; + } + + return DeclarativeEnvironment::find_binding_and_index(name); +} + } diff --git a/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.h b/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.h index 6b1b13e3f3..f2652165ad 100644 --- a/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.h +++ b/Userland/Libraries/LibJS/Runtime/ModuleEnvironment.h @@ -27,12 +27,6 @@ public: virtual ThrowCompletionOr get_this_binding(VM&) const final; ThrowCompletionOr create_import_binding(FlyString name, Module* module, FlyString binding_name); - // Note: Although the spec does not explicitly say this we also have to implement HasBinding as - // the HasBinding method of Declarative Environment records states: - // "It determines if the argument identifier is one of the identifiers bound by the record" - // And this means that we have to include the indirect bindings of a Module Environment. - virtual ThrowCompletionOr has_binding(FlyString const& name, Optional* = nullptr) const override; - private: explicit ModuleEnvironment(Environment* outer_environment); @@ -43,6 +37,8 @@ private: }; IndirectBinding const* get_indirect_binding(FlyString const& name) const; + virtual Optional find_binding_and_index(FlyString const& name) const override; + // FIXME: Since we always access this via the name this could be a map. Vector m_indirect_bindings; }; diff --git a/Userland/Libraries/LibJS/Tests/modules/accessing-lex-import-before-decl.mjs b/Userland/Libraries/LibJS/Tests/modules/accessing-lex-import-before-decl.mjs new file mode 100644 index 0000000000..1b96bf8aef --- /dev/null +++ b/Userland/Libraries/LibJS/Tests/modules/accessing-lex-import-before-decl.mjs @@ -0,0 +1,23 @@ +let passed = true; + +try { + importedLexVariable; + passed = false; +} catch (e) { + if (!(e instanceof ReferenceError)) + throw new Error("Expected importedLexVariable; to throw ReferenceError got " + e); +} + +try { + // Even though value is let, this should still throw TypeError because it is immutable! + importedLexVariable = 0; + passed = false; +} catch (e) { + if (!(e instanceof TypeError)) + throw new Error("Expected importedLexVariable = 0; to throw TypeError got " + e); +} + +import { value as importedLexVariable } from "./accessing-lex-import-before-decl.mjs"; +export let value = 123; + +export { passed }; diff --git a/Userland/Libraries/LibJS/Tests/modules/accessing-var-import-before-decl.mjs b/Userland/Libraries/LibJS/Tests/modules/accessing-var-import-before-decl.mjs new file mode 100644 index 0000000000..44410c82df --- /dev/null +++ b/Userland/Libraries/LibJS/Tests/modules/accessing-var-import-before-decl.mjs @@ -0,0 +1,17 @@ +let passed = true; + +if (importedVarValue !== undefined) + throw new Error("Expected importedVarValue === undefined; got " + importedVarValue); + +try { + importedVarValue = 0; + passed = false; +} catch (e) { + if (!(e instanceof TypeError)) + throw new Error("Expected importedVarValue = 0; to throw TypeError got " + e); +} + +import { value as importedVarValue } from "./accessing-var-import-before-decl.mjs"; +export var value = 123; + +export { passed }; diff --git a/Userland/Libraries/LibJS/Tests/modules/basic-modules.js b/Userland/Libraries/LibJS/Tests/modules/basic-modules.js index cf662213fd..f14554a1ee 100644 --- a/Userland/Libraries/LibJS/Tests/modules/basic-modules.js +++ b/Userland/Libraries/LibJS/Tests/modules/basic-modules.js @@ -194,6 +194,14 @@ describe("in- and exports", () => { test("can export namespace via binding", () => { expectModulePassed("./re-export-namespace-via-binding.mjs"); }); + + test("import variable before import statement behaves as undefined and non mutable variable", () => { + expectModulePassed("./accessing-var-import-before-decl.mjs"); + }); + + test("import lexical binding before import statement behaves as initialized but non mutable binding", () => { + expectModulePassed("./accessing-lex-import-before-decl.mjs"); + }); }); describe("loops", () => {