fix/audit-findings #5
2 changed files with 131 additions and 12 deletions
fix(rust): resolve methods on the type the caller means, not its leaf name
The Rust impl index keyed types by leaf name, so `a::Config` and
`b::Config` shared one `(Config, load)` entry and the inherent method
won: `use crate:🅱️:Config; c.load()` produced a `declared-receiver-type`
edge to `a::Config::load`.
The index now records which files declare each type name and the file
of every impl. When several files declare a name, a call resolves only
among impls of the type the calling file means (its own declaration,
then a `use` binding, then the only declaration); with no such binding
the call is left external as `ambiguous` instead of guessed. Unique
names behave as before.
Test: rust_methods::same_named_types_in_different_modules_resolve_through_use_bindings
fails before this change (edge to src/a.rs).
commit
b281a52255
94
src/scan.rs
94
src/scan.rs
|
|
@ -140,16 +140,21 @@ struct RustCall {
|
||||||
receiver: Option<crate::source::Receiver>,
|
receiver: Option<crate::source::Receiver>,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A method node id and the trait it implements, if any.
|
/// A method node id, the trait it implements (if any), and the file of its `impl`.
|
||||||
type RustMethod = (String, Option<String>);
|
type RustMethod = (String, Option<String>, String);
|
||||||
|
|
||||||
/// Crate-wide Rust impl/trait facts keyed by leaf type names.
|
/// Crate-wide Rust impl/trait facts keyed by leaf type names. Leaf names are not
|
||||||
|
/// identities: when several files declare the same name, `rust_type_file` settles
|
||||||
|
/// which one a use site means (declaring file, then `use` binding) or reports it
|
||||||
|
/// as ambiguous.
|
||||||
#[derive(Default)]
|
#[derive(Default)]
|
||||||
struct RustIndex {
|
struct RustIndex {
|
||||||
/// (type or trait, method) -> candidate methods.
|
/// (type or trait, method) -> candidate methods.
|
||||||
methods: HashMap<(String, String), Vec<RustMethod>>,
|
methods: HashMap<(String, String), Vec<RustMethod>>,
|
||||||
/// Struct, enum, and trait names declared in the repository.
|
/// Struct, enum, and trait names declared in the repository.
|
||||||
types: HashSet<String>,
|
types: HashSet<String>,
|
||||||
|
/// Leaf type/trait name -> files declaring it.
|
||||||
|
type_files: HashMap<String, Vec<String>>,
|
||||||
/// (struct, field) -> declared type.
|
/// (struct, field) -> declared type.
|
||||||
fields: HashMap<(String, String), crate::source::CppType>,
|
fields: HashMap<(String, String), crate::source::CppType>,
|
||||||
/// Method node id -> declared return type.
|
/// Method node id -> declared return type.
|
||||||
|
|
@ -657,6 +662,14 @@ impl ScanState {
|
||||||
let Some(id) = id else { continue };
|
let Some(id) = id else { continue };
|
||||||
if declaration.kind == NodeKind::Class {
|
if declaration.kind == NodeKind::Class {
|
||||||
self.rust.types.insert(declaration.name.clone());
|
self.rust.types.insert(declaration.name.clone());
|
||||||
|
let files = self
|
||||||
|
.rust
|
||||||
|
.type_files
|
||||||
|
.entry(declaration.name.clone())
|
||||||
|
.or_default();
|
||||||
|
if !files.iter().any(|file| file == path) {
|
||||||
|
files.push(path.to_owned());
|
||||||
|
}
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
let Some(owner) = declaration
|
let Some(owner) = declaration
|
||||||
|
|
@ -676,7 +689,7 @@ impl ScanState {
|
||||||
.methods
|
.methods
|
||||||
.entry((owner, declaration.name.clone()))
|
.entry((owner, declaration.name.clone()))
|
||||||
.or_default()
|
.or_default()
|
||||||
.push((id.clone(), implemented));
|
.push((id.clone(), implemented, path.to_owned()));
|
||||||
if let Some(returns) = &declaration.return_type {
|
if let Some(returns) = &declaration.return_type {
|
||||||
self.rust.returns.insert(id.clone(), returns.clone());
|
self.rust.returns.insert(id.clone(), returns.clone());
|
||||||
}
|
}
|
||||||
|
|
@ -1690,7 +1703,7 @@ impl ScanState {
|
||||||
(field.accessed(true), "declared-receiver-type")
|
(field.accessed(true), "declared-receiver-type")
|
||||||
}
|
}
|
||||||
Some(Receiver::Returned { ty, func, fallible }) => (
|
Some(Receiver::Returned { ty, func, fallible }) => (
|
||||||
self.rust_constructed_type(ty, func, *fallible, call)
|
self.rust_constructed_type(ty, func, *fallible, call, path)
|
||||||
.ok_or("receiver-type-unknown")?,
|
.ok_or("receiver-type-unknown")?,
|
||||||
"declared-receiver-type",
|
"declared-receiver-type",
|
||||||
),
|
),
|
||||||
|
|
@ -1699,7 +1712,7 @@ impl ScanState {
|
||||||
if self.rust_type_is_foreign(&ty, path) {
|
if self.rust_type_is_foreign(&ty, path) {
|
||||||
return Err("receiver-type-external");
|
return Err("receiver-type-external");
|
||||||
}
|
}
|
||||||
self.rust_method(&ty, method).map(|id| (id, label))
|
self.rust_method(&ty, method, path).map(|id| (id, label))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A type name the caller's file imports from outside the crate.
|
/// A type name the caller's file imports from outside the crate.
|
||||||
|
|
@ -1746,30 +1759,86 @@ impl ScanState {
|
||||||
if !self.rust.types.contains(&ty) {
|
if !self.rust.types.contains(&ty) {
|
||||||
return Err("qualifier-unknown");
|
return Err("qualifier-unknown");
|
||||||
}
|
}
|
||||||
self.rust_method(&ty, method)
|
self.rust_method(&ty, method, path)
|
||||||
}
|
}
|
||||||
|
|
||||||
fn rust_method(&self, ty: &str, method: &str) -> Result<String, &'static str> {
|
fn rust_method(&self, ty: &str, method: &str, path: &str) -> Result<String, &'static str> {
|
||||||
let candidates = self
|
let candidates = self
|
||||||
.rust
|
.rust
|
||||||
.methods
|
.methods
|
||||||
.get(&(ty.to_owned(), method.to_owned()))
|
.get(&(ty.to_owned(), method.to_owned()))
|
||||||
.map(Vec::as_slice)
|
.map(Vec::as_slice)
|
||||||
.unwrap_or_default();
|
.unwrap_or_default();
|
||||||
let inherent: Vec<_> = candidates.iter().filter(|(_, t)| t.is_none()).collect();
|
// Several repository types share this leaf name: keep only the impls of the
|
||||||
let chosen: Vec<_> = if inherent.is_empty() {
|
// one this use site means, or refuse to guess.
|
||||||
|
let candidates: Vec<&RustMethod> = if self
|
||||||
|
.rust
|
||||||
|
.type_files
|
||||||
|
.get(ty)
|
||||||
|
.is_some_and(|files| files.len() > 1)
|
||||||
|
{
|
||||||
|
let Some(meant) = self.rust_type_file(ty, path) else {
|
||||||
|
return Err("ambiguous");
|
||||||
|
};
|
||||||
|
candidates
|
||||||
|
.iter()
|
||||||
|
.filter(|(_, _, impl_path)| {
|
||||||
|
self.rust_type_file(ty, impl_path).as_deref() == Some(meant.as_str())
|
||||||
|
})
|
||||||
|
.collect()
|
||||||
|
} else {
|
||||||
candidates.iter().collect()
|
candidates.iter().collect()
|
||||||
|
};
|
||||||
|
let inherent: Vec<_> = candidates
|
||||||
|
.iter()
|
||||||
|
.copied()
|
||||||
|
.filter(|(_, t, _)| t.is_none())
|
||||||
|
.collect();
|
||||||
|
let chosen: Vec<_> = if inherent.is_empty() {
|
||||||
|
candidates
|
||||||
} else {
|
} else {
|
||||||
inherent
|
inherent
|
||||||
};
|
};
|
||||||
match chosen.as_slice() {
|
match chosen.as_slice() {
|
||||||
[(id, _)] => Ok(id.clone()),
|
[(id, _, _)] => Ok(id.clone()),
|
||||||
[] if self.rust.types.contains(ty) => Err("member-not-found"),
|
[] if self.rust.types.contains(ty) => Err("member-not-found"),
|
||||||
[] => Err("receiver-type-external"),
|
[] => Err("receiver-type-external"),
|
||||||
_ => Err("ambiguous"),
|
_ => Err("ambiguous"),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The file declaring the repository type `leaf` as seen from `path`: the file
|
||||||
|
/// itself, then a `use` binding that names it, then the only declaration. `None`
|
||||||
|
/// when several files declare `leaf` and nothing in `path` says which.
|
||||||
|
fn rust_type_file(&self, leaf: &str, path: &str) -> Option<String> {
|
||||||
|
let files = self.rust.type_files.get(leaf)?;
|
||||||
|
if files.iter().any(|file| file == path) {
|
||||||
|
return Some(path.to_owned());
|
||||||
|
}
|
||||||
|
for binding in self
|
||||||
|
.source_bindings
|
||||||
|
.get(path)
|
||||||
|
.into_iter()
|
||||||
|
.flatten()
|
||||||
|
.filter(|binding| binding.local == leaf)
|
||||||
|
{
|
||||||
|
let Some(file) = self
|
||||||
|
.resolve_import(&binding.module, "rust", path)
|
||||||
|
.and_then(|id| self.node(&id))
|
||||||
|
.and_then(|node| node.path.clone())
|
||||||
|
else {
|
||||||
|
continue;
|
||||||
|
};
|
||||||
|
if files.contains(&file) {
|
||||||
|
return Some(file);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
match files.as_slice() {
|
||||||
|
[only] => Some(only.clone()),
|
||||||
|
_ => None,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// `Type::func(..)` yields `Type` when `func` is declared to return `Self`
|
/// `Type::func(..)` yields `Type` when `func` is declared to return `Self`
|
||||||
/// or `Type` (behind `?`, also `Result<..>`/`Option<..>` of either).
|
/// or `Type` (behind `?`, also `Result<..>`/`Option<..>` of either).
|
||||||
fn rust_constructed_type(
|
fn rust_constructed_type(
|
||||||
|
|
@ -1778,13 +1847,14 @@ impl ScanState {
|
||||||
func: &str,
|
func: &str,
|
||||||
fallible: bool,
|
fallible: bool,
|
||||||
call: &RustCall,
|
call: &RustCall,
|
||||||
|
path: &str,
|
||||||
) -> Option<String> {
|
) -> Option<String> {
|
||||||
let ty = if ty == "Self" {
|
let ty = if ty == "Self" {
|
||||||
call.self_type.clone()?
|
call.self_type.clone()?
|
||||||
} else {
|
} else {
|
||||||
ty.to_owned()
|
ty.to_owned()
|
||||||
};
|
};
|
||||||
let id = self.rust_method(&ty, func).ok()?;
|
let id = self.rust_method(&ty, func, path).ok()?;
|
||||||
let returns = self.rust.returns.get(&id)?;
|
let returns = self.rust.returns.get(&id)?;
|
||||||
let returns_type = |value: &str| value == "Self" || value == ty;
|
let returns_type = |value: &str| value == "Self" || value == ty;
|
||||||
let direct = returns_type(returns);
|
let direct = returns_type(returns);
|
||||||
|
|
|
||||||
|
|
@ -153,3 +153,52 @@ fn foreign_imports_generics_and_competing_trait_impls_never_guess() {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn same_named_types_in_different_modules_resolve_through_use_bindings() {
|
||||||
|
// Types were keyed by leaf name, so `b::Config` (method from a trait impl) and
|
||||||
|
// `a::Config` (inherent method) shared one key and the inherent one won: a call on
|
||||||
|
// `b::Config` was attributed to `a::Config::load`.
|
||||||
|
let model = scan_files(&[
|
||||||
|
("src/lib.rs", "pub mod a;\npub mod b;\npub mod user;\n"),
|
||||||
|
(
|
||||||
|
"src/a.rs",
|
||||||
|
"pub struct Config;\nimpl Config {\n pub fn load(&self) -> u32 { 1 }\n}\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"src/b.rs",
|
||||||
|
"pub struct Config;\npub trait Loader { fn load(&self) -> u32; }\nimpl Loader for Config {\n fn load(&self) -> u32 { 2 }\n}\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"src/user.rs",
|
||||||
|
"use crate::b::Config;\nuse crate::b::Loader;\npub fn run(c: &Config) -> u32 { c.load() }\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"src/other.rs",
|
||||||
|
"pub fn guess(c: &Config) -> u32 { c.load() }\n",
|
||||||
|
),
|
||||||
|
]);
|
||||||
|
let targets = |source: &Node| {
|
||||||
|
model
|
||||||
|
.edges
|
||||||
|
.iter()
|
||||||
|
.filter(|e| e.kind == EdgeKind::Calls && e.source == source.id)
|
||||||
|
.map(|e| {
|
||||||
|
let target = model.nodes.iter().find(|n| n.id == e.target).unwrap();
|
||||||
|
(
|
||||||
|
target.path.clone().unwrap_or_default(),
|
||||||
|
e.attributes
|
||||||
|
.get("source.unresolved-reason")
|
||||||
|
.and_then(|v| v.as_str())
|
||||||
|
.unwrap_or("")
|
||||||
|
.to_owned(),
|
||||||
|
)
|
||||||
|
})
|
||||||
|
.collect::<Vec<_>>()
|
||||||
|
};
|
||||||
|
let run = function(&model, "src/user.rs", "run");
|
||||||
|
assert_eq!(targets(run), [("src/b.rs".to_owned(), String::new())]);
|
||||||
|
// Without a binding, two same-named types leave the call ambiguous, not guessed.
|
||||||
|
let guess = function(&model, "src/other.rs", "guess");
|
||||||
|
assert_eq!(targets(guess), [(String::new(), "ambiguous".to_owned())]);
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue