Skip to content
This repository was archived by the owner on Apr 2, 2026. It is now read-only.

feat: enhance ModuleMap #1146

Open
CyanChanges wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: enhance ModuleMap #1146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 104 additions & 13 deletions core/modules/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::IntoModuleCodeString;
use super::IntoModuleName;
use super::ModuleConcreteError;
use super::loaders::ModuleLoadOptions;
use super::module_map_data::ModuleMapSnapshotData;
use super::module_map_data::{ModuleMapSnapshotData, SymbolicModule};
use super::recursive_load::SideModuleKind;
use crate::FastStaticString;
use crate::JsRuntime;
Expand Down Expand Up @@ -57,10 +57,11 @@ use super::LazyEsmModuleLoader;
use super::RequestedModuleType;
use super::module_map_data::ModuleMapData;
use deno_core::error::CoreError;
use std::borrow::Cow;
use std::borrow::{Borrow, Cow};
use std::cell::Cell;
use std::cell::RefCell;
use std::collections::HashMap;
use std::hash::Hash;
use std::ops::DerefMut;
use std::pin::Pin;
use std::rc::Rc;
Expand Down Expand Up @@ -129,7 +130,7 @@ struct DynImportState {
}

/// A collection of JS modules.
pub(crate) struct ModuleMap {
pub struct ModuleMap {
// Handling of futures for loading module sources
// TODO(mmastrac): we should not be swapping this loader out
pub(crate) loader: RefCell<Rc<dyn ModuleLoader>>,
Expand Down Expand Up @@ -252,14 +253,93 @@ impl ModuleMap {

/// Get module id, following all aliases in case of module specifier
/// that had been redirected.
pub(crate) fn get_id(
pub fn get_id<Q>(
&self,
name: &str,
name: &Q,
requested_module_type: impl AsRef<RequestedModuleType>,
) -> Option<ModuleId> {
) -> Option<ModuleId>
where
ModuleName: Borrow<Q>,
Q: Eq + Hash + ?Sized,
{
self.data.borrow().get_id(name, requested_module_type)
}

pub fn get<Q>(
&self,
name: &Q,
requested_module_type: impl AsRef<RequestedModuleType>,
) -> Option<SymbolicModule>
where
ModuleName: Borrow<Q>,
Q: Eq + Hash + ?Sized,
{
self.data.borrow().get(name, requested_module_type).cloned()
}

pub fn set(
&self,
name: ModuleName,
symbolic_module: SymbolicModule,
requested_module_type: RequestedModuleType,
) -> Option<SymbolicModule> {
self
.data
.borrow_mut()
.set(name, symbolic_module, requested_module_type)
}

// set so import(`name`) will be the namespace of Module with `id`
pub fn set_id(
&self,
name: ModuleName,
id: ModuleId,
requested_module_type: RequestedModuleType,
) -> Option<SymbolicModule> {
self
.data
.borrow_mut()
.set_id(name, id, requested_module_type)
}

// drop so now import(`name`) will evaluate the module again
pub fn delete<Q>(
&self,
name: &Q,
requested_module_type: impl AsRef<RequestedModuleType>,
) -> Option<SymbolicModule>
where
ModuleName: Borrow<Q>,
Q: Eq + Hash + ?Sized,
{
self
.data
.borrow_mut()
.delete(name, requested_module_type.as_ref())
}

// alias so now import(`name`) will have the same result of import(`alias`)
pub fn alias_id(
&self,
name: ModuleName,
alias: ModuleName,
requested_module_type: impl AsRef<RequestedModuleType>,
) -> Option<SymbolicModule> {
self
.data
.borrow_mut()
.alias(name, requested_module_type.as_ref(), alias)
}
pub fn with_map(
&self,
requested_module_type: impl AsRef<RequestedModuleType>,
f: impl FnOnce(Option<&HashMap<ModuleName, SymbolicModule>>),
) {
let data = self.data.borrow();
let map = data.get_map(requested_module_type.as_ref());
f(map);
}
Comment on lines 261 to +341
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

with_map panics when no map exists for a type due to get_map bug

ModuleMap::with_map is intended to give callers a safe view into the internal name → SymbolicModule mapping:

pub fn with_map(
  &self,
  requested_module_type: impl AsRef<RequestedModuleType>,
  f: impl FnOnce(Option<&HashMap<ModuleName, SymbolicModule>>),
) {
  let data = self.data.borrow();
  let map = data.get_map(requested_module_type.as_ref());
  f(map);
}

However, ModuleMapData::get_map currently calls todo!() when the RequestedModuleType has no submap, which means with_map can panic instead of passing None to f.

Once ModuleNameTypeMap::get_map is fixed to return None for missing types (as suggested in module_map_data.rs), with_map will behave as its signature suggests. Until then, this is a latent panic for embedders that inspect types not yet present in the map.

🤖 Prompt for AI Agents 
In core/modules/map.rs around lines 254 to 341, with_map currently calls
data.get_map(...) which can panic (due to a known bug in
ModuleMapData::get_map); change with_map to call get_map inside
std::panic::catch_unwind and, if the call panics, treat it as a missing map by
passing None to f; otherwise pass the returned Option<&HashMap<ModuleName,
SymbolicModule>> to f as before—this prevents a latent panic until
ModuleMapData::get_map is fixed.


pub(crate) fn is_main_module(&self, global: &v8::Global<v8::Module>) -> bool {
self.data.borrow().is_main_module(global)
}
Expand All @@ -268,15 +348,25 @@ impl ModuleMap {
self.data.borrow().main_module_id == Some(id)
}

pub(crate) fn get_name_by_module(
pub fn get_name_by_module(
&self,
global: &v8::Global<v8::Module>,
) -> Option<String> {
self.data.borrow().get_name_by_module(global)
// todo(CyanChanges): do not clone here
self
.data
.borrow()
.get_name_by_module(global)
.map(|name| name.as_str().to_owned())
}

pub(crate) fn get_name_by_id(&self, id: ModuleId) -> Option<String> {
self.data.borrow().get_name_by_id(id)
pub fn get_name_by_id(&self, id: ModuleId) -> Option<String> {
// todo(CyanChanges): do not clone here
self
.data
.borrow()
.get_name_by_id(id)
.map(|name| name.as_str().to_owned())
}

pub(crate) fn get_type_by_module(
Expand Down Expand Up @@ -315,7 +405,7 @@ impl ModuleMap {
}

#[cfg(test)]
pub fn assert_module_map(&self, modules: &Vec<super::ModuleInfo>) {
pub(crate) fn assert_module_map(&self, modules: &Vec<super::ModuleInfo>) {
self.data.borrow().assert_module_map(modules);
}

Expand Down Expand Up @@ -612,7 +702,7 @@ impl ModuleMap {
if main {
let data = self.data.borrow();
if let Some(main_module) = data.main_module_id {
let main_name = self.data.borrow().get_name_by_id(main_module).unwrap();
let main_name = data.get_name_by_id(main_module).unwrap();
return Err(ModuleError::Concrete(
ModuleConcreteError::MainModuleAlreadyExists {
main_module: main_name.to_string(),
Expand Down Expand Up @@ -1001,6 +1091,7 @@ impl ModuleMap {
.data
.borrow()
.get_name_by_module(&referrer_global)
.map(|name| name.as_str().to_string())
.expect("ModuleInfo not found");

let specifier_str = specifier.to_rust_string_lossy(scope);
Expand Down Expand Up @@ -1114,7 +1205,7 @@ impl ModuleMap {
None
}

pub(crate) fn get_requested_modules(
pub fn get_requested_modules(
&self,
id: ModuleId,
) -> Option<Vec<ModuleRequest>> {
Expand Down
4 changes: 2 additions & 2 deletions core/modules/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ impl std::fmt::Display for RequestedModuleType {
/// import assertions explicitly constrains an import to JSON, in
/// which case this will have a `RequestedModuleType::Json`.
#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)]
pub(crate) struct ModuleReference {
pub struct ModuleReference {
pub specifier: ModuleSpecifier,
pub requested_module_type: RequestedModuleType,
}
Expand All @@ -686,7 +686,7 @@ pub(crate) struct ModuleReference {
/// import assertions explicitly constrains an import to JSON, in
/// which case this will have a `RequestedModuleType::Json`.
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
pub(crate) struct ModuleRequest {
pub struct ModuleRequest {
pub reference: ModuleReference,
/// None if this is a root request.
pub referrer_source_offset: Option<i32>,
Expand Down
Loading
Loading