fix/audit-findings #5

Merged
trevon merged 14 commits from fix/audit-findings into main 2026-09-25 02:39:30 +00:00
2 changed files with 77 additions and 11 deletions
Showing only changes of commit 195d118702 - Show all commits

fix(cpp): treat rival using-directives as ambiguous, not first-come

With `using namespace a; using namespace b;` and both `a::run` and
`b::run` declared, a call to `run` resolved to whichever directive came
first in file order and was labelled `using-directive`. In C++ that
call is ambiguous (or an overload choice Brick does not model).

Directives are now grouped: each `using N::x;` and `using namespace N;`
nominates at most one declaration (its innermost resolving scope), and
the call resolves only when exactly one distinct declaration is
nominated; otherwise it stays external as `ambiguous`.

Test: cpp_xtu::two_using_directives_naming_the_same_function_are_ambiguous
fails before this change; its single-directive case still resolves.
trvon 2026-09-24 19:53:10 -06:00
Signed by: trevon
GPG key ID: 38686EA797D7A8A7

View file

@ -1480,11 +1480,24 @@ impl ScanState {
}
}
if !absolute {
for qualified in self.cpp_using_candidates(name, &call.lookup_scope, &closure) {
if let Some(result) = self.cpp_lookup(&qualified, &closure) {
return result.map(|id| (id, Some("using-directive")));
// Every directive is consulted: two that nominate different
// declarations make the call ambiguous rather than first-come.
let mut nominated: Vec<(String, Result<String, &'static str>)> = Vec::new();
for group in self.cpp_using_candidate_groups(name, &call.lookup_scope, &closure) {
for qualified in group {
if let Some(result) = self.cpp_lookup(&qualified, &closure) {
if !nominated.iter().any(|(seen, _)| seen == &qualified) {
nominated.push((qualified, result));
}
break;
}
}
}
match nominated.as_slice() {
[] => {}
[(_, result)] => return result.clone().map(|id| (id, Some("using-directive"))),
_ => return Err("ambiguous"),
}
}
Err("not-visible")
}
@ -1601,7 +1614,27 @@ impl ScanState {
scope: &str,
closure: &HashSet<String>,
) -> Vec<String> {
let mut candidates = Vec::new();
let mut candidates: Vec<String> = self
.cpp_using_candidate_groups(name, scope, closure)
.into_iter()
.flatten()
.collect();
let mut seen = HashSet::new();
candidates.retain(|candidate| seen.insert(candidate.clone()));
candidates
}
/// `cpp_using_candidates` grouped by directive: one group per `using N::x;`
/// and per `using namespace N;`, whose members are the scope levels that
/// directive may name (innermost first). Separate groups are rival
/// nominations, so more than one resolving group is ambiguous.
fn cpp_using_candidate_groups(
&self,
name: &str,
scope: &str,
closure: &HashSet<String>,
) -> Vec<Vec<String>> {
let mut groups = Vec::new();
let (head, rest) = match name.split_once("::") {
Some((head, rest)) => (head, Some(rest)),
None => (name, None),
@ -1611,21 +1644,24 @@ impl ScanState {
for file in files {
for (leaf, target) in self.cpp.using_names.get(file).into_iter().flatten() {
if leaf == head {
candidates.push(match rest {
groups.push(vec![match rest {
Some(rest) => qualify(target, rest),
None => target.clone(),
});
}]);
}
}
for namespace in self.cpp.using_namespaces.get(file).into_iter().flatten() {
for level in scope_levels(scope) {
candidates.push(qualify(&qualify(&level, namespace), name));
}
groups.push(
scope_levels(scope)
.into_iter()
.map(|level| qualify(&qualify(&level, namespace), name))
.collect(),
);
}
}
let mut seen = HashSet::new();
candidates.retain(|candidate| seen.insert(candidate.clone()));
candidates
groups.retain(|group| seen.insert(group.clone()));
groups
}
/// Method lookup in a class, then breadth-first through in-repository bases.

View file

@ -326,3 +326,33 @@ fn brace_default_arguments_do_not_hide_prototypes_or_report_errors() {
assert_eq!(reason, None);
assert!(model.diagnostics.is_empty(), "{:?}", model.diagnostics);
}
#[test]
fn two_using_directives_naming_the_same_function_are_ambiguous() {
// With `using namespace a; using namespace b;` and both `a::run` and `b::run`
// defined, the call resolved to whichever directive came first. C++ makes that
// ambiguous (or an overload choice Brick cannot make), so it must stay external.
let model = scan_files(&[
("src/a.h", "namespace a { int run(int x); }\n"),
("src/b.h", "namespace b { int run(int x); }\n"),
(
"src/defs.cpp",
"#include \"a.h\"\n#include \"b.h\"\nnamespace a { int run(int x) { return x; } }\nnamespace b { int run(int x) { return -x; } }\n",
),
(
"src/main.cpp",
"#include \"a.h\"\n#include \"b.h\"\nusing namespace a;\nusing namespace b;\nint main() { return run(1); }\n",
),
// Clean negative: one directive still resolves with its label.
(
"src/only_a.cpp",
"#include \"a.h\"\nusing namespace a;\nint only() { return run(1); }\n",
),
]);
let (target, reason) = single_call(&model, function(&model, "src/main.cpp", "main"));
assert_eq!(target.kind, NodeKind::External);
assert_eq!(reason.as_deref(), Some("ambiguous"));
let (target, reason) = single_call(&model, function(&model, "src/only_a.cpp", "only"));
assert_eq!(target.path.as_deref(), Some("src/defs.cpp"));
assert_eq!(reason, None);
}