Skip to content

refactor(core): drop redundant per-call FileId/path indices in detectors #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BartWaardenburg merged 2 commits into from
May 12, 2026
Merged

refactor(core): drop redundant per-call FileId/path indices in detectors #357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6349779
refactor(core): drop redundant per-call FileId/path indices in detectors
BartWaardenburg May 12, 2026
2c5733d
refactor(core): pin ModuleGraph::modules FileId-as-index invariant
BartWaardenburg May 12, 2026
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
136 changes: 57 additions & 79 deletions crates/core/src/analyze/unused_exports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,15 +243,6 @@ pub fn find_unused_exports(
None
};

// Precompute reachable FileIds so we can distinguish meaningful references
// (from reachable modules) from unreachable-to-unreachable references.
let reachable_files: FxHashSet<u32> = graph
.modules
.iter()
.filter(|m| m.is_reachable())
.map(|m| m.file_id.0)
.collect();

let module_results: Vec<(Vec<UnusedExport>, Vec<UnusedExport>, Vec<StaleSuppression>)> = graph
.modules
.par_iter()
Expand Down Expand Up @@ -311,10 +302,15 @@ pub fn find_unused_exports(
let has_cross_file_ref = if module.is_reachable() {
!export.references.is_empty()
} else {
export
.references
.iter()
.any(|r| reachable_files.contains(&r.from_file.0))
export.references.iter().any(|r| {
graph.modules.get(r.from_file.0 as usize).is_some_and(|m| {
debug_assert_eq!(
m.file_id, r.from_file,
"ModuleGraph::modules FileId-as-index invariant broken"
);
m.is_reachable()
})
})
};
// Treat side-effect-registered exports (Lit @customElement,
// customElements.define) as referenced even when no other file
Expand Down Expand Up @@ -595,22 +591,20 @@ pub fn find_private_type_leaks(
suppressions: &SuppressionContext<'_>,
line_offsets_by_file: &LineOffsetsMap<'_>,
) -> Vec<PrivateTypeLeak> {
let module_by_id: FxHashMap<FileId, &ModuleNode> = graph
.modules
.iter()
.map(|module| (module.file_id, module))
.collect();

let mut leaks = Vec::new();
for module_info in modules {
if module_info.public_signature_type_references.is_empty()
|| module_info.local_type_declarations.is_empty()
{
continue;
}
let Some(module) = module_by_id.get(&module_info.file_id) else {
let Some(module) = graph.modules.get(module_info.file_id.0 as usize) else {
continue;
};
debug_assert_eq!(
module.file_id, module_info.file_id,
"ModuleGraph::modules FileId-as-index invariant broken"
);
if is_storybook_file(&module.path) || is_route_convention_file(&module.path, &config.root) {
continue;
}
Expand Down Expand Up @@ -665,17 +659,21 @@ pub fn find_private_type_leaks(
fn collect_dynamic_reexport_sources(
resolved_modules: &[crate::resolve::ResolvedModule],
graph: &ModuleGraph,
module_idx_by_file_id: &FxHashMap<FileId, usize>,
re_export_sources: &mut FxHashMap<usize, FxHashSet<usize>>,
) {
use crate::extract::ExportName;
use fallow_types::extract::ImportedName;

for resolved in resolved_modules {
let Some(&wrapper_idx) = module_idx_by_file_id.get(&resolved.file_id) else {
let wrapper_idx = resolved.file_id.0 as usize;
let Some(wrapper) = graph.modules.get(wrapper_idx) else {
continue;
};
let wrapper_exports = &graph.modules[wrapper_idx].exports;
debug_assert_eq!(
wrapper.file_id, resolved.file_id,
"ModuleGraph::modules FileId-as-index invariant broken"
);
let wrapper_exports = &wrapper.exports;

for dynamic_import in &resolved.resolved_dynamic_imports {
let crate::resolve::ResolveResult::InternalModule(source_file_id) =
Expand All @@ -698,9 +696,10 @@ fn collect_dynamic_reexport_sources(
continue;
}

let Some(&source_idx) = module_idx_by_file_id.get(source_file_id) else {
let source_idx = source_file_id.0 as usize;
if source_idx >= graph.modules.len() {
continue;
};
}
re_export_sources
.entry(wrapper_idx)
.or_default()
Expand All @@ -726,17 +725,16 @@ pub fn find_duplicate_exports(
resolved_modules: &[crate::resolve::ResolvedModule],
) -> Vec<DuplicateExport> {
let ignore_matchers = config.compiled_ignore_exports.as_slice();
// Build a map from FileId to module index for dynamic re-export source lookup.
let module_idx_by_file_id: FxHashMap<FileId, usize> = graph
.modules
.iter()
.enumerate()
.map(|(idx, m)| (m.file_id, idx))
.collect();

// Build a set of re-export relationships: (re-exporting module idx) -> set of (source module idx)
// Build a set of re-export relationships: (re-exporting module idx) -> set of (source module idx).
// Module idx and `file_id.0 as usize` are interchangeable; see the
// invariant doc on `ModuleGraph::modules`.
let mut re_export_sources: FxHashMap<usize, FxHashSet<usize>> = FxHashMap::default();
for (idx, module) in graph.modules.iter().enumerate() {
debug_assert_eq!(
module.file_id.0 as usize, idx,
"ModuleGraph::modules FileId-as-index invariant broken"
);
for re in &module.re_exports {
re_export_sources
.entry(idx)
Expand All @@ -753,12 +751,7 @@ pub fn find_duplicate_exports(
// import targeting `InternalModule(S)` AND A also exports the name being
// imported. The export-side check guards against false negatives where a
// module dynamically imports something but does not actually re-export it.
collect_dynamic_reexport_sources(
resolved_modules,
graph,
&module_idx_by_file_id,
&mut re_export_sources,
);
collect_dynamic_reexport_sources(resolved_modules, graph, &mut re_export_sources);

struct ExportEntry {
module_idx: usize,
Expand All @@ -769,11 +762,6 @@ pub fn find_duplicate_exports(
}

let mut export_locations: FxHashMap<String, Vec<ExportEntry>> = FxHashMap::default();
let file_id_by_path: FxHashMap<&std::path::Path, FileId> = graph
.modules
.iter()
.map(|module| (module.path.as_path(), module.file_id))
.collect();

for (idx, module) in graph.modules.iter().enumerate() {
if !module.is_reachable() || module.is_entry_point() {
Expand Down Expand Up @@ -857,24 +845,28 @@ pub fn find_duplicate_exports(
// For each pair (A, B), if A re-exports from B or B re-exports from A,
// they are part of the same export chain, not true duplicates.
let module_indices: FxHashSet<usize> = locations.iter().map(|e| e.module_idx).collect();
let independent: Vec<DuplicateLocation> = locations
.into_iter()
.filter(|e| {
let sources = re_export_sources.get(&e.module_idx);
let has_source_in_set =
sources.is_some_and(|s| s.iter().any(|src| module_indices.contains(src)));
!has_source_in_set
})
.map(|e| {
let (line, col) =
byte_offset_to_line_col(line_offsets_by_file, e.file_id, e.span_start);
DuplicateLocation {
path: e.path,
line,
col,
}
})
.collect();
let (independent_file_ids, independent): (Vec<FileId>, Vec<DuplicateLocation>) =
locations
.into_iter()
.filter(|e| {
let sources = re_export_sources.get(&e.module_idx);
let has_source_in_set = sources
.is_some_and(|s| s.iter().any(|src| module_indices.contains(src)));
!has_source_in_set
})
.map(|e| {
let (line, col) =
byte_offset_to_line_col(line_offsets_by_file, e.file_id, e.span_start);
(
e.file_id,
DuplicateLocation {
path: e.path,
line,
col,
},
)
})
.unzip();

if independent.len() <= 1 {
return None;
Expand All @@ -885,7 +877,7 @@ pub fn find_duplicate_exports(
// modules in different directories) that happen to export the same name
// are not actionable duplicates since they can never be confused at an
// import site.
let has_shared_importer = has_common_importer(&independent, graph, &file_id_by_path);
let has_shared_importer = has_common_importer(&independent_file_ids, graph);
if has_shared_importer {
Some(DuplicateExport {
export_name: name,
Expand All @@ -904,29 +896,15 @@ pub fn find_duplicate_exports(
/// from both. This filters out false positives from unrelated leaf modules (e.g.,
/// SvelteKit route files in different directories) that coincidentally export the
/// same name but are never imported together.
fn has_common_importer(
locations: &[DuplicateLocation],
graph: &ModuleGraph,
file_id_by_path: &FxHashMap<&std::path::Path, FileId>,
) -> bool {
if locations.len() <= 1 {
return false;
}

// Collect FileIds for the duplicate locations through a pre-built path index.
let file_ids: Vec<FileId> = locations
.iter()
.filter_map(|loc| file_id_by_path.get(loc.path.as_path()).copied())
.collect();

fn has_common_importer(file_ids: &[FileId], graph: &ModuleGraph) -> bool {
if file_ids.len() <= 1 {
return false;
}

let duplicate_files: FxHashSet<FileId> = file_ids.iter().copied().collect();
let mut importer_owner: FxHashMap<FileId, FileId> = FxHashMap::default();

for file_id in file_ids {
for &file_id in file_ids {
let idx = file_id.0 as usize;
if idx >= graph.reverse_deps.len() {
continue;
Expand Down
8 changes: 8 additions & 0 deletions crates/graph/src/graph/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ pub use types::{ExportSymbol, ModuleNode, ReExportEdge, ReferenceKind, SymbolRef
#[derive(Debug)]
pub struct ModuleGraph {
/// All modules indexed by `FileId`.
///
/// Invariant: `modules[file_id.0 as usize].file_id == file_id` for every
/// `FileId` in the graph. Holds because `discover/walk.rs` assigns FileIds
/// sequentially via `.enumerate()` after path-sorting, and
/// `build::populate_edges` pushes one `ModuleNode` per file in iteration
/// order. Detectors rely on this for O(1) FileId-to-module lookup
/// (`graph.modules.get(file_id.0 as usize)`) instead of building a
/// per-call `FxHashMap<FileId, &ModuleNode>`.
pub modules: Vec<ModuleNode>,
/// Flat edge storage for cache-friendly iteration.
edges: Vec<Edge>,
Expand Down