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
62 changes: 44 additions & 18 deletions crates/sc-lint-boundary/src/graph/build.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::*;
use crate::render::hex_encode;
use std::collections::BTreeMap;
use std::collections::BTreeSet;

Expand Down Expand Up @@ -112,9 +111,49 @@ pub(crate) fn build_workspace_graph(root: &Path) -> Result<GraphExport> {
}
}

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<String> {
let mut names = BTreeSet::new();
for item in items {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
},
),
Expand Down
12 changes: 12 additions & 0 deletions crates/sc-lint-boundary/src/graph/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::*;
use crate::render::hex_encode;
use cargo_metadata::MetadataCommand;

mod build;
Expand Down Expand Up @@ -188,6 +189,17 @@ fn impl_owner(self_ty: &Type) -> Result<ImplOwner> {
}
}

fn trait_impl_key(owner: &ImplOwner, path: &syn::Path) -> String {
// Keep generic arguments: Trait<u8> and Trait<u16> 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()
Expand Down
46 changes: 45 additions & 1 deletion crates/sc-lint-boundary/src/graph/reference_collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use super::*;
#[derive(Default)]
pub(super) struct ReferenceCollector {
owner_name: Option<String>,
impl_self_type: Option<Type>,
local_owner_names: BTreeSet<String>,
workspace_dependency_roots: BTreeSet<String>,
references: BTreeSet<CollectedReference>,
Expand All @@ -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<String> {
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<CollectedReference> {
self.references
}
Expand Down Expand Up @@ -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);
}

Expand Down
2 changes: 2 additions & 0 deletions crates/sc-lint-boundary/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
171 changes: 170 additions & 1 deletion crates/sc-lint-boundary/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down Expand Up @@ -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) { <Self as Typed>::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>(T);
pub trait Convert<T> { fn convert(&self); }
impl Convert<u8> for Adapter<u8> { fn convert(&self) {} }
impl Convert<u16> for Adapter<u8> { fn convert(&self) {} }
impl Convert<u8> for Adapter<u16> { 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(); <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 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
);
}
Loading
Loading