Skip to content

Commit

Permalink
refactor(linter): use RwLock<FxHashMap> instead of FxDashMap for …
Browse files Browse the repository at this point in the history
…module record data (#8061)
  • Loading branch information
Boshen committed Dec 22, 2024
1 parent 7cb84f3 commit 547c102
Show file tree
Hide file tree
Showing 12 changed files with 90 additions and 74 deletions.
18 changes: 9 additions & 9 deletions crates/oxc_linter/src/module_graph_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,38 +200,38 @@ impl ModuleGraphVisitor {
}
};
}
for module_record_ref in &module_record.loaded_modules {
for pair in module_record.loaded_modules.read().unwrap().iter() {
if self.depth > self.max_depth {
return VisitFoldWhile::Stop(accumulator.into_inner());
}

let path = &module_record_ref.resolved_absolute_path;
let path = &pair.1.resolved_absolute_path;
if !self.traversed.insert(path.clone()) {
continue;
}

if !filter(module_record_ref.pair(), module_record) {
if !filter(pair, module_record) {
continue;
}

self.depth += 1;

event(ModuleGraphVisitorEvent::Enter, module_record_ref.pair(), module_record);
enter(module_record_ref.pair(), module_record);
event(ModuleGraphVisitorEvent::Enter, pair, module_record);
enter(pair, module_record);

accumulate!(fold(accumulator.into_inner(), module_record_ref.pair(), module_record));
accumulate!(fold(accumulator.into_inner(), pair, module_record));
accumulate!(self.filter_fold_recursive(
accumulator,
module_record_ref.value(),
pair.1,
filter,
fold,
event,
enter,
leave
));

event(ModuleGraphVisitorEvent::Leave, module_record_ref.pair(), module_record);
leave(module_record_ref.pair(), module_record);
event(ModuleGraphVisitorEvent::Leave, pair, module_record);
leave(pair, module_record);

self.depth -= 1;
}
Expand Down
15 changes: 7 additions & 8 deletions crates/oxc_linter/src/module_record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,15 @@
use std::{
fmt,
path::{Path, PathBuf},
sync::Arc,
sync::{Arc, RwLock},
};

use dashmap::DashMap;
use rustc_hash::{FxBuildHasher, FxHashMap};
use rustc_hash::FxHashMap;

use oxc_semantic::Semantic;
use oxc_span::{CompactStr, Span};
pub use oxc_syntax::module_record::RequestedModule;

type FxDashMap<K, V> = DashMap<K, V, FxBuildHasher>;

/// ESM Module Record
///
/// All data inside this data structure are for ESM, no commonjs data is allowed.
Expand Down Expand Up @@ -49,7 +46,7 @@ pub struct ModuleRecord {
///
/// Note that Oxc does not support cross-file analysis, so this map will be empty after
/// [`ModuleRecord`] is created. You must link the module records yourself.
pub loaded_modules: FxDashMap<CompactStr, Arc<ModuleRecord>>,
pub loaded_modules: RwLock<FxHashMap<CompactStr, Arc<ModuleRecord>>>,

/// `[[ImportEntries]]`
///
Expand Down Expand Up @@ -81,7 +78,7 @@ pub struct ModuleRecord {

/// Reexported bindings from `export * from 'specifier'`
/// Keyed by resolved path
pub exported_bindings_from_star_export: FxDashMap<PathBuf, Vec<CompactStr>>,
pub exported_bindings_from_star_export: RwLock<FxHashMap<PathBuf, Vec<CompactStr>>>,

/// `export default name`
/// ^^^^^^^ span
Expand All @@ -93,8 +90,10 @@ impl fmt::Debug for ModuleRecord {
// recursively formatting loaded modules can crash when the module graph is cyclic
let loaded_modules = self
.loaded_modules
.read()
.unwrap()
.iter()
.map(|entry| (entry.key().to_string()))
.map(|(key, _)| (key.to_string()))
.reduce(|acc, key| format!("{acc}, {key}"))
.unwrap_or_default();
let loaded_modules = format!("{{ {loaded_modules} }}");
Expand Down
11 changes: 6 additions & 5 deletions crates/oxc_linter/src/rules/import/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,28 +53,29 @@ declare_oxc_lint!(
impl Rule for Default {
fn run_once(&self, ctx: &LintContext<'_>) {
let module_record = ctx.module_record();
let loaded_modules = module_record.loaded_modules.read().unwrap();
for import_entry in &module_record.import_entries {
let ImportImportName::Default(default_span) = import_entry.import_name else {
continue;
};

let specifier = import_entry.module_request.name();
let Some(remote_module_record_ref) = module_record.loaded_modules.get(specifier) else {
let Some(remote_module_record) = loaded_modules.get(specifier) else {
continue;
};
if !remote_module_record_ref.has_module_syntax {
if !remote_module_record.has_module_syntax {
continue;
}
if !remote_module_record_ref
if !remote_module_record
.resolved_absolute_path
.extension()
.and_then(|ext| ext.to_str())
.is_some_and(|ext| VALID_EXTENSIONS.contains(&ext))
{
continue;
}
if remote_module_record_ref.export_default.is_none()
&& !remote_module_record_ref.exported_bindings.contains_key("default")
if remote_module_record.export_default.is_none()
&& !remote_module_record.exported_bindings.contains_key("default")
{
ctx.diagnostic(default_diagnostic(specifier, default_span));
}
Expand Down
18 changes: 6 additions & 12 deletions crates/oxc_linter/src/rules/import/export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ impl Rule for Export {
let mut all_export_names = FxHashMap::default();
let mut visited = FxHashSet::default();

let loaded_modules = module_record.loaded_modules.read().unwrap();
module_record.star_export_entries.iter().for_each(|star_export_entry| {
if star_export_entry.is_type {
return;
Expand All @@ -64,17 +65,11 @@ impl Rule for Export {
let Some(module_request) = &star_export_entry.module_request else {
return;
};
let Some(remote_module_record_ref) =
module_record.loaded_modules.get(module_request.name())
else {
let Some(remote_module_record) = loaded_modules.get(module_request.name()) else {
return;
};

walk_exported_recursive(
remote_module_record_ref.value(),
&mut export_names,
&mut visited,
);
walk_exported_recursive(remote_module_record, &mut export_names, &mut visited);

if export_names.is_empty() {
ctx.diagnostic(no_named_export(module_request.name(), module_request.span()));
Expand Down Expand Up @@ -126,16 +121,15 @@ fn walk_exported_recursive(
for name in module_record.exported_bindings.keys() {
result.insert(name.clone());
}
let loaded_modules = module_record.loaded_modules.read().unwrap();
for export_entry in &module_record.star_export_entries {
let Some(module_request) = &export_entry.module_request else {
continue;
};
let Some(remote_module_record_ref) =
module_record.loaded_modules.get(module_request.name())
else {
let Some(remote_module_record) = loaded_modules.get(module_request.name()) else {
continue;
};
walk_exported_recursive(remote_module_record_ref.value(), result, visited);
walk_exported_recursive(remote_module_record, result, visited);
}
}

Expand Down
12 changes: 7 additions & 5 deletions crates/oxc_linter/src/rules/import/named.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,17 @@ impl Rule for Named {

let module_record = ctx.module_record();

let loaded_modules = module_record.loaded_modules.read().unwrap();
for import_entry in &module_record.import_entries {
// Get named import
let ImportImportName::Name(import_name) = &import_entry.import_name else {
continue;
};
let specifier = import_entry.module_request.name();
// Get remote module record
let Some(remote_module_record_ref) = module_record.loaded_modules.get(specifier) else {
let Some(remote_module_record) = loaded_modules.get(specifier) else {
continue;
};
let remote_module_record = remote_module_record_ref.value();
if !remote_module_record.has_module_syntax {
continue;
}
Expand All @@ -118,15 +118,18 @@ impl Rule for Named {
// check re-export
if remote_module_record
.exported_bindings_from_star_export
.read()
.unwrap()
.iter()
.any(|entry| entry.value().contains(&import_name.name))
.any(|(_, value)| value.contains(&import_name.name))
{
continue;
}

ctx.diagnostic(named_diagnostic(name, specifier, import_span));
}

let loaded_modules = module_record.loaded_modules.read().unwrap();
for export_entry in &module_record.indirect_export_entries {
let Some(module_request) = &export_entry.module_request else {
continue;
Expand All @@ -136,10 +139,9 @@ impl Rule for Named {
};
let specifier = module_request.name();
// Get remote module record
let Some(remote_module_record_ref) = module_record.loaded_modules.get(specifier) else {
let Some(remote_module_record) = loaded_modules.get(specifier) else {
continue;
};
let remote_module_record = remote_module_record_ref.value();
if !remote_module_record.has_module_syntax {
continue;
}
Expand Down
30 changes: 18 additions & 12 deletions crates/oxc_linter/src/rules/import/namespace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,32 +123,33 @@ impl Rule for Namespace {
return;
}

let loaded_modules = module_record.loaded_modules.read().unwrap();

for entry in &module_record.import_entries {
let (source, module) = match &entry.import_name {
ImportImportName::NamespaceObject => {
let source = entry.module_request.name();
if let Some(module) = module_record.loaded_modules.get(source) {
(source.to_string(), Arc::clone(module.value()))
if let Some(module) = loaded_modules.get(source) {
(source.to_string(), Arc::clone(module))
} else {
return;
}
}
ImportImportName::Name(name) => {
let Some(loaded_module) =
module_record.loaded_modules.get(entry.module_request.name())
let Some(loaded_module) = loaded_modules.get(entry.module_request.name())
else {
return;
};
let Some(source) = get_module_request_name(name.name(), &loaded_module) else {
let Some(source) = get_module_request_name(name.name(), loaded_module) else {
return;
};

let Some(loaded_module) = &loaded_module.loaded_modules.get(source.as_str())
else {
let loaded_module = loaded_module.loaded_modules.read().unwrap();
let Some(loaded_module) = loaded_module.get(source.as_str()) else {
return;
};

(source, Arc::clone(loaded_module.value()))
(source, Arc::clone(loaded_module))
}
ImportImportName::Default(_) => {
// TODO: Hard to confirm if it's a namespace object
Expand Down Expand Up @@ -275,10 +276,11 @@ fn check_deep_namespace_for_node(

if let Some(module_source) = get_module_request_name(name, module) {
let parent_node = ctx.nodes().parent_node(node.id())?;
let module_record = module.loaded_modules.get(module_source.as_str())?;
let loaded_modules = module.loaded_modules.read().unwrap();
let module_record = loaded_modules.get(module_source.as_str())?;
let mut namespaces = namespaces.to_owned();
namespaces.push(name.into());
check_deep_namespace_for_node(parent_node, source, &namespaces, module_record.value(), ctx);
check_deep_namespace_for_node(parent_node, source, &namespaces, module_record, ctx);
} else {
check_binding_exported(
name,
Expand Down Expand Up @@ -313,11 +315,13 @@ fn check_deep_namespace_for_object_pattern(
if let Some(module_source) = get_module_request_name(&name, module) {
let mut next_namespaces = namespaces.to_owned();
next_namespaces.push(name.to_string());

let loaded_modules = module.loaded_modules.read().unwrap();
check_deep_namespace_for_object_pattern(
pattern,
source,
next_namespaces.as_slice(),
module.loaded_modules.get(module_source.as_str()).unwrap().value(),
loaded_modules.get(module_source.as_str()).unwrap(),
ctx,
);
continue;
Expand Down Expand Up @@ -353,8 +357,10 @@ fn check_binding_exported(
|| (name == "default" && module.export_default.is_some())
|| module
.exported_bindings_from_star_export
.read()
.unwrap()
.iter()
.any(|entry| entry.value().iter().any(|s| s.as_str() == name))
.any(|(_, value)| value.iter().any(|s| s.as_str() == name))
{
return;
}
Expand Down
3 changes: 2 additions & 1 deletion crates/oxc_linter/src/rules/import/no_duplicates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,12 @@ impl Rule for NoDuplicates {
fn run_once(&self, ctx: &LintContext<'_>) {
let module_record = ctx.module_record();

let loaded_modules = module_record.loaded_modules.read().unwrap();
let groups = module_record
.requested_modules
.iter()
.map(|(source, requested_modules)| {
let resolved_absolute_path = module_record.loaded_modules.get(source).map_or_else(
let resolved_absolute_path = loaded_modules.get(source).map_or_else(
|| source.to_string(),
|module| module.resolved_absolute_path.to_string_lossy().to_string(),
);
Expand Down
5 changes: 3 additions & 2 deletions crates/oxc_linter/src/rules/import/no_named_as_default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,13 @@ impl Rule for NoNamedAsDefault {
};

let specifier = import_entry.module_request.name();
let Some(remote_module_record_ref) = module_record.loaded_modules.get(specifier) else {
let remote_module_record = module_record.loaded_modules.read().unwrap();
let Some(remote_module_record) = remote_module_record.get(specifier) else {
continue;
};

let import_name = import_entry.local_name.name();
if remote_module_record_ref.exported_bindings.contains_key(import_name) {
if remote_module_record.exported_bindings.contains_key(import_name) {
ctx.diagnostic(no_named_as_default_diagnostic(
*import_span,
import_name,
Expand Down
13 changes: 9 additions & 4 deletions crates/oxc_linter/src/rules/import/no_named_as_default_member.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::sync::Arc;

use oxc_ast::{
ast::{BindingPatternKind, Expression, IdentifierReference, MemberExpression},
AstKind,
Expand Down Expand Up @@ -82,11 +84,12 @@ impl Rule for NoNamedAsDefaultMember {
};

let specifier = import_entry.module_request.name();
let Some(remote_module_record_ref) = module_record.loaded_modules.get(specifier) else {
let remote_module_record = module_record.loaded_modules.read().unwrap();
let Some(remote_module_record) = remote_module_record.get(specifier) else {
continue;
};

if remote_module_record_ref.exported_bindings.is_empty() {
if remote_module_record.exported_bindings.is_empty() {
continue;
}

Expand All @@ -95,8 +98,10 @@ impl Rule for NoNamedAsDefaultMember {
return;
};

has_members_map
.insert(symbol_id, (remote_module_record_ref, import_entry.module_request.clone()));
has_members_map.insert(
symbol_id,
(Arc::clone(remote_module_record), import_entry.module_request.clone()),
);
}

if has_members_map.is_empty() {
Expand Down
5 changes: 3 additions & 2 deletions crates/oxc_linter/src/rules/import/no_self_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ impl Rule for NoSelfImport {
let module_record = ctx.module_record();
let resolved_absolute_path = &module_record.resolved_absolute_path;
for (request, requested_modules) in &module_record.requested_modules {
let Some(remote_module_record_ref) = module_record.loaded_modules.get(request) else {
let remote_module_record = module_record.loaded_modules.read().unwrap();
let Some(remote_module_record) = remote_module_record.get(request) else {
continue;
};
if remote_module_record_ref.value().resolved_absolute_path == *resolved_absolute_path {
if remote_module_record.resolved_absolute_path == *resolved_absolute_path {
for requested_module in requested_modules {
ctx.diagnostic(no_self_import_diagnostic(requested_module.span));
}
Expand Down
Loading

0 comments on commit 547c102

Please sign in to comment.