Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use super::{FileNode, InternedStr};
use std::path::Path;
use std::sync::Arc;

impl FileNode {
pub(crate) fn from_parts(id: u64, path: Arc<Path>) -> Self {
Self { id, path }
}
}

impl InternedStr {
pub(crate) fn from_parts(id: u64, value: Arc<str>) -> Self {
Self { id, value }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ where
.iter()
.filter_map(|node| {
if let NodeId::Symbol { file, symbol } = node {
Some((file.clone_arc(), Arc::clone(symbol)))
Some((file.clone_arc(), symbol.clone_arc()))
} else {
None
}
Expand Down Expand Up @@ -62,7 +62,7 @@ where
) = (&node, neighbor)
{
if neighbor_file == owner
&& root_symbols.contains(&(owner.clone_arc(), Arc::clone(symbol)))
&& root_symbols.contains(&(owner.clone_arc(), symbol.clone_arc()))
{
continue;
}
Expand Down
12 changes: 6 additions & 6 deletions crates/no-mistakes/src/codebase/dependencies/graph/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,21 @@ fn cached_node_sort_key(n: &NodeId) -> NodeSortKey {
match n {
NodeId::File(path) => NodeSortKey::new(Some(path.clone_arc()), "", None, None),
NodeId::Symbol { file, symbol } => {
NodeSortKey::new(Some(file.clone_arc()), "#", Some(Arc::clone(symbol)), None)
NodeSortKey::new(Some(file.clone_arc()), "#", Some(symbol.clone_arc()), None)
}
NodeId::Module(specifier) => {
NodeSortKey::new(None, "module:", Some(Arc::clone(specifier)), None)
NodeSortKey::new(None, "module:", Some(specifier.clone_arc()), None)
}
NodeId::QueueJob { queue_file, job } => NodeSortKey::new(
Some(queue_file.clone_arc()),
"#",
Some(Arc::clone(job)),
Some(job.clone_arc()),
None,
),
NodeId::WorkflowJob { workflow_file, job } => NodeSortKey::new(
Some(workflow_file.clone_arc()),
"#job:",
Some(Arc::clone(job)),
Some(job.clone_arc()),
None,
),
NodeId::WorkflowStep {
Expand All @@ -70,7 +70,7 @@ fn cached_node_sort_key(n: &NodeId) -> NodeSortKey {
} => NodeSortKey::new(
Some(workflow_file.clone_arc()),
"#job:",
Some(Arc::clone(job)),
Some(job.clone_arc()),
Some(*step),
),
NodeId::TrpcProcedure {
Expand All @@ -79,7 +79,7 @@ fn cached_node_sort_key(n: &NodeId) -> NodeSortKey {
} => NodeSortKey::new(
Some(router_file.clone_arc()),
"#procedure:",
Some(Arc::clone(procedure)),
Some(procedure.clone_arc()),
None,
),
}
Expand Down
99 changes: 89 additions & 10 deletions crates/no-mistakes/src/codebase/dependencies/graph/tests/types.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
#[cfg(test)]
mod tests_types {
use crate::codebase::dependencies::graph::{EdgeKind, NodeId, VitestSetupField};
use crate::codebase::analysis_session::PathInterner;
use crate::codebase::dependencies::graph::{
EdgeKind, FileNode, InternedStr, NodeId, VitestSetupField,
};
use std::collections::HashSet;
use std::hash::{Hash, Hasher};
use std::path::{Path, PathBuf};
use std::sync::Arc;

fn hash_of(node: &NodeId) -> u64 {
let mut hasher = std::collections::hash_map::DefaultHasher::new();
Expand Down Expand Up @@ -92,8 +96,6 @@ mod tests_types {

#[test]
fn interned_symbol_and_job_strings_share_arc_on_clone_and_keep_sort_hash() {
use std::sync::Arc;

let path = PathBuf::from("src/jobs.ts");
let symbol = NodeId::symbol(&path, "send");
let queue = NodeId::queue_job(&path, "send");
Expand All @@ -103,28 +105,28 @@ mod tests_types {
let symbol_clone = symbol.clone();
match (&symbol, &symbol_clone) {
(NodeId::Symbol { symbol: left, .. }, NodeId::Symbol { symbol: right, .. }) => {
assert!(Arc::ptr_eq(left, right))
assert!(Arc::ptr_eq(left.as_arc(), right.as_arc()))
}
_ => panic!("expected Symbol"),
}
let queue_clone = queue.clone();
match (&queue, &queue_clone) {
(NodeId::QueueJob { job: left, .. }, NodeId::QueueJob { job: right, .. }) => {
assert!(Arc::ptr_eq(left, right));
assert!(Arc::ptr_eq(left.as_arc(), right.as_arc()));
}
_ => panic!("expected QueueJob"),
}
let workflow_clone = workflow.clone();
match (&workflow, &workflow_clone) {
(NodeId::WorkflowJob { job: left, .. }, NodeId::WorkflowJob { job: right, .. }) => {
assert!(Arc::ptr_eq(left, right))
assert!(Arc::ptr_eq(left.as_arc(), right.as_arc()))
}
_ => panic!("expected WorkflowJob"),
}
let step_clone = step.clone();
match (&step, &step_clone) {
(NodeId::WorkflowStep { job: left, .. }, NodeId::WorkflowStep { job: right, .. }) => {
assert!(Arc::ptr_eq(left, right))
assert!(Arc::ptr_eq(left.as_arc(), right.as_arc()))
}
_ => panic!("expected WorkflowStep"),
}
Expand All @@ -145,7 +147,7 @@ mod tests_types {
let owned: Arc<str> = Arc::from("reuse");
let reused = NodeId::symbol(&path, owned.clone());
match reused {
NodeId::Symbol { symbol, .. } => assert!(Arc::ptr_eq(&owned, &symbol)),
NodeId::Symbol { symbol, .. } => assert!(Arc::ptr_eq(&owned, symbol.as_arc())),
other => panic!("expected Symbol, got {other:?}"),
}
}
Expand Down Expand Up @@ -219,8 +221,6 @@ mod tests_types {

#[test]
fn file_nodes_hash_by_path_bytes_and_clone_shares_the_arc() {
use std::sync::Arc;

let left = NodeId::file("src/widget.ts");
let right = NodeId::file(PathBuf::from("src/widget.ts"));
assert_eq!(left, right);
Expand All @@ -236,4 +236,83 @@ mod tests_types {
}
assert!(left < NodeId::file("src/z.ts"));
}

#[test]
fn file_node_rejects_equality_on_id_mismatch_without_requiring_equal_paths() {
let path = Arc::<Path>::from(Path::new("src/widget.ts"));
let left = FileNode::from_parts(1, Arc::clone(&path));
let right = FileNode::from_parts(2, path);
assert_eq!(left.as_ref(), right.as_ref());
assert_ne!(left, right);
assert_ne!(hash_of_value(&left), hash_of_value(&right));
}

#[test]
fn file_node_matching_ids_still_compare_path_bytes() {
let left = FileNode::from_parts(1, Arc::from(Path::new("src/a.ts")));
let right = FileNode::from_parts(1, Arc::from(Path::new("src/b.ts")));
assert_ne!(left, right);
assert!(left < right);
}

#[test]
fn interned_str_equal_content_distinct_arcs_hash_equal() {
let left = InternedStr::new(Arc::<str>::from("Widget"));
let right = InternedStr::new(Arc::<str>::from("Widget"));
assert!(!Arc::ptr_eq(left.as_arc(), right.as_arc()));
assert_eq!(left, right);
assert_eq!(left, left.clone());
assert_eq!(hash_of_value(&left), hash_of_value(&right));
assert_eq!(left.as_ref(), "Widget");
assert_eq!(&*left, "Widget");
assert_eq!(left.to_string(), "Widget");
let cloned = left.clone_arc();
assert!(Arc::ptr_eq(left.as_arc(), &cloned));
assert_eq!(left.cmp(&right), std::cmp::Ordering::Equal);
assert!(left.partial_cmp(&right).is_some());
assert_eq!(
InternedStr::from("Alpha").cmp(&InternedStr::from("Beta")),
std::cmp::Ordering::Less
);
assert_eq!(
InternedStr::from(String::from("Z")),
InternedStr::from(Arc::<str>::from("Z"))
);
}

#[test]
fn interned_str_rejects_equality_on_id_mismatch_and_compares_bytes_on_collision() {
let shared = Arc::<str>::from("Widget");
let left = InternedStr::from_parts(1, Arc::clone(&shared));
let right = InternedStr::from_parts(2, shared);
assert_eq!(left.as_ref(), right.as_ref());
assert_ne!(left, right);

let collision_left = InternedStr::from_parts(7, Arc::<str>::from("alpha"));
let collision_right = InternedStr::from_parts(7, Arc::<str>::from("beta"));
assert_ne!(collision_left, collision_right);
assert!(collision_left < collision_right);
}

#[test]
fn node_id_symbol_and_symbol_in_remain_hash_eq_compatible() {
let interner = PathInterner::new();
let standalone = NodeId::symbol("src/widget.ts", "Widget");
let interned = NodeId::symbol_in(&interner, "src/widget.ts", "Widget");
assert_eq!(standalone, interned);
assert_eq!(hash_of(&standalone), hash_of(&interned));
match (&standalone, &interned) {
(NodeId::Symbol { symbol: left, .. }, NodeId::Symbol { symbol: right, .. }) => {
assert!(!Arc::ptr_eq(left.as_arc(), right.as_arc()));
assert_eq!(left.as_ref(), right.as_ref());
}
other => panic!("expected Symbol, got {other:?}"),
}
}

fn hash_of_value<T: Hash>(value: &T) -> u64 {
let mut hasher = std::collections::hash_map::DefaultHasher::new();
value.hash(&mut hasher);
hasher.finish()
}
}
21 changes: 12 additions & 9 deletions crates/no-mistakes/src/codebase/dependencies/graph/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,33 @@ include!("types_file_node.rs");
/// A node in the dependency graph: a source file, external module, or virtual node.
///
/// File paths are interned as [`FileNode`] (integer hash + `Arc<Path>`) and
/// symbol/job/module names as `Arc<str>` so cloning a `NodeId` does not copy
/// those bytes. Construct with `NodeId::file` / `symbol` / `module` / …; keep
/// matching `NodeId::File(path)` and `NodeId::Symbol { file, .. }`.
/// symbol/job/module names as [`InternedStr`] so cloning a `NodeId` does not
/// copy those bytes. Construct with `NodeId::file` / `symbol` / `module` / …;
/// keep matching `NodeId::File(path)` and `NodeId::Symbol { file, .. }`.
#[derive(Debug, Clone, PartialOrd, Ord)]
pub enum NodeId {
File(FileNode),
Symbol {
file: FileNode,
symbol: Arc<str>,
symbol: InternedStr,
},
Module(Arc<str>),
Module(InternedStr),
QueueJob {
queue_file: FileNode,
job: Arc<str>,
job: InternedStr,
},
WorkflowJob {
workflow_file: FileNode,
job: Arc<str>,
job: InternedStr,
},
WorkflowStep {
workflow_file: FileNode,
job: Arc<str>,
job: InternedStr,
step: usize,
},
TrpcProcedure {
router_file: FileNode,
procedure: Arc<str>,
procedure: InternedStr,
},
}

Expand Down Expand Up @@ -147,3 +147,6 @@ type ParsedImports<'a> = Vec<(
&'a crate::codebase::ts_source::facts::TsFileFacts,
HashSet<String>,
)>;

#[cfg(test)]
mod interned_identity_tests;
Loading
Loading