fix module loader cache (#5278)

Fixes #5270.

This makes SimpleModuleLoader cache modules with their import attributes
instead of only by path.
It also adds a regression test for the json case where the same path is
imported once with attributes and later without them.

Tests:
- cargo test -p boa_engine --test imports -- --nocapture
- cargo test -p boa_engine --test module -- --nocapture
- cargo clippy -p boa_engine --all-features --all-targets -- -D warnings

Author: Monti-27

core/engine/src/module/loader/mod.rs

@@ -287,6 +287,23 @@ impl ModuleLoader for MapModuleLoader {
     }
 }
 
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+struct ModuleCacheKey {
+    path: PathBuf,
+    attributes: Box<[ImportAttribute]>,
+}
+
+impl ModuleCacheKey {
+    fn new(path: PathBuf, attributes: &[ImportAttribute]) -> Self {
+        let mut attributes = attributes.to_vec();
+        attributes.sort_unstable_by(|left, right| left.key().cmp(right.key()));
+        Self {
+            path,
+            attributes: attributes.into_boxed_slice(),
+        }
+    }
+}
+
 /// A simple module loader that loads modules relative to a root path.
 ///
 /// # Note
@@ -297,7 +314,7 @@ impl ModuleLoader for MapModuleLoader {
 #[derive(Debug)]
 pub struct SimpleModuleLoader {
     root: PathBuf,
-    module_map: GcRefCell<FxHashMap<PathBuf, Module>>,
+    module_map: GcRefCell<FxHashMap<ModuleCacheKey, Module>>,
 }
 
 impl SimpleModuleLoader {
@@ -323,38 +340,39 @@ impl SimpleModuleLoader {
     /// Inserts a new module onto the module map.
     #[inline]
     pub fn insert(&self, path: PathBuf, module: Module) {
-        self.module_map.borrow_mut().insert(path, module);
+        self.insert_with_attributes(path, &[], module);
     }
 
     /// Inserts a new module onto the module map with the given attributes.
-    ///
-    /// This is an alias for `insert` in this implementation, as it ignores attributes.
     #[inline]
     pub fn insert_with_attributes(
         &self,
         path: PathBuf,
-        _attributes: &[ImportAttribute],
+        attributes: &[ImportAttribute],
         module: Module,
     ) {
-        self.insert(path, module);
+        self.module_map
+            .borrow_mut()
+            .insert(ModuleCacheKey::new(path, attributes), module);
     }
 
     /// Gets a module from its original path.
     #[inline]
     pub fn get(&self, path: &Path) -> Option<Module> {
-        self.module_map.borrow().get(path).cloned()
+        self.get_with_attributes(path, &[])
     }
 
     /// Gets a module from its original path and attributes.
-    ///
-    /// This is an alias for `get` in this implementation, as it ignores attributes.
     #[inline]
     pub fn get_with_attributes(
         &self,
         path: &Path,
-        _attributes: &[ImportAttribute],
+        attributes: &[ImportAttribute],
     ) -> Option<Module> {
-        self.get(path)
+        self.module_map
+            .borrow()
+            .get(&ModuleCacheKey::new(path.to_path_buf(), attributes))
+            .cloned()
     }
 }
 

core/engine/tests/assets/data.json

@@ -0,0 +1 @@
+{ "ok": true }

core/engine/tests/imports.rs

@@ -5,7 +5,7 @@ use std::rc::Rc;
 
 use boa_engine::builtins::promise::PromiseState;
 use boa_engine::module::SimpleModuleLoader;
-use boa_engine::{Context, JsValue, Source, js_string};
+use boa_engine::{Context, JsString, JsValue, Source, js_string};
 
 /// Test that relative imports work with the simple module loader.
 #[test]
@@ -45,3 +45,63 @@ fn subdirectories() {
         }
     }
 }
+
+#[test]
+fn json_import_attributes_are_part_of_the_cache_key() {
+    let assets_dir =
+        PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap()).join("tests/assets");
+
+    let loader = Rc::new(SimpleModuleLoader::new(assets_dir).unwrap());
+    let mut context = Context::builder()
+        .module_loader(loader.clone())
+        .build()
+        .unwrap();
+
+    let source = Source::from_bytes(
+        b"
+        import json from 'data.json' with { type: 'json' };
+        export let value = json;
+        export let p = import('data.json');
+    ",
+    );
+    let module = boa_engine::Module::parse(source, None, &mut context).unwrap();
+    let result = module.load_link_evaluate(&mut context);
+
+    context.run_jobs().unwrap();
+    assert_eq!(
+        result.state(),
+        PromiseState::Fulfilled(JsValue::undefined())
+    );
+
+    let value = module
+        .namespace(&mut context)
+        .get(js_string!("value"), &mut context)
+        .unwrap();
+    assert_eq!(
+        JsString::from(value.to_json(&mut context).unwrap().unwrap().to_string()),
+        js_string!(r#"{"ok":true}"#)
+    );
+
+    let p = module
+        .namespace(&mut context)
+        .get(js_string!("p"), &mut context)
+        .unwrap();
+    let p_obj = p.as_promise().unwrap();
+    context.run_jobs().unwrap();
+
+    match p_obj.state() {
+        PromiseState::Rejected(e) => {
+            let error = e
+                .as_object()
+                .expect("expected rejection to be an Error object");
+            let name = error.get(js_string!("name"), &mut context).unwrap();
+            assert_eq!(name.as_string().unwrap(), js_string!("TypeError"));
+            let message = error.get(js_string!("message"), &mut context).unwrap();
+            assert_eq!(
+                message.as_string().unwrap(),
+                js_string!("module `data.json` needs an import attribute of type \"json\"")
+            );
+        }
+        state => panic!("expected dynamic import to reject, got {state:?}"),
+    }
+}