From bd481201c4c685258867ec7a9375dbe3b2528ee9 Mon Sep 17 00:00:00 2001 From: Rand Lee Date: Sat, 19 Sep 2026 13:08:47 -0700 Subject: [PATCH 1/2] fix(boundary): harden trait method resolution --- crates/sc-lint-boundary/src/graph/build.rs | 17 +- crates/sc-lint-boundary/src/graph/mod.rs | 8 +- crates/sc-lint-boundary/src/tests.rs | 197 +++++++++++++++++++++ 3 files changed, 218 insertions(+), 4 deletions(-) diff --git a/crates/sc-lint-boundary/src/graph/build.rs b/crates/sc-lint-boundary/src/graph/build.rs index d31f365a..333b3b25 100644 --- a/crates/sc-lint-boundary/src/graph/build.rs +++ b/crates/sc-lint-boundary/src/graph/build.rs @@ -123,6 +123,12 @@ fn resolve_trait_method_edges(builder: &mut GraphBuilder) { .map(|node| (node.id.clone(), node)) .collect(); let known: BTreeSet<_> = builder.nodes.iter().map(|node| node.id.clone()).collect(); + let reference_impls: BTreeSet<_> = builder + .nodes + .iter() + .filter(|node| node.kind == "impl" && node.label.contains(" for &")) + .map(|node| node.id.clone()) + .collect(); for edge in &mut builder.edges { if !matches!(edge.kind, "references" | "references_expr") || known.contains(&edge.to) { continue; @@ -143,9 +149,14 @@ fn resolve_trait_method_edges(builder: &mut GraphBuilder) { } } let prefix = format!("{owner}::impl::"); - let mut candidates = methods - .values() - .filter(|node| node.id.starts_with(&prefix) && node.label == method); + let mut candidates = methods.values().filter(|node| { + node.id.starts_with(&prefix) + && node.label == method + && node + .id + .rsplit_once("::") + .is_none_or(|(impl_id, _)| !reference_impls.contains(&NodeId::new(impl_id))) + }); if let Some(candidate) = candidates.next() && candidates.next().is_none() { diff --git a/crates/sc-lint-boundary/src/graph/mod.rs b/crates/sc-lint-boundary/src/graph/mod.rs index ed3ab4e0..6e706191 100644 --- a/crates/sc-lint-boundary/src/graph/mod.rs +++ b/crates/sc-lint-boundary/src/graph/mod.rs @@ -150,6 +150,7 @@ struct ImplOwner { name: String, self_type: String, is_reference: bool, + needs_self_discriminator: bool, } fn impl_owner(self_ty: &Type) -> Result { @@ -163,6 +164,10 @@ fn impl_owner(self_ty: &Type) -> Result { name: segment.ident.to_string(), self_type: self_ty.to_token_stream().to_string(), is_reference: false, + // A qualified path such as `crate::Owner` names the same type + // as `Owner`; retain the established implementation ID. Only + // arguments distinguish otherwise-overlapping path owners. + needs_self_discriminator: !matches!(segment.arguments, syn::PathArguments::None), }) } Type::Reference(reference) => { @@ -178,6 +183,7 @@ fn impl_owner(self_ty: &Type) -> Result { }; owner.self_type = format!("&{lifetime}{mutability}{}", owner.self_type); owner.is_reference = true; + owner.needs_self_discriminator = true; Ok(owner) } Type::Paren(paren) => impl_owner(&paren.elem), @@ -194,7 +200,7 @@ fn trait_impl_key(owner: &ImplOwner, path: &syn::Path) -> String { 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 { + if owner.needs_self_discriminator { key.push_str(&format!("::self::{}", hex_encode(self_key.as_bytes()))); } key diff --git a/crates/sc-lint-boundary/src/tests.rs b/crates/sc-lint-boundary/src/tests.rs index f6f7edfb..9ac4b450 100644 --- a/crates/sc-lint-boundary/src/tests.rs +++ b/crates/sc-lint-boundary/src/tests.rs @@ -3009,3 +3009,200 @@ fn trait_self_calls_resolve_without_inventing_inherent_methods() { 1 ); } + +#[test] +fn qualified_generic_reference_trait_calls_resolve_to_the_matching_impl_method() { + 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 Convert { fn first(); fn second(); } + impl Convert for &Adapter { + fn first() { <&Adapter as Convert>::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("Convert")) + .unwrap(); + let second = graph + .nodes + .iter() + .find(|node| node.label == "second" && node.impl_trait.as_deref() == Some("Convert")) + .unwrap(); + assert!(graph.edges.iter().any(|edge| { + edge.kind == "references_expr" && edge.from == first.id && edge.to == second.id + })); +} + +#[test] +fn generic_reference_impls_keep_separate_nodes_and_do_not_capture_owned_calls() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Foo(T); + pub trait Convert { fn convert(); } + impl Convert for &Foo { fn convert() {} } + impl Convert for &Foo { fn convert() {} } + impl Convert for &Foo { fn convert() {} } + pub fn unresolved() { Foo::convert(); } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let methods: Vec<_> = graph + .nodes + .iter() + .filter(|node| node.kind == "method" && node.impl_trait.as_deref() == Some("Convert")) + .collect(); + assert_eq!(methods.len(), 3); + assert_eq!( + methods + .iter() + .map(|node| &node.id) + .collect::>() + .len(), + 3 + ); + let unresolved = graph + .nodes + .iter() + .find(|node| node.kind == "function" && node.label == "unresolved") + .unwrap(); + let expected_target = format!( + "{}::Foo::convert", + unresolved.id.rsplit_once("::").unwrap().0 + ); + assert!(graph.edges.iter().any(|edge| { + edge.kind == "references_expr" && edge.from == unresolved.id && edge.to == expected_target + })); + assert!(!graph.edges.iter().any(|edge| { + edge.kind == "references_expr" + && edge.from == unresolved.id + && methods.iter().any(|method| edge.to == method.id) + })); +} + +#[test] +fn ambiguous_unqualified_trait_method_call_remains_unresolved() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Foo; + pub trait First { fn call(); } + pub trait Second { fn call(); } + impl First for Foo { fn call() {} } + impl Second for Foo { fn call() {} } + pub fn unresolved() { Foo::call(); } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let unresolved = graph + .nodes + .iter() + .find(|node| node.kind == "function" && node.label == "unresolved") + .unwrap(); + let expected_target = format!("{}::Foo::call", unresolved.id.rsplit_once("::").unwrap().0); + assert!(graph.edges.iter().any(|edge| { + edge.kind == "references_expr" && edge.from == unresolved.id && edge.to == expected_target + })); +} + +#[test] +fn self_method_prefers_inherent_dispatch_while_ufcs_dispatches_to_the_trait_impl() { + 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 Adapter { 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 inherent_second = graph + .nodes + .iter() + .find(|node| node.label == "second" && node.impl_kind == Some(ImplKind::Inherent)) + .unwrap(); + let trait_second = graph + .nodes + .iter() + .find(|node| node.label == "second" && node.impl_trait.as_deref() == Some("Action")) + .unwrap(); + for target in [&inherent_second.id, &trait_second.id] { + assert!(graph.edges.iter().any(|edge| { + edge.kind == "references_expr" && edge.from == first.id && edge.to == *target + })); + } +} + +#[test] +fn qualified_non_generic_self_path_keeps_the_established_trait_impl_id() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Foo; + pub trait Action { fn act(); } + impl Action for crate::Foo { fn act() {} } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let owner = graph.nodes.iter().find(|node| node.label == "Foo").unwrap(); + let implementation = graph + .nodes + .iter() + .find(|node| node.kind == "impl" && node.impl_trait.as_deref() == Some("Action")) + .unwrap(); + assert_eq!( + implementation.id.as_str(), + format!("{}::impl::416374696f6e", owner.id) + ); + assert!(!implementation.id.as_str().contains("::self::")); +} From 15e4000095a2e55f9206838b5d8f5f17c35e0e14 Mon Sep 17 00:00:00 2001 From: Rand Lee Date: Sat, 19 Sep 2026 13:10:31 -0700 Subject: [PATCH 2/2] fix(boundary): track reference impls structurally --- crates/sc-lint-boundary/src/graph/build.rs | 13 ++++----- crates/sc-lint-boundary/src/lib.rs | 1 + crates/sc-lint-boundary/src/tests.rs | 34 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/crates/sc-lint-boundary/src/graph/build.rs b/crates/sc-lint-boundary/src/graph/build.rs index 333b3b25..bce34156 100644 --- a/crates/sc-lint-boundary/src/graph/build.rs +++ b/crates/sc-lint-boundary/src/graph/build.rs @@ -123,12 +123,7 @@ fn resolve_trait_method_edges(builder: &mut GraphBuilder) { .map(|node| (node.id.clone(), node)) .collect(); let known: BTreeSet<_> = builder.nodes.iter().map(|node| node.id.clone()).collect(); - let reference_impls: BTreeSet<_> = builder - .nodes - .iter() - .filter(|node| node.kind == "impl" && node.label.contains(" for &")) - .map(|node| node.id.clone()) - .collect(); + let reference_impl_ids = &builder.reference_impl_ids; for edge in &mut builder.edges { if !matches!(edge.kind, "references" | "references_expr") || known.contains(&edge.to) { continue; @@ -155,7 +150,7 @@ fn resolve_trait_method_edges(builder: &mut GraphBuilder) { && node .id .rsplit_once("::") - .is_none_or(|(impl_id, _)| !reference_impls.contains(&NodeId::new(impl_id))) + .is_none_or(|(impl_id, _)| !reference_impl_ids.contains(&NodeId::new(impl_id))) }); if let Some(candidate) = candidates.next() && candidates.next().is_none() @@ -555,6 +550,10 @@ fn ingest_module_items( owner_name }; + if owner.is_reference { + builder.reference_impl_ids.insert(impl_node_id.clone()); + } + if !builder .nodes .iter() diff --git a/crates/sc-lint-boundary/src/lib.rs b/crates/sc-lint-boundary/src/lib.rs index c8c3a569..84438340 100644 --- a/crates/sc-lint-boundary/src/lib.rs +++ b/crates/sc-lint-boundary/src/lib.rs @@ -395,6 +395,7 @@ impl ItemVisibility { struct GraphBuilder { nodes: Vec, edges: Vec, + reference_impl_ids: BTreeSet, } impl GraphBuilder { diff --git a/crates/sc-lint-boundary/src/tests.rs b/crates/sc-lint-boundary/src/tests.rs index 9ac4b450..3e39a5d7 100644 --- a/crates/sc-lint-boundary/src/tests.rs +++ b/crates/sc-lint-boundary/src/tests.rs @@ -3206,3 +3206,37 @@ fn qualified_non_generic_self_path_keeps_the_established_trait_impl_id() { ); assert!(!implementation.id.as_str().contains("::self::")); } + +#[test] +fn non_reference_generic_trait_impl_remains_an_unqualified_method_candidate() { + let fixture = WorkspaceFixture::new(); + fixture.write_workspace_root(); + fixture.write_package_manifest("example"); + fixture.write_source( + "example", + "lib.rs", + r#" + pub struct Foo; + pub trait Borrowed { fn call(); } + impl Borrowed<&'static str> for Foo { fn call() {} } + pub fn resolved() { Foo::call(); } + "#, + ); + let graph = export_workspace_graph(&ExportGraphOptions { + root: fixture.root().to_path_buf(), + }) + .unwrap(); + let resolved = graph + .nodes + .iter() + .find(|node| node.kind == "function" && node.label == "resolved") + .unwrap(); + let method = graph + .nodes + .iter() + .find(|node| node.kind == "method" && node.impl_trait.as_deref() == Some("Borrowed")) + .unwrap(); + assert!(graph.edges.iter().any(|edge| { + edge.kind == "references_expr" && edge.from == resolved.id && edge.to == method.id + })); +}