feat: enhance ModuleMap#1146
Conversation
|
|
|
@CyanChanges can you please rebase this PR to the latest |
CyanChanges
force-pushed
the
enhance-module-map
branch
from
4351188 to
9ccb765
Compare
WalkthroughThis pull request expands the public API of the module management system by promoting previously private types and methods to public visibility. ModuleMap transitions from pub(crate) to pub with new public accessor/mutator methods (get, set, delete, alias_id, with_map), along with name resolution methods (get_name_by_module, get_name_by_id). Similarly, ModuleReference, ModuleRequest, and SymbolicModule are made public. ModuleMapData receives updated method signatures supporting generic Borrow-based key comparisons and returns reference types instead of owned Strings. A new unsafe function module_map_from is added to JsRuntime to expose module map retrieval from a V8 scope. Test coverage is extended to validate aliasing and mapping behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1146 +/- ##
==========================================
- Coverage 81.43% 80.29% -1.15%
==========================================
Files 97 105 +8
Lines 23877 27618 +3741
==========================================
+ Hits 19445 22176 +2731
- Misses 4432 5442 +1010 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/modules/module_map_data.rs (1)
73-82: FixModuleNameTypeMap::get_mapto honor itsOptioncontract
ModuleNameTypeMap::get_mapcurrently does:pub fn get_map( &self, module_type: &RequestedModuleType, ) -> Option<&HashMap<ModuleName, T>> { let index = match self.map_index(module_type) { Some(index) => index, None => todo!(), }; self.submaps.get(index) }but it’s exposed via
ModuleMapData::get_mapandModuleMap::with_mapas anOption<...>. If the requested module type has never been inserted, callers logically expectNone, not a panic fromtodo!().That’s a correctness bug and will be very surprising for embedders using
with_mapfor inspection.Consider:
pub fn get_map( &self, module_type: &RequestedModuleType, ) -> Option<&HashMap<ModuleName, T>> { - let index = match self.map_index(module_type) { - Some(index) => index, - None => todo!(), - }; - - self.submaps.get(index) + let index = self.map_index(module_type)?; + self.submaps.get(index) }(You may also want to standardize the bounds across
get/deleteto useModuleName: Borrow<Q>for clarity, but that’s cosmetic.)Also applies to: 108-139, 334-339
🧹 Nitpick comments (2)
core/modules/module_map_data.rs (1)
24-45: SymbolicModule and ModuleMapData alias/id helpers look sound
- Making
SymbolicModulepublic withAlias(ModuleName)andMod(ModuleId)is consistent with the newModuleMapAPI surface.- The manual
CloneforSymbolicModuleusesModuleName::try_clone()and falls back toModuleName::from(name.to_string()), which is safe and avoids unnecessary allocations in cheap-clone cases.ModuleMapData::get_idcorrectly follows alias chains viaSymbolicModule::Aliasuntil it reaches aModentry; it usesBorrowonModuleNameso callers can pass either&stror&ModuleName.get/set/set_id/delete/alias/get_handleand the newget_name_by_module/get_name_by_idaccessors all line up with whatModuleMapexposes and avoid extra cloning by returning references where possible.Apart from the
get_mapissue called out separately, this refactor looks internally consistent.Also applies to: 239-319, 360-392, 450-452
core/modules/map.rs (1)
132-160: PublicModuleMapAPI and helpers appear coherent with internal data modelThe structural/API changes here look good:
- Promoting
ModuleMaptopubwhile keeping fields non-public exposes a controlled surface of operations.get_id/get/set/set_id/delete/alias_idcorrectly delegate toModuleMapData, usingBorrow-based generics so callers can pass either&stror&ModuleName.getreturning an ownedSymbolicModulevia.cloned()is appropriate givenSymbolicModule: Cloneand avoids leaking internal references.- Public
get_name_by_module/get_name_by_idandget_requested_modulesprovide the necessary introspection for tests and embedders, even though they clone (as the TODO comments note, avoiding extra allocations later would be a nice optimization, but not required for correctness).- The new
wake_modulecallback and other internal callers now usingJsRealm::module_map_from(scope)are consistent with existing uses of that helper elsewhere in the codebase.Aside from the
with_map/get_mappanic path already flagged, the expandedModuleMapsurface is internally consistent and behaves as expected.Also applies to: 254-333, 351-370, 1088-1095, 1196-1203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/modules/map.rs(9 hunks)core/modules/mod.rs(2 hunks)core/modules/module_map_data.rs(11 hunks)core/modules/tests.rs(4 hunks)core/runtime/jsruntime.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
core/modules/tests.rs (4)
core/modules/map.rs (2)
get(268-278)new(215-240)core/modules/module_map_data.rs (2)
get(73-81)get(270-281)core/modules/loaders.rs (5)
new(228-241)new(350-354)new(499-509)new(564-573)new(645-657)core/fast_string.rs (1)
from_static(179-189)
core/runtime/jsruntime.rs (1)
core/runtime/jsrealm.rs (1)
module_map_from(272-280)
core/modules/map.rs (2)
core/fast_string.rs (4)
borrow(93-95)borrow(376-378)hash(74-76)hash(345-347)core/modules/module_map_data.rs (10)
get_id(241-268)get(73-81)get(270-281)set(283-291)set_id(293-305)delete(108-126)delete(307-318)alias(320-332)get_name_by_module(360-368)get_name_by_id(390-392)
core/modules/module_map_data.rs (2)
core/fast_string.rs (11)
borrow(93-95)borrow(376-378)hash(74-76)hash(345-347)from(62-64)from(68-70)from(412-416)from(422-425)from(431-435)from(441-445)from(449-457)core/modules/map.rs (9)
delete(306-319)get_id(256-266)get(268-278)set(280-290)set_id(293-303)is_alias(395-401)get_handle(379-384)get_name_by_module(351-361)get_name_by_id(363-370)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-linux / linux test-ops
- GitHub Check: build-linux / linux test-miri
- GitHub Check: build-linux / linux test
- GitHub Check: build-linux-arm / linux-arm test-miri
- GitHub Check: build-windows / windows lint
- GitHub Check: build-windows / windows test
- GitHub Check: build-macos / macos lint
- GitHub Check: build-linux-arm / linux-arm test
- GitHub Check: build-linux-arm / linux-arm lint
- GitHub Check: build-linux / linux lint-deps
- GitHub Check: build-macos / macos test
- GitHub Check: build-linux / linux coverage
🔇 Additional comments (2)
core/modules/mod.rs (1)
678-693: Public exposure ofModuleReference/ModuleRequestlooks consistentMaking
ModuleReferenceandModuleRequestpublic aligns with the new publicModuleMapAPIs that return these types. No functional change or invariants affected here.core/modules/tests.rs (1)
32-32: New module map tests correctly exercise aliasing and ID mappingThe added
SymbolicModuleimports, the extra assertion intest_recursive_load, and the newtest_mapall match the newModuleMapAPI:
- Using
RequestedModuleType::Nonefor JS modules is consistent with howcreate_module_infostores entries.alias_idvs manualset(SymbolicModule::Alias(..))are validated against each other.set_idvsset(SymbolicModule::Mod(..))are similarly cross-checked.- Lookups via both
&ModuleNameand&str("#alias0","#id0") correctly drive the genericBorrow-based API.deleteis verified to fully remove the alias entry.No issues from a correctness or lifetime perspective.
Also applies to: 56-56, 363-423, 2470-2534
| /// 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); | ||
| } |
There was a problem hiding this comment.
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.
| /// # SAFETY | ||
| /// try to acquire [`ModuleMap`] with HandleScope from a Isolate | ||
| /// that is not initialized with `deno_core::JsRuntime` will be undefined behavior | ||
| /// | ||
| /// manipulating internal module may cause problems | ||
| pub unsafe fn module_map_from(scope: &mut v8::PinScope) -> Rc<ModuleMap> { | ||
| JsRealm::module_map_from(scope) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify safety contract for JsRuntime::module_map_from
The implementation is fine (it just forwards to JsRealm::module_map_from), but the safety docs are quite minimal for a public unsafe API:
- Callers must ensure the
scope’s current context actually belongs to aJsRuntimeinstance from this crate, and that the runtime is still alive. - Returned
Rc<ModuleMap>exposes internal mutable state not normally reachable; misusing it (e.g., mutating from callbacks in unexpected phases) can easily violate runtime invariants.
Consider expanding the # SAFETY section to spell out these invariants explicitly and to warn that the ModuleMap must only be used on the owning isolate’s thread and while the JsRuntime is still valid.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the function in question
fd "jsruntime.rs" --type f -x cat {} | head -2200 | tail -100
Length of output: 3629
🏁 Script executed:
#!/bin/bash
# Search for JsRealm::module_map_from implementation
rg -n "module_map_from" --type rust -A 5 -B 2Length of output: 6010
🏁 Script executed:
#!/bin/bash
# Check for other unsafe pub fns in jsruntime.rs to compare safety doc patterns
rg -n "pub unsafe fn" core/runtime/jsruntime.rs -A 3 -B 1Length of output: 197
Expand safety documentation for JsRuntime::module_map_from
The safety docs for this public unsafe API are indeed too minimal. Expand the # SAFETY section to explicitly state:
- The
PinScope's current context must belong to aJsRuntimeinstance created by this crate (not just any isolate) - The
JsRuntimeinstance must remain valid for the lifetime of the returnedRc<ModuleMap> - The
ModuleMapmust only be accessed on the isolate's owning thread - Callers must not mutate the
ModuleMapin ways that violate runtime invariants (e.g., during callback phases where the runtime expects immutability)
🤖 Prompt for AI Agents
In core/runtime/jsruntime.rs around lines 2156 to 2163, the existing SAFETY doc
for the public unsafe function module_map_from is too minimal; expand it to
explicitly state that the provided v8::PinScope's current context must belong to
a JsRuntime instance created by this crate (not an arbitrary isolate), that the
JsRuntime instance must remain valid for the entire lifetime of the returned
Rc<ModuleMap>, that the ModuleMap must only be accessed on the isolate's owning
thread, and that callers must not mutate the ModuleMap in ways that violate
runtime invariants (for example during callback phases where the runtime expects
immutability); update the comment block to include these bullet points concisely
and unambiguously directly above the function signature.
|
closing old PRs |
|
This is actually useful in context of Node.js module loader API that we need to tackle soon. |
5 participants
Issue: #1143