From 89ee19f2cbf82ea38a7d0c1c33a1b88162bc28b8 Mon Sep 17 00:00:00 2001 From: Rand Lee Date: Fri, 18 Sep 2026 21:51:47 -0700 Subject: [PATCH] fix(boundary): distinguish trait methods and qualified forwarding edges --- crates/sc-lint-boundary/src/graph/build.rs | 62 +++++-- crates/sc-lint-boundary/src/graph/mod.rs | 12 ++ .../src/graph/reference_collector.rs | 46 ++++- crates/sc-lint-boundary/src/lib.rs | 2 + crates/sc-lint-boundary/src/tests.rs | 171 +++++++++++++++++- docs/sc-lint-boundary/graph-schema.md | 16 ++ 6 files changed, 289 insertions(+), 20 deletions(-) diff --git a/crates/sc-lint-boundary/src/graph/build.rs b/crates/sc-lint-boundary/src/graph/build.rs index e91ec918..d31f365a 100644 --- a/crates/sc-lint-boundary/src/graph/build.rs +++ b/crates/sc-lint-boundary/src/graph/build.rs @@ -1,5 +1,4 @@ use super::*; -use crate::render::hex_encode; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -112,9 +111,49 @@ pub(crate) fn build_workspace_graph(root: &Path) -> Result { } } + resolve_trait_method_edges(&mut builder); Ok(builder.finish()) } +fn resolve_trait_method_edges(builder: &mut GraphBuilder) { + let methods: BTreeMap<_, _> = builder + .nodes + .iter() + .filter(|node| node.kind == "method") + .map(|node| (node.id.clone(), node)) + .collect(); + let known: BTreeSet<_> = builder.nodes.iter().map(|node| node.id.clone()).collect(); + for edge in &mut builder.edges { + if !matches!(edge.kind, "references" | "references_expr") || known.contains(&edge.to) { + continue; + } + let Some((owner, method)) = edge.to.rsplit_once("::") else { + continue; + }; + // Self::method inside a trait impl resolves to that impl when no + // inherent method exists. Otherwise accept only an unambiguous impl. + if let Some((source_owner, _)) = edge.from.split_once("::impl::") + && source_owner == owner + && let Some((source_impl, _)) = edge.from.rsplit_once("::") + { + let candidate = NodeId::new(format!("{source_impl}::{method}")); + if methods.contains_key(&candidate) { + edge.to = candidate; + continue; + } + } + let prefix = format!("{owner}::impl::"); + let mut candidates = methods + .values() + .filter(|node| node.id.starts_with(&prefix) && node.label == method); + if let Some(candidate) = candidates.next() + && candidates.next().is_none() + { + edge.to = candidate.id.clone(); + } + } +} + fn collect_owner_names(items: &[Item]) -> BTreeSet { let mut names = BTreeSet::new(); for item in items { @@ -494,25 +533,11 @@ fn ingest_module_items( .trait_ .as_ref() .map(|(_, path, _)| trait_path_key(path)); - let impl_node_id = if let Some(trait_path) = &trait_path { - NodeId::new(format!( - "{owner_node_id}::impl::{}", - hex_encode(trait_path.as_bytes()) - )) + let impl_node_id = if let Some((_, path, _)) = &item_impl.trait_ { + NodeId::new(format!("{owner_node_id}::{}", trait_impl_key(&owner, path))) } else { NodeId::new(format!("{owner_node_id}::impl::inherent")) }; - - // Keep established path-owner IDs stable. References share the - // target type, but must not merge their impls or methods with it. - let impl_node_id = if owner.is_reference { - NodeId::new(format!( - "{impl_node_id}::self::{}", - hex_encode(owner.self_type.as_bytes()) - )) - } else { - impl_node_id - }; let owner_label = if owner.is_reference { &owner.self_type } else { @@ -602,7 +627,7 @@ fn ingest_module_items( for impl_item in item_impl.items { if let ImplItem::Fn(method) = impl_item { - let method_owner = if owner.is_reference { + let method_owner = if item_impl.trait_.is_some() { &impl_node_id } else { &owner_node_id @@ -647,6 +672,7 @@ fn ingest_module_items( Some(owner_name), &context.workspace_dependency_roots, |collector| { + collector.set_impl_self_type(&item_impl.self_ty); collector.visit_impl_item_fn(&method); }, ), diff --git a/crates/sc-lint-boundary/src/graph/mod.rs b/crates/sc-lint-boundary/src/graph/mod.rs index 972f797a..ed3ab4e0 100644 --- a/crates/sc-lint-boundary/src/graph/mod.rs +++ b/crates/sc-lint-boundary/src/graph/mod.rs @@ -1,4 +1,5 @@ use super::*; +use crate::render::hex_encode; use cargo_metadata::MetadataCommand; mod build; @@ -188,6 +189,17 @@ fn impl_owner(self_ty: &Type) -> Result { } } +fn trait_impl_key(owner: &ImplOwner, path: &syn::Path) -> String { + // Keep generic arguments: Trait and Trait are different impls. + let trait_key = path.to_token_stream().to_string().replace(' ', ""); + let mut key = format!("impl::{}", hex_encode(trait_key.as_bytes())); + let self_key = owner.self_type.replace(' ', ""); + if owner.is_reference || self_key != owner.name { + key.push_str(&format!("::self::{}", hex_encode(self_key.as_bytes()))); + } + key +} + pub(crate) fn trait_path_key(path: &syn::Path) -> String { path.segments .iter() diff --git a/crates/sc-lint-boundary/src/graph/reference_collector.rs b/crates/sc-lint-boundary/src/graph/reference_collector.rs index d121e27a..269345b6 100644 --- a/crates/sc-lint-boundary/src/graph/reference_collector.rs +++ b/crates/sc-lint-boundary/src/graph/reference_collector.rs @@ -3,6 +3,7 @@ use super::*; #[derive(Default)] pub(super) struct ReferenceCollector { owner_name: Option, + impl_self_type: Option, local_owner_names: BTreeSet, workspace_dependency_roots: BTreeSet, references: BTreeSet, @@ -16,12 +17,48 @@ impl ReferenceCollector { ) -> Self { Self { owner_name: owner_name.map(ToOwned::to_owned), + impl_self_type: None, local_owner_names: local_owner_names.clone(), workspace_dependency_roots: workspace_dependency_roots.keys().cloned().collect(), references: BTreeSet::new(), } } + pub(super) fn set_impl_self_type(&mut self, self_type: &Type) { + self.impl_self_type = Some(self_type.clone()); + } + + fn qualified_method_path(&self, expression: &syn::ExprPath) -> Option { + let qself = expression.qself.as_ref()?; + if qself.position == 0 || expression.path.segments.len() != qself.position + 1 { + return None; + } + let is_self = matches!(qself.ty.as_ref(), Type::Path(path) if path.path.is_ident("Self")); + let self_type = if is_self { + self.impl_self_type.as_ref()? + } else { + qself.ty.as_ref() + }; + let owner = impl_owner(self_type).ok()?; + let trait_path = syn::Path { + leading_colon: expression.path.leading_colon, + segments: expression + .path + .segments + .iter() + .take(qself.position) + .cloned() + .collect(), + }; + let method = expression.path.segments.last()?; + Some(format!( + "{}::{}::{}", + owner.name, + trait_impl_key(&owner, &trait_path), + method.ident + )) + } + fn into_references(self) -> BTreeSet { self.references } @@ -75,7 +112,14 @@ impl<'ast> Visit<'ast> for ReferenceCollector { } fn visit_expr_path(&mut self, expr_path: &'ast syn::ExprPath) { - self.maybe_insert_path(&expr_path.path, ReferenceKind::Expr); + if let Some(path) = self.qualified_method_path(expr_path) { + self.references.insert(CollectedReference { + path, + kind: ReferenceKind::Expr, + }); + } else { + self.maybe_insert_path(&expr_path.path, ReferenceKind::Expr); + } syn::visit::visit_expr_path(self, expr_path); } diff --git a/crates/sc-lint-boundary/src/lib.rs b/crates/sc-lint-boundary/src/lib.rs index 31bc3180..e75c875e 100644 --- a/crates/sc-lint-boundary/src/lib.rs +++ b/crates/sc-lint-boundary/src/lib.rs @@ -451,6 +451,8 @@ impl GraphBuilder { .then_with(|| left.from.cmp(&right.from)) .then_with(|| left.to.cmp(&right.to)) }); + // Deferred method resolution can make previously distinct edges equal. + self.edges.dedup(); GraphExport { tool: SC_LINT_BOUNDARY_TOOL, diff --git a/crates/sc-lint-boundary/src/tests.rs b/crates/sc-lint-boundary/src/tests.rs index 24a67d91..3839f81e 100644 --- a/crates/sc-lint-boundary/src/tests.rs +++ b/crates/sc-lint-boundary/src/tests.rs @@ -2723,7 +2723,7 @@ fn reference_impls_and_methods_do_not_collide_with_owned_impls() { assert!( methods .iter() - .any(|node| node.id.as_str() == format!("{}::act", owner.id)) + .any(|node| node.id.as_str() == format!("{}::impl::416374696f6e::act", owner.id)) ); for implementation in implementations { assert!( @@ -2776,3 +2776,172 @@ fn reference_method_cycles_keep_the_underlying_type_owner() { .all(|owner| owner.ends_with("::Owner")) ); } + +#[test] +fn trait_adapters_preserve_distinct_methods_and_forwarding_edges() { + for reverse_order in [false, true] { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + let definitions = "pub struct Adapter; pub trait Typed { fn write(&self); } pub trait Legacy { fn write(&self); }"; + let inherent = "impl Adapter { pub fn write(&self) {} }"; + let traits = "impl Typed for Adapter { fn write(&self) { Adapter::write(self); } } impl Legacy for Adapter { fn write(&self) { ::write(self); } }"; + let source = if reverse_order { + format!("{definitions} {traits} {inherent}") + } else { + format!("{definitions} {inherent} {traits}") + }; + fixture.write_source("example", "lib.rs", &source); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let owner = graph + .nodes + .iter() + .find(|node| node.label == "Adapter") + .unwrap(); + let methods: Vec<_> = graph + .nodes + .iter() + .filter(|node| { + node.kind == "method" + && node.label == "write" + && node.id.starts_with(owner.id.as_str()) + }) + .collect(); + assert_eq!(methods.len(), 3); + let inherent = methods + .iter() + .find(|node| node.impl_kind == Some(ImplKind::Inherent)) + .unwrap(); + let typed = methods + .iter() + .find(|node| node.impl_trait.as_deref() == Some("Typed")) + .unwrap(); + let legacy = methods + .iter() + .find(|node| node.impl_trait.as_deref() == Some("Legacy")) + .unwrap(); + assert_eq!(inherent.id.as_str(), format!("{}::write", owner.id)); + for (from, to) in [ + (typed.id.clone(), inherent.id.clone()), + (legacy.id.clone(), typed.id.clone()), + ] { + assert_ne!(from, to); + assert!( + graph.edges.iter().any(|edge| edge.kind == "references_expr" + && edge.from == from + && edge.to == to) + ); + } + assert!( + !graph + .edges + .iter() + .any(|edge| edge.kind == "references_expr" && edge.from == edge.to) + ); + let report = analyze_workspace(&AnalyzeOptions { + root: fixture.root().to_path_buf(), + format: OutputFormat::Json, + rule: Some(RuleFilter::Cycles), + }) + .unwrap(); + assert!( + !report.findings.is_empty(), + "identity correction must not suppress owner-level cycle policy" + ); + assert!(report.findings.iter().all(|finding| { + finding + .owner_ids + .iter() + .all(|id| id.as_str() == owner.id.as_str()) + })); + } +} + +#[test] +fn generic_trait_and_self_arguments_distinguish_impl_methods() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Adapter(T); + pub trait Convert { fn convert(&self); } + impl Convert for Adapter { fn convert(&self) {} } + impl Convert for Adapter { fn convert(&self) {} } + impl Convert for Adapter { fn convert(&self) {} } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let implementations: Vec<_> = graph + .nodes + .iter() + .filter(|node| node.kind == "impl" && node.impl_trait.as_deref() == Some("Convert")) + .collect(); + let methods: Vec<_> = graph + .nodes + .iter() + .filter(|node| node.kind == "method" && node.impl_trait.as_deref() == Some("Convert")) + .collect(); + assert_eq!(implementations.len(), 3); + assert_eq!(methods.len(), 3); + for implementation in implementations { + assert_eq!( + graph + .edges + .iter() + .filter(|edge| edge.kind == "contains" + && edge.from == implementation.id + && methods.iter().any(|method| method.id == edge.to)) + .count(), + 1 + ); + } +} + +#[test] +fn trait_self_calls_resolve_without_inventing_inherent_methods() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Adapter; + pub trait Action { fn first(); fn second(); } + impl Action for Adapter { fn first() { Self::second(); ::second(); } fn second() {} } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let first = graph + .nodes + .iter() + .find(|node| node.label == "first" && node.impl_trait.as_deref() == Some("Action")) + .unwrap(); + let second = graph + .nodes + .iter() + .find(|node| node.label == "second" && node.impl_trait.as_deref() == Some("Action")) + .unwrap(); + assert_eq!( + graph + .edges + .iter() + .filter(|edge| edge.kind == "references_expr" + && edge.from == first.id + && edge.to == second.id) + .count(), + 1 + ); +} diff --git a/docs/sc-lint-boundary/graph-schema.md b/docs/sc-lint-boundary/graph-schema.md index 42197ce0..648d9203 100644 --- a/docs/sc-lint-boundary/graph-schema.md +++ b/docs/sc-lint-boundary/graph-schema.md @@ -69,6 +69,22 @@ Notes: - `crate` - `restricted` +### Method identity + +Inherent methods retain `::` IDs. Trait methods use +`::`, so traits with the same method name do not share a node +with each other or with inherent methods. Implementation keys retain trait +arguments and nontrivial self types, including references and generic arguments. +Consumers should use node metadata and `contains`/`declares`/`targets` edges +rather than infer trait ownership from a method-name suffix. + +Qualified trait calls resolve to the corresponding implementation method. +Unqualified calls prefer an existing inherent target; otherwise the analyzer +uses the current trait implementation or a unique trait-method candidate. +Ambiguous candidates remain unresolved instead of being merged. Cycle rules +still operate on the underlying type owner; distinct methods do not imply a +waiver of owner-level cycle diagnostics. + ## Edge Model Current edge kinds: