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
16 changes: 13 additions & 3 deletions crates/sc-lint-boundary/src/graph/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +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_impl_ids = &builder.reference_impl_ids;
for edge in &mut builder.edges {
if !matches!(edge.kind, "references" | "references_expr") || known.contains(&edge.to) {
continue;
Expand All @@ -143,9 +144,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_impl_ids.contains(&NodeId::new(impl_id)))
});
if let Some(candidate) = candidates.next()
&& candidates.next().is_none()
{
Expand Down Expand Up @@ -544,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()
Expand Down
8 changes: 7 additions & 1 deletion crates/sc-lint-boundary/src/graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ImplOwner> {
Expand All @@ -163,6 +164,10 @@ fn impl_owner(self_ty: &Type) -> Result<ImplOwner> {
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) => {
Expand All @@ -178,6 +183,7 @@ fn impl_owner(self_ty: &Type) -> Result<ImplOwner> {
};
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),
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions crates/sc-lint-boundary/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ impl ItemVisibility {
struct GraphBuilder {
nodes: Vec<GraphNode>,
edges: Vec<GraphEdge>,
reference_impl_ids: BTreeSet<NodeId>,
}

impl GraphBuilder {
Expand Down
231 changes: 231 additions & 0 deletions crates/sc-lint-boundary/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3009,3 +3009,234 @@ 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<T> { fn first(); fn second(); }
impl Convert<u8> for &Adapter {
fn first() { <&Adapter as Convert<u8>>::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>(T);
pub trait Convert<T> { fn convert(); }
impl Convert<u8> for &Foo<u8> { fn convert() {} }
impl Convert<u16> for &Foo<u8> { fn convert() {} }
impl Convert<u8> for &Foo<u16> { 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::<BTreeSet<_>>()
.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(); <Self as Action>::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::"));
}

#[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<T> { 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
}));
}
Loading