From 74bfd418bf206cb97230fc728aa58ebb832d67e2 Mon Sep 17 00:00:00 2001 From: Rand Lee Date: Fri, 18 Sep 2026 21:22:46 -0700 Subject: [PATCH] fix(boundary): support reference impl owners without graph collisions --- crates/sc-lint-boundary/src/analysis.rs | 3 +- crates/sc-lint-boundary/src/graph/build.rs | 32 ++++- crates/sc-lint-boundary/src/graph/mod.rs | 67 +++++++-- crates/sc-lint-boundary/src/tests.rs | 149 ++++++++++++++++++++- 4 files changed, 233 insertions(+), 18 deletions(-) diff --git a/crates/sc-lint-boundary/src/analysis.rs b/crates/sc-lint-boundary/src/analysis.rs index 386d2e55..c35226f8 100644 --- a/crates/sc-lint-boundary/src/analysis.rs +++ b/crates/sc-lint-boundary/src/analysis.rs @@ -520,7 +520,8 @@ fn owner_id_for_node_id(node_id: &NodeId, node_kind: &str) -> Option { .rsplit_once("::") .map(|(parent, _)| OwnerId::new(parent)), "method" => node_id - .rsplit_once("::") + .split_once("::impl::") + .or_else(|| node_id.rsplit_once("::")) .map(|(parent, _)| OwnerId::new(parent)), "variant" => node_id .rsplit_once("::variant::") diff --git a/crates/sc-lint-boundary/src/graph/build.rs b/crates/sc-lint-boundary/src/graph/build.rs index 881f74e8..e91ec918 100644 --- a/crates/sc-lint-boundary/src/graph/build.rs +++ b/crates/sc-lint-boundary/src/graph/build.rs @@ -487,7 +487,8 @@ fn ingest_module_items( ); } Item::Impl(item_impl) => { - let owner_name = impl_owner_name(&item_impl.self_ty)?; + let owner = impl_owner(&item_impl.self_ty)?; + let owner_name = &owner.name; let owner_node_id = NodeId::new(format!("{parent_module_id}::{owner_name}")); let trait_path = item_impl .trait_ @@ -502,6 +503,22 @@ fn ingest_module_items( 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 { + owner_name + }; + if !builder .nodes .iter() @@ -533,8 +550,8 @@ fn ingest_module_items( kind: NodeKind::Impl.as_str(), label: trait_path .as_ref() - .map(|path| format!("impl {path} for {owner_name}")) - .unwrap_or_else(|| format!("impl {owner_name}")), + .map(|path| format!("impl {path} for {owner_label}")) + .unwrap_or_else(|| format!("impl {owner_label}")), visibility: None, package: context.package_name.clone(), target: Some(context.target_name.clone()), @@ -585,8 +602,13 @@ 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 { + &impl_node_id + } else { + &owner_node_id + }; let method_id = - NodeId::new(format!("{owner_node_id}::{}", method.sig.ident)); + NodeId::new(format!("{method_owner}::{}", method.sig.ident)); builder.add_node(GraphNode { id: method_id.clone(), kind: NodeKind::Method.as_str(), @@ -622,7 +644,7 @@ fn ingest_module_items( module_path, collect_references_with( &local_owner_names, - Some(&owner_name), + Some(owner_name), &context.workspace_dependency_roots, |collector| { 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 4178314f..972f797a 100644 --- a/crates/sc-lint-boundary/src/graph/mod.rs +++ b/crates/sc-lint-boundary/src/graph/mod.rs @@ -145,20 +145,44 @@ fn parse_rust_file(path: &Path) -> Result { .with_context(|| format!("failed to parse Rust source {}", path.display())) } -fn impl_owner_name(self_ty: &Type) -> Result { +struct ImplOwner { + name: String, + self_type: String, + is_reference: bool, +} + +fn impl_owner(self_ty: &Type) -> Result { match self_ty { Type::Path(type_path) => { - // `syn::Type::Path` always stores at least one segment for a valid path type. - if let Some(segment) = type_path.path.segments.last() { - Ok(segment.ident.to_string()) + let segment = + type_path.path.segments.last().ok_or_else(|| { + anyhow::anyhow!("impl owner path is missing a terminal segment") + })?; + Ok(ImplOwner { + name: segment.ident.to_string(), + self_type: self_ty.to_token_stream().to_string(), + is_reference: false, + }) + } + Type::Reference(reference) => { + let mut owner = impl_owner(&reference.elem)?; + let lifetime = reference + .lifetime + .as_ref() + .map_or_else(String::new, |value| format!("{} ", value.to_token_stream())); + let mutability = if reference.mutability.is_some() { + "mut " } else { - Err(anyhow::anyhow!( - "impl owner path is missing a terminal segment" - )) - } + "" + }; + owner.self_type = format!("&{lifetime}{mutability}{}", owner.self_type); + owner.is_reference = true; + Ok(owner) } + Type::Paren(paren) => impl_owner(&paren.elem), + Type::Group(group) => impl_owner(&group.elem), _ => anyhow::bail!( - "unsupported impl owner type `{}`; only path owners are supported", + "unsupported impl owner type `{}`; expected a path or reference to a path", self_ty.to_token_stream() ), } @@ -205,3 +229,28 @@ pub(crate) fn load_metadata(root: &Path) -> Result { .exec() .with_context(|| format!("failed to load cargo metadata for {}", root.display())) } + +#[cfg(test)] +mod owner_tests { + use super::*; + + #[test] + fn parenthesized_and_grouped_owners_are_transparent() { + let path: Type = syn::parse_quote!(Owner); + let parenthesized: Type = syn::parse_quote!((Owner)); + let grouped = Type::Group(syn::TypeGroup { + group_token: Default::default(), + elem: Box::new(parenthesized), + }); + let plain = impl_owner(&path).unwrap(); + let wrapped = impl_owner(&grouped).unwrap(); + assert_eq!(plain.name, wrapped.name); + assert_eq!(plain.self_type, wrapped.self_type); + assert!(!wrapped.is_reference); + let reference: Type = syn::parse_quote!((&'a mut (Owner))); + let wrapped = impl_owner(&reference).unwrap(); + assert_eq!(wrapped.name, "Owner"); + assert_eq!(wrapped.self_type, "&'a mut Owner"); + assert!(wrapped.is_reference); + } +} diff --git a/crates/sc-lint-boundary/src/tests.rs b/crates/sc-lint-boundary/src/tests.rs index 16f88628..24a67d91 100644 --- a/crates/sc-lint-boundary/src/tests.rs +++ b/crates/sc-lint-boundary/src/tests.rs @@ -2042,7 +2042,7 @@ fn preserves_full_trait_path_in_trait_impl_self_loop_messages() { } #[test] -fn rejects_non_path_impl_owners() { +fn rejects_impl_owners_without_a_named_target() { let fixture = WorkspaceFixture::new(); fixture.write_workspace_root(); fixture.write_package_manifest("example"); @@ -2052,7 +2052,7 @@ fn rejects_non_path_impl_owners() { r#" pub struct Loop; - impl core::fmt::Display for &Loop { + impl core::fmt::Display for (Loop, Loop) { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { write!(f, "loop") } @@ -2067,7 +2067,7 @@ fn rejects_non_path_impl_owners() { let message = format!("{error:#}"); assert!(message.contains("unsupported impl owner type")); - assert!(message.contains("&Loop") || message.contains("& Loop")); + assert!(message.contains("(Loop , Loop)")); } #[test] @@ -2633,3 +2633,146 @@ fn trim_indentation(input: &str) -> String { } output } + +#[test] +fn supports_generic_field_value_reference_impl_owner() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + mod serde { pub trait Serialize {} } + pub struct FieldValue<'a, T: ?Sized>(&'a T); + pub trait SerializeKindTag {} + impl SerializeKindTag for &FieldValue<'_, T> {} + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let owner = graph + .nodes + .iter() + .find(|node| node.label == "FieldValue") + .unwrap(); + let implementation = graph + .nodes + .iter() + .find(|node| node.impl_trait.as_deref() == Some("SerializeKindTag")) + .unwrap(); + assert!(implementation.label.contains("&FieldValue")); + assert!( + graph + .edges + .iter() + .any(|edge| edge.kind == EdgeKind::Targets.as_str() + && edge.from == implementation.id + && edge.to == owner.id) + ); +} + +#[test] +fn reference_impls_and_methods_do_not_collide_with_owned_impls() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Owner; + pub trait Action { fn act(&self); } + impl Action for Owner { fn act(&self) {} } + impl Action for &Owner { fn act(&self) {} } + impl Action for &mut Owner { fn act(&self) {} } + impl Action for &&Owner { fn act(&self) {} } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let owner = graph + .nodes + .iter() + .find(|node| node.label == "Owner") + .unwrap(); + let implementations: Vec<_> = graph + .nodes + .iter() + .filter(|node| node.kind == "impl") + .collect(); + let methods: Vec<_> = graph + .nodes + .iter() + .filter(|node| node.kind == "method" && node.impl_trait.as_deref() == Some("Action")) + .collect(); + assert_eq!(implementations.len(), 4); + assert_eq!(methods.len(), 4); + let owned = implementations + .iter() + .find(|node| node.label == "impl Action for Owner") + .unwrap(); + assert_eq!( + owned.id.as_str(), + format!("{}::impl::416374696f6e", owner.id) + ); + assert!( + methods + .iter() + .any(|node| node.id.as_str() == format!("{}::act", owner.id)) + ); + for implementation in implementations { + assert!( + graph + .edges + .iter() + .any(|edge| edge.kind == EdgeKind::Targets.as_str() + && edge.from == implementation.id + && edge.to == owner.id) + ); + let method_edges: Vec<_> = graph + .edges + .iter() + .filter(|edge| { + edge.kind == EdgeKind::Contains.as_str() + && edge.from == implementation.id + && methods.iter().any(|method| method.id == edge.to) + }) + .collect(); + assert_eq!(method_edges.len(), 1); + } +} + +#[test] +fn reference_method_cycles_keep_the_underlying_type_owner() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Owner; + pub trait Action { fn act(&self); } + impl Action for &Owner { fn act(&self) { let _ = Owner; } } + "#, + ); + let report = analyze_workspace(&AnalyzeOptions { + root: fixture.root().to_path_buf(), + format: OutputFormat::Json, + rule: Some(RuleFilter::Cycles), + }) + .unwrap(); + assert_eq!(report.findings.len(), 1); + assert_eq!(report.findings[0].rule_id, RuleId::ScbCycle003); + assert!( + report.findings[0] + .owner_ids + .iter() + .all(|owner| owner.ends_with("::Owner")) + ); +}