fix/audit-findings #5
3 changed files with 124 additions and 1 deletions
fix(cpp): never resolve calls into #elif/#else branches
Every preprocessor branch was indexed as live. In yams,
InternalEventBus.h guards spdlog with
`#if __has_include(<spdlog/spdlog.h>) ... #else namespace spdlog { stubs }`,
so 1,243 spdlog::warn/info/error call sites resolved to the fallback
stubs, which became the 4th and 5th highest fan-in nodes.
Brick does not evaluate conditions, so only the first branch is treated
as live: declarations and prototypes inside `#elif`/`#else` stay in the
graph (marked `source.preprocessor-branch: alternative`) but are not
definitions, declarations or classes for call resolution. Branches are
tracked textually from the directive lines, because conditions like
`__has_include(<x>)` leave tree-sitter-cpp with an error-recovered tree
that has no `preproc_else` node to find. Include-guard (`#ifndef`) code
is a first branch and still resolves.
Test: cpp_xtu::preprocessor_alternative_branches_are_not_call_targets
fails before this change (spdlog::info resolves into the #elif stub).
commit
2b19142557
|
|
@ -616,6 +616,12 @@ impl ScanState {
|
||||||
if declaration.is_virtual {
|
if declaration.is_virtual {
|
||||||
attributes.insert("source.virtual".into(), serde_json::json!(true));
|
attributes.insert("source.virtual".into(), serde_json::json!(true));
|
||||||
}
|
}
|
||||||
|
if declaration.preprocessor_alternative {
|
||||||
|
attributes.insert(
|
||||||
|
"source.preprocessor-branch".into(),
|
||||||
|
serde_json::json!("alternative"),
|
||||||
|
);
|
||||||
|
}
|
||||||
if document.parse_incomplete && declaration.kind == NodeKind::Function {
|
if document.parse_incomplete && declaration.kind == NodeKind::Function {
|
||||||
attributes.insert("source.parse-incomplete".into(), serde_json::json!(true));
|
attributes.insert("source.parse-incomplete".into(), serde_json::json!(true));
|
||||||
}
|
}
|
||||||
|
|
@ -712,6 +718,9 @@ impl ScanState {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
for (declaration, id) in document.declarations.iter().zip(&ids) {
|
for (declaration, id) in document.declarations.iter().zip(&ids) {
|
||||||
|
if declaration.preprocessor_alternative {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
if declaration.kind == NodeKind::Class
|
if declaration.kind == NodeKind::Class
|
||||||
&& let Some(name) = &declaration.qualified_name
|
&& let Some(name) = &declaration.qualified_name
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -30,6 +30,9 @@ pub(crate) struct Declaration {
|
||||||
/// Rust outer attributes that may register the item with a framework
|
/// Rust outer attributes that may register the item with a framework
|
||||||
/// (`test`, `tokio::main`, `tool`, ...), excluding lint/doc/cfg attributes.
|
/// (`test`, `tokio::main`, `tool`, ...), excluding lint/doc/cfg attributes.
|
||||||
pub attributes: Vec<String>,
|
pub attributes: Vec<String>,
|
||||||
|
/// C/C++: inside an `#elif`/`#else` branch. Which branch is compiled is
|
||||||
|
/// unknown, so such declarations are structure, never resolution targets.
|
||||||
|
pub preprocessor_alternative: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) struct ImportBinding {
|
pub(crate) struct ImportBinding {
|
||||||
|
|
@ -129,6 +132,56 @@ pub(crate) fn supports(language: &str) -> bool {
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Inside an `#elif`/`#else` branch of a conditional. Brick does not evaluate
|
||||||
|
/// preprocessor conditions, so only the first branch is treated as live. The
|
||||||
|
/// directive lines are tracked textually: conditions such as
|
||||||
|
/// `#if __has_include(<x>)` leave tree-sitter with an error-recovered tree
|
||||||
|
/// that has no `preproc_else` node to find.
|
||||||
|
fn in_preprocessor_alternative(node: Node<'_>, alternative_lines: &[bool]) -> bool {
|
||||||
|
alternative_lines
|
||||||
|
.get(node.start_position().row)
|
||||||
|
.copied()
|
||||||
|
.unwrap_or(false)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Per 0-based line: inside a non-first branch of some enclosing conditional.
|
||||||
|
fn preprocessor_alternative_lines(source: &str) -> Vec<bool> {
|
||||||
|
// One entry per open conditional: the index of the branch being read.
|
||||||
|
let mut branches: Vec<usize> = Vec::new();
|
||||||
|
source
|
||||||
|
.lines()
|
||||||
|
.map(|line| {
|
||||||
|
let directive = line.trim_start();
|
||||||
|
let directive = directive
|
||||||
|
.strip_prefix('#')
|
||||||
|
.map(|rest| rest.trim_start())
|
||||||
|
.unwrap_or("");
|
||||||
|
let word = directive
|
||||||
|
.split(|c: char| !c.is_ascii_alphanumeric() && c != '_')
|
||||||
|
.next()
|
||||||
|
.unwrap_or("");
|
||||||
|
match word {
|
||||||
|
"if" | "ifdef" | "ifndef" => {
|
||||||
|
let alternative = branches.iter().any(|&branch| branch > 0);
|
||||||
|
branches.push(0);
|
||||||
|
alternative
|
||||||
|
}
|
||||||
|
"elif" | "elifdef" | "elifndef" | "else" => {
|
||||||
|
if let Some(branch) = branches.last_mut() {
|
||||||
|
*branch += 1;
|
||||||
|
}
|
||||||
|
true
|
||||||
|
}
|
||||||
|
"endif" => {
|
||||||
|
branches.pop();
|
||||||
|
branches.iter().any(|&branch| branch > 0)
|
||||||
|
}
|
||||||
|
_ => branches.iter().any(|&branch| branch > 0),
|
||||||
|
}
|
||||||
|
})
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
pub(crate) fn extract(language: &str, path: &str, source: &str) -> Result<Document> {
|
pub(crate) fn extract(language: &str, path: &str, source: &str) -> Result<Document> {
|
||||||
let (grammar, version) = match language {
|
let (grammar, version) = match language {
|
||||||
"cpp" => (tree_sitter_cpp::LANGUAGE, "cpp:0.23.4"),
|
"cpp" => (tree_sitter_cpp::LANGUAGE, "cpp:0.23.4"),
|
||||||
|
|
@ -144,6 +197,11 @@ pub(crate) fn extract(language: &str, path: &str, source: &str) -> Result<Docume
|
||||||
"dart" => (tree_sitter_dart::LANGUAGE, "dart:0.2.0"),
|
"dart" => (tree_sitter_dart::LANGUAGE, "dart:0.2.0"),
|
||||||
_ => anyhow::bail!("unsupported grammar: {language}"),
|
_ => anyhow::bail!("unsupported grammar: {language}"),
|
||||||
};
|
};
|
||||||
|
let alternative_lines = if language == "cpp" {
|
||||||
|
preprocessor_alternative_lines(source)
|
||||||
|
} else {
|
||||||
|
Vec::new()
|
||||||
|
};
|
||||||
let mut parser = Parser::new();
|
let mut parser = Parser::new();
|
||||||
parser
|
parser
|
||||||
.set_language(&grammar.into())
|
.set_language(&grammar.into())
|
||||||
|
|
@ -304,7 +362,11 @@ pub(crate) fn extract(language: &str, path: &str, source: &str) -> Result<Docume
|
||||||
),
|
),
|
||||||
None => (name.clone(), String::new(), None, false),
|
None => (name.clone(), String::new(), None, false),
|
||||||
};
|
};
|
||||||
if let Some(qualified) = &qualified_name {
|
let preprocessor_alternative =
|
||||||
|
language == "cpp" && in_preprocessor_alternative(node, &alternative_lines);
|
||||||
|
if let Some(qualified) = &qualified_name
|
||||||
|
&& !preprocessor_alternative
|
||||||
|
{
|
||||||
doc.declared.push(qualified.clone());
|
doc.declared.push(qualified.clone());
|
||||||
}
|
}
|
||||||
let (impl_type, impl_trait) = if language == "rust" {
|
let (impl_type, impl_trait) = if language == "rust" {
|
||||||
|
|
@ -323,6 +385,7 @@ pub(crate) fn extract(language: &str, path: &str, source: &str) -> Result<Docume
|
||||||
Vec::new()
|
Vec::new()
|
||||||
};
|
};
|
||||||
doc.declarations.push(Declaration {
|
doc.declarations.push(Declaration {
|
||||||
|
preprocessor_alternative,
|
||||||
is_virtual,
|
is_virtual,
|
||||||
attributes,
|
attributes,
|
||||||
exports: exported_names(node, &name),
|
exports: exported_names(node, &name),
|
||||||
|
|
@ -440,6 +503,7 @@ pub(crate) fn extract(language: &str, path: &str, source: &str) -> Result<Docume
|
||||||
}
|
}
|
||||||
if language == "cpp" {
|
if language == "cpp" {
|
||||||
if !has_error(node)
|
if !has_error(node)
|
||||||
|
&& !in_preprocessor_alternative(node, &alternative_lines)
|
||||||
&& let Some(prototype) = cpp_prototype(node, source)
|
&& let Some(prototype) = cpp_prototype(node, source)
|
||||||
{
|
{
|
||||||
if cpp_is_virtual(node) {
|
if cpp_is_virtual(node) {
|
||||||
|
|
|
||||||
|
|
@ -356,3 +356,53 @@ fn two_using_directives_naming_the_same_function_are_ambiguous() {
|
||||||
assert_eq!(target.path.as_deref(), Some("src/defs.cpp"));
|
assert_eq!(target.path.as_deref(), Some("src/defs.cpp"));
|
||||||
assert_eq!(reason, None);
|
assert_eq!(reason, None);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn preprocessor_alternative_branches_are_not_call_targets() {
|
||||||
|
// An `#else` fallback (yams' spdlog stubs in InternalEventBus.h) was indexed as
|
||||||
|
// live, so every spdlog::warn call site in the repository resolved to the stub and
|
||||||
|
// the stubs became false hubs. Brick cannot know which branch is active, so only
|
||||||
|
// the first branch may be a resolution target.
|
||||||
|
let model = scan_files(&[
|
||||||
|
(
|
||||||
|
"include/log.h",
|
||||||
|
"#if __has_include(<spdlog/spdlog.h>)\n#include <spdlog/spdlog.h>\n#elif defined(ALT)\nnamespace spdlog { inline void info(const char*) {} }\n#else\nnamespace spdlog { inline void warn(const char*) {} }\n#endif\n#ifndef LOG_H\n#define LOG_H\nnamespace app { inline int level() { return 1; } }\n#endif\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"src/main.cpp",
|
||||||
|
"#include \"log.h\"\nint main() { spdlog::warn(\"x\"); spdlog::info(\"y\"); return app::level(); }\n",
|
||||||
|
),
|
||||||
|
]);
|
||||||
|
let found = calls(&model, function(&model, "src/main.cpp", "main"));
|
||||||
|
let mut by_name: Vec<(String, NodeKind, Option<String>)> = found
|
||||||
|
.iter()
|
||||||
|
.map(|(node, reason)| (node.name.clone(), node.kind, reason.clone()))
|
||||||
|
.collect();
|
||||||
|
by_name.sort_by(|a, b| a.0.cmp(&b.0));
|
||||||
|
// First-branch (include-guard) code still resolves; the alternatives do not.
|
||||||
|
assert!(
|
||||||
|
by_name
|
||||||
|
.iter()
|
||||||
|
.any(|(name, kind, _)| name == "level" && *kind == NodeKind::Function)
|
||||||
|
);
|
||||||
|
for (name, kind, _) in &by_name {
|
||||||
|
if name.ends_with("warn") || name.ends_with("info") {
|
||||||
|
assert_eq!(
|
||||||
|
*kind,
|
||||||
|
NodeKind::External,
|
||||||
|
"{name} resolved into an #else/#elif branch"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let stub = model
|
||||||
|
.nodes
|
||||||
|
.iter()
|
||||||
|
.find(|n| n.kind == NodeKind::Function && n.name == "warn")
|
||||||
|
.expect("alternative branches stay visible as structure");
|
||||||
|
assert_eq!(
|
||||||
|
stub.attributes
|
||||||
|
.get("source.preprocessor-branch")
|
||||||
|
.and_then(|v| v.as_str()),
|
||||||
|
Some("alternative")
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue