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.

5 files changed, 85 insertions, 17 deletions

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<Value> 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<bool> ModuleEnvironment::has_binding(FlyString const& name, Optional<size_t>* 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<bool> ModuleEnvironment::delete_binding(VM&, FlyString const&)
 {
@@ -102,4 +91,39 @@ ModuleEnvironment::IndirectBinding const* ModuleEnvironment::get_indirect_bindin
     return &(*binding_or_end);
 }
 
+Optional<ModuleEnvironment::BindingAndIndex> 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<ModuleEnvironment>(target_env));
+        auto& target_module_environment = static_cast<ModuleEnvironment&>(*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<Value> get_this_binding(VM&) const final;
     ThrowCompletionOr<void> 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<bool> has_binding(FlyString const& name, Optional<size_t>* = 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<BindingAndIndex> find_binding_and_index(FlyString const& name) const override;
+
     // FIXME: Since we always access this via the name this could be a map.
     Vector<IndirectBinding> 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", () => {