From 956a032bbe7605f388ad5909f239a70724066c7c Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Fri, 11 Sep 2026 15:29:18 -0700 Subject: [PATCH 1/6] query: answer about one definition of a name, and say which one A name in C is not one thing. Linux defines `pr_warn` nine times. A callee query reports each definition separately; `callers`, `func` and `callchain` have to start somewhere, and each picked a definition without saying so. Four things went wrong with that. `func` asked what the name calls once per definition, by name, so every definition was shown the same preferred answer: $ semcode -q "func pr_warn" # before Note: Found 9 function definitions with name 'pr_warn' ==> Function 1 of 9: File: include/linux/printk.h Calls: 4 -> fprintf -> va_end -> va_start -> vfprintf Those are the callees of arch/x86/tools/insn_decoder_test.c:48, a host build tool, and all nine blocks carried them. include/linux/printk.h:563 calls pr_fmt and printk. Each definition now answers with the row read at its own file and line: $ semcode -q "func pr_warn" # after File: include/linux/printk.h Calls: 2 -> pr_fmt -> printk File: arch/x86/tools/insn_decoder_test.c Calls: 4 -> fprintf ... A call chain named one definition and walked another. The header line and the callee list were ranked in two places, one preferring the program being audited and the other a long `.c` body, so `callchain pr_warn` reported include/linux/printk.h:563 and then expanded the host tool's fprintf. One chooser now decides, and the callee lookup finds the row by where it was read. The choice preferred another program in the same tree. A source tree can build more than one program: Linux builds host tools from every directory named `tools` -- the top-level one and twelve more under `arch/` and `drivers/` -- example code from `samples` and prose from `Documentation`. Eight of the nine definitions of `pr_warn` are outside the kernel image, and ranking `.c` above `.h` put one of them first. A path component decides this, not a prefix: the definition that made it necessary is arch/x86/tools/insn_decoder_test.c, which no prefix of `tools/` matches. `scripts` and `usr` read as though they belong on that list and are deliberately absent, because both hold code that reaches the built image -- scripts/module-common.c is compiled into every `.ko`. Some rows are neither a definition nor a declaration. arch/x86/xen/suspend_hvm.c:22 is `BUG_ON(xen_set_upcall_vector(cpu));`, a call stored under the name it calls, and being a `.c` file in the tree being audited it outranked every real definition of BUG_ON. The row's own text is asked first now, so a use of a name is not an answer about it. Where a name has several definitions the answer says so, names the one it is about and lists the rest, because which one a call site reaches depends on the file it is written in and on the configuration the tree is built with, and neither is recorded here: $ semcode -q "callers pr_warn" Ambiguous: 'pr_warn' is defined 9 times in this revision. This answer is about include/linux/printk.h:563; the others are arch/x86/tools/insn_decoder_test.c:48, rust/kernel/device.rs:346, ... That note sends the reader to `func`, so the two had to agree on how many definitions a name has, and they did not: three predicates answered "is this row a definition" three ways. A 50-byte body threshold dropped six definitions of `kfree` to five, and requiring braces in a header dropped every macro defined in one, listing `container_of` 11 times where the note counted 12. All three now ask the row's own text, the test a callee query already used. Over 41 names sampled across the tree, the three commands agreed on 33 and disagreed on 6 before, and agree on 39 with 2 defined once after. A definition in another language is ranked last where a strict majority of the name's definitions share one language, which is what makes the C macro win over `Device::pr_warn` in rust/kernel/device.rs. That method is stored under its bare name; until it is stored qualified, a name shared across two languages is not one ambiguous name and this stands in for knowing that. $ regress.sh # against the base commit functions 1054808 -> 1054808 +0 call_edges 3197541 -> 3197541 +0 VERDICT: ok -- kernel: idempotent PASS Nothing in the index changes: this is what a query does with what is already stored, so no statistic moves on either tree. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/bin/query_impl/commands.rs | 14 +- src/callchain.rs | 30 ++- src/database/connection.rs | 226 ++++++++++++++--- src/lib.rs | 7 +- src/search.rs | 55 +++-- src/types.rs | 115 +++++++++ tests/ambiguous_single_answer.rs | 406 +++++++++++++++++++++++++++++++ 7 files changed, 782 insertions(+), 71 deletions(-) create mode 100644 tests/ambiguous_single_answer.rs diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index ce1d639..98a86cc 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -201,10 +201,12 @@ async fn show_callchain_with_limits( ); // First, check if function exists using git-aware query - let func_opt = db.find_function_git_aware(function_name, git_sha).await?; + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - let func = match func_opt { - Some(f) => f, + let chosen = match chosen_opt { + Some(chosen) => chosen, None => { println!( "{} Function '{}' not found in database at git SHA {}", @@ -215,6 +217,12 @@ async fn show_callchain_with_limits( return Ok(()); } }; + // One chain, so one definition. Say so rather than presenting the choice + // as the tree's only answer. + if let Some(note) = chosen.ambiguity_note() { + println!("{} {}", "Ambiguous:".bold().yellow(), note); + } + let func = chosen.function; println!("{}", "=== Function Information ===".bold().green()); println!( diff --git a/src/callchain.rs b/src/callchain.rs index 182d8b9..cd2d0b9 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -277,10 +277,17 @@ pub async fn show_callers_to_writer( writeln!(writer, "{search_msg}")?; // Search for function - macros are now stored as functions - let func_opt = db.find_function_git_aware(name, git_sha).await?; - - match func_opt { - Some(func) => { + let chosen_opt = db.find_function_git_aware_reporting(name, git_sha).await?; + + match chosen_opt { + Some(chosen) => { + // Callers are found by name, and a name can belong to several + // functions. Listing them under one definition's heading says the + // callers of the others belong to it. + if let Some(note) = chosen.ambiguity_note() { + writeln!(writer, "{} {}", "Ambiguous:".bold().yellow(), note)?; + } + let func = chosen.function; // Always use git-aware callers query let callers = db.get_function_callers_git_aware(name, git_sha).await?; let indirect = db.find_indirect_callers(name, git_sha).await?; @@ -1102,10 +1109,17 @@ pub async fn show_callchain_to_writer( writeln!(writer, "{search_msg}")?; // Use provided git SHA - let func_opt = db.find_function_git_aware(name, git_sha).await?; - - match func_opt { - Some(func) => { + let chosen_opt = db.find_function_git_aware_reporting(name, git_sha).await?; + + match chosen_opt { + Some(chosen) => { + // A chain is read as one path, so it starts at one definition. It + // said which file that was and not that there had been a choice, + // which reads as the tree having one. + if let Some(note) = chosen.ambiguity_note() { + writeln!(writer, "{} {}", "Ambiguous:".bold().yellow(), note)?; + } + let func = chosen.function; let header = format!("{}", "=== Function Call Chain ===".bold().green()); writeln!(writer, "{header}")?; diff --git a/src/database/connection.rs b/src/database/connection.rs index ee7da02..f212a23 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -23,7 +23,7 @@ use crate::database::content::{ContentInfo, ContentStore}; use crate::database::processed_files::{ProcessedFileRecord, ProcessedFileStore}; use crate::database::vectors::VectorStore; use crate::treesitter_analyzer::TreeSitterAnalyzer; -use crate::types::{FunctionInfo, TypeInfo, TypedefInfo}; +use crate::types::{ChosenDefinition, FunctionInfo, TypeInfo, TypedefInfo}; use crate::vectorizer::CodeVectorizer; use crate::workdir::WorkdirIndex; use crate::worktree::{WorkingCopy, WorkingCopyHashes}; @@ -2266,12 +2266,49 @@ impl DatabaseManager { self.find_function_with_manifest(name, &git_manifest).await } + /// Find a function by name, and say which other definitions were set aside. + /// + /// For a command that must answer about one definition. `callers` and + /// `callchain` start from a single function and cannot report every + /// definition the way a callee query does, so they report the choice. + pub async fn find_function_git_aware_reporting( + &self, + name: &str, + git_sha: &str, + ) -> Result> { + let git_manifest = self.git_manifest_cached(git_sha).await?; + if git_manifest.is_empty() { + let why = self.why_nothing_resolved(git_sha); + return Ok(self + .function_not_at_revision(name, git_sha, why) + .await? + .map(|function| ChosenDefinition { + function, + others: Vec::new(), + })); + } + self.find_function_with_manifest_reporting(name, &git_manifest) + .await + } + /// Find a function by name using a pre-generated git manifest (fast - no manifest regeneration) pub async fn find_function_with_manifest( &self, name: &str, git_manifest: &crate::database::resolution::RevisionPaths, ) -> Result> { + Ok(self + .find_function_with_manifest_reporting(name, git_manifest) + .await? + .map(|chosen| chosen.function)) + } + + /// The body of `find_function_with_manifest`, keeping the alternatives. + pub async fn find_function_with_manifest_reporting( + &self, + name: &str, + git_manifest: &crate::database::resolution::RevisionPaths, + ) -> Result> { let revision = match git_manifest.revision() { "" => "the revision asked about", sha => sha, @@ -2286,9 +2323,10 @@ impl DatabaseManager { // No row has ever named this function: the on-miss path is the // only one that can still find it, in an edit the index has // never seen. - return self + return Ok(self .function_not_at_revision(name, revision, Absent::PathsNotInTree) - .await; + .await? + .map(ChosenDefinition::only)); } // Step 2: Use manifest to get hashes for candidate files (fast HashMap lookups) @@ -2300,9 +2338,10 @@ impl DatabaseManager { } if resolved_hashes.is_empty() { - return self + return Ok(self .function_not_at_revision(name, revision, Absent::PathsNotInTree) - .await; + .await? + .map(ChosenDefinition::only)); } // Step 3: stat (and, on a mismatch, hash) each candidate against the @@ -2328,13 +2367,13 @@ impl DatabaseManager { // Step 4: Pick the best result (prefer implementation over declaration) if matches.is_empty() { - return self + return Ok(self .function_not_at_revision(name, revision, Absent::ContentNotIndexed) - .await; + .await? + .map(ChosenDefinition::only)); } - let best_match = self.select_best_function_match(matches); - Ok(Some(best_match)) + Ok(Some(self.choose_definition(matches))) } /// Get just the types field for a function using pre-generated manifest (very fast - no body fetching) @@ -2505,7 +2544,16 @@ impl DatabaseManager { .await; } - let implementations = self.filter_implementations_only(matches); + let mut implementations = self.filter_implementations_only(matches); + // A stable order, so two runs and two readers see the same list. The + // candidate files are resolved through a hash map, so without this the + // nine definitions of `pr_warn` came back in a different order each + // run, which reads as the tree having changed. + implementations.sort_by(|a, b| { + a.file_path + .cmp(&b.file_path) + .then(a.line_start.cmp(&b.line_start)) + }); tracing::info!( "Git-aware lookup succeeded: found {} implementations of '{}' at commit '{}'", implementations.len(), @@ -2516,6 +2564,14 @@ impl DatabaseManager { } /// Filter out declarations, keeping only function implementations + /// + /// The row's own text decides, the same test a callee query uses. A body + /// length threshold decided it before -- "more than 50 bytes" -- and + /// dropped short definitions that are not declarations at all: of six + /// definitions of `kfree` in Linux, a callee query reported six and this + /// listed five. Two commands in one answer then disagreed about how many + /// definitions a name has, which is the thing the reader was being asked + /// to go and check. fn filter_implementations_only(&self, functions: Vec) -> Vec { functions .into_iter() @@ -2524,34 +2580,107 @@ impl DatabaseManager { if func.return_type.is_empty() { return true; } - - // Filter criteria: exclude likely declarations - let span = func.line_end.saturating_sub(func.line_start); - let has_substantial_body = func.body.len() > 50; // More than just a declaration - let is_likely_declaration = span <= 1 && func.body.trim().ends_with(';'); - - // Keep functions that have substantial bodies and are not declarations - has_substantial_body && !is_likely_declaration + !func.body.is_empty() && !crate::types::text_is_prototype(&func.body) }) .collect() } /// Select the best function match, prioritizing definitions over declarations - fn select_best_function_match(&self, mut matches: Vec) -> FunctionInfo { + fn select_best_function_match(&self, matches: Vec) -> FunctionInfo { + self.choose_definition(matches).function + } + + /// The definition to answer about, and every other definition of the name. + /// + /// The choice is a heuristic and stays one -- which definition a call site + /// reaches depends on the file it is written in and on the configuration + /// the tree is built with, and the index knows neither. What changes is + /// that the alternatives come back with it, so a caller can say a choice + /// was made instead of presenting it as the answer. + /// + /// A prototype is not an alternative: nearly every exported function has + /// one, and counting it would call almost every name ambiguous. The row's + /// own text decides, the same test a callee query uses. + fn choose_definition(&self, mut matches: Vec) -> ChosenDefinition { if matches.len() == 1 { - return matches.into_iter().next().unwrap(); - } + return ChosenDefinition::only(matches.into_iter().next().unwrap()); + } + + // The language most definitions of this name are written in. Two + // languages that give one name to two things are not one ambiguous + // name, and `Device::pr_warn` in rust/kernel/device.rs is stored under + // the bare name, so it competes with the C macro that 4,065 functions + // call. Ranking the minority language last answers the question the + // tree is mostly written in; where a name is defined in one language + // this decides nothing. + let mut by_language: HashMap<&str, usize> = HashMap::new(); + for candidate in &matches { + *by_language + .entry(crate::types::path_language(&candidate.file_path)) + .or_default() += 1; + } + // A strict majority or nothing: more than half the definitions, not + // merely more than any other language. Two definitions in two + // languages have no majority, and picking the alphabetically later one + // would decide a C tree's answer by the spelling of an extension. The + // rung is a stand-in for an extraction defect -- a Rust method stored + // under its bare name -- so it fires where the tree is nearly + // unanimous and stays out of the way otherwise. + let highest = by_language.values().copied().max().unwrap_or_default(); + let unique_top = by_language + .values() + .filter(|count| **count == highest) + .count() + == 1; + let majority_language = match unique_top && highest * 2 > matches.len() { + true => by_language + .iter() + .find(|(_, count)| **count == highest) + .map(|(language, _)| language.to_string()) + .unwrap_or_default(), + false => String::new(), + }; // Prioritize by multiple criteria matches.sort_by(|a, b| { - // 1. Prefer .c files over .h files + // 1. Prefer a row that defines the name. Some rows are neither a + // definition nor a declaration: arch/x86/xen/suspend_hvm.c:22 + // is `BUG_ON(xen_set_upcall_vector(cpu));`, a call site stored + // under the name it calls. It is a .c file in the tree being + // audited, so every rung below this one ranked it first, and a + // question about BUG_ON was answered from a use of it. + let a_defines = !crate::types::text_is_prototype(&a.body); + let b_defines = !crate::types::text_is_prototype(&b.body); + if a_defines != b_defines { + return b_defines.cmp(&a_defines); + } + + // 2. Prefer the program being audited over another program that + // shares the tree. Without this, `pr_warn` answers from + // arch/x86/tools/insn_decoder_test.c -- a .c file, so rung 2 + // ranks it above include/linux/printk.h, and a kernel question + // gets a host tool's answer. + let a_other = crate::types::path_is_other_program(&a.file_path); + let b_other = crate::types::path_is_other_program(&b.file_path); + if a_other != b_other { + return a_other.cmp(&b_other); + } + + // 3. Prefer the language most definitions of the name are in. + let a_majority = crate::types::path_language(&a.file_path) == majority_language; + let b_majority = crate::types::path_language(&b.file_path) == majority_language; + if a_majority != b_majority { + return b_majority.cmp(&a_majority); + } + + // 4. Prefer .c files over .h files let a_is_source = a.file_path.ends_with(".c"); let b_is_source = b.file_path.ends_with(".c"); if a_is_source != b_is_source { return b_is_source.cmp(&a_is_source); } - // 2. Prefer functions with bodies (implementations) + // 5. Prefer functions with bodies (implementations) let a_span = a.line_end.saturating_sub(a.line_start); let b_span = b.line_end.saturating_sub(b.line_start); let a_has_body = a_span > 0 && a.body.len() > 50; @@ -2560,15 +2689,25 @@ impl DatabaseManager { return b_has_body.cmp(&a_has_body); } - // 3. Prefer functions with parameters + // 6. Prefer functions with parameters let a_has_params = !a.parameters.is_empty(); let b_has_params = !b.parameters.is_empty(); if a_has_params != b_has_params { return b_has_params.cmp(&a_has_params); } - // 4. Prefer longer bodies (more implementation detail) - b.body.len().cmp(&a.body.len()) + // 7. Prefer longer bodies (more implementation detail) + let by_body = b.body.len().cmp(&a.body.len()); + if by_body != std::cmp::Ordering::Equal { + return by_body; + } + + // 8. A tie decided by nothing is still decided the same way twice: + // the file order a lookup returns is not stable, and two runs + // answering differently reads as the tree having changed. + a.file_path + .cmp(&b.file_path) + .then(a.line_start.cmp(&b.line_start)) }); tracing::debug!( @@ -2578,7 +2717,16 @@ impl DatabaseManager { matches[0].file_path ); - matches.into_iter().next().unwrap() + let mut matches = matches.into_iter(); + let function = matches.next().unwrap(); + let others = matches + .filter(|candidate| !crate::types::text_is_prototype(&candidate.body)) + .map(|candidate| crate::types::DefinitionSite { + file_path: candidate.file_path, + line_start: candidate.line_start, + }) + .collect(); + ChosenDefinition { function, others } } /// Find all functions by name without git awareness (non-git-aware) @@ -5545,13 +5693,20 @@ impl DatabaseManager { Ok(definitions) } - /// The callees of the definition this revision most likely means: an - /// implementation over a declaration, and the longest body among equals. + /// The callees of the definition this revision most likely means. /// /// A single answer is what a call chain needs -- walking every definition of /// every name multiplies a chain by the ambiguity at each step. Where the /// choice is shown to a reader rather than walked, /// `get_function_callees_by_definition` reports all of them instead. + /// + /// Which definition that is comes from `choose_definition`, the same place + /// the header line above a chain comes from, and the row is then looked up + /// by where it was read. Ranking here as well is what made a chain + /// contradict itself: this function preferred a long `.c` body and the + /// header preferred the program being audited, so `callchain pr_warn` named + /// include/linux/printk.h:563 and then listed the callees of + /// arch/x86/tools/insn_decoder_test.c:48. pub async fn get_function_callees_with_manifest( &self, function_name: &str, @@ -5560,12 +5715,17 @@ impl DatabaseManager { let definitions = self .get_function_callees_by_definition(function_name, git_manifest) .await?; + let chosen = self + .find_function_with_manifest_reporting(function_name, git_manifest) + .await?; + let Some(chosen) = chosen else { + return Ok(Vec::new()); + }; Ok(definitions .into_iter() - .max_by_key(|definition| { - let line_count = definition.line_end.saturating_sub(definition.line_start); - let is_header = definition.file_path.ends_with(".h"); - (if is_header { 0 } else { 1 }, line_count) + .find(|definition| { + definition.file_path == chosen.function.file_path + && definition.line_start == chosen.function.line_start }) .map(|definition| definition.callees) .unwrap_or_default()) diff --git a/src/lib.rs b/src/lib.rs index ec42b4d..f6b2229 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -39,9 +39,10 @@ pub use text_utils::preprocess_code; pub use treesitter_analyzer::{ParameterFate, TreeSitterAnalyzer}; pub use types::Handover; pub use types::{ - ArgumentFunction, DispatchKind, DispatchSite, FieldInfo, FunctionInfo, GitCommitInfo, - GitFileEntry, GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, ParameterInfo, - Registration, RegistrationKind, TypeInfo, TypedefInfo, + path_is_other_program, path_language, ArgumentFunction, ChosenDefinition, DefinitionSite, + DispatchKind, DispatchSite, FieldInfo, FunctionInfo, GitCommitInfo, GitFileEntry, + GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, ParameterInfo, Registration, + RegistrationKind, TypeInfo, TypedefInfo, }; pub use vectorizer::CodeVectorizer; pub use workdir::WorkdirIndex; diff --git a/src/search.rs b/src/search.rs index cd231e6..62eb015 100644 --- a/src/search.rs +++ b/src/search.rs @@ -1226,6 +1226,13 @@ pub async fn query_function_or_macro_to_writer_verbose( } /// Check if a function is actually a definition (has implementation) vs just a declaration +/// +/// The row's own text decides, the same test a callee query and the definition +/// chooser use. Requiring braces in a header decided it before, which dropped +/// every macro defined in one: `container_of` was listed 11 times where a +/// callee query and the ambiguity note both counted 12. Three commands then +/// disagreed about how many definitions a name has, and the note sends the +/// reader here to see them. pub fn is_function_definition(func: &crate::FunctionInfo) -> bool { if func.body.is_empty() { return false; // Empty body is definitely a declaration @@ -1236,26 +1243,7 @@ pub fn is_function_definition(func: &crate::FunctionInfo) -> bool { return true; } - let body = func.body.trim(); - - // If body ends with just a semicolon, it's a declaration - if body.ends_with(';') && !body.contains('{') { - return false; - } - - // If it contains braces, it's likely a definition - if body.contains('{') && body.contains('}') { - return true; - } - - // Header files typically contain declarations - if func.file_path.ends_with(".h") || func.file_path.ends_with(".hpp") { - // In header files, be more strict - require braces for definitions - return body.contains('{') && body.contains('}'); - } - - // For .c/.cpp files, if it's not just a semicolon-terminated line, assume it's a definition - !body.ends_with(';') + !crate::types::text_is_prototype(&func.body) } async fn query_function_or_macro_to_writer_with_options( @@ -1294,6 +1282,22 @@ async fn query_function_or_macro_to_writer_with_options( )?; } + // What each definition calls, keyed by where it was read. Asking + // by name once per definition returns whichever one a heuristic + // prefers, every time, so nine definitions of `pr_warn` were each + // shown the callees of arch/x86/tools/insn_decoder_test.c. + let per_definition: std::collections::HashMap<(String, u32), Vec> = db + .get_function_callees_by_definition_git_aware(name, git_sha) + .await? + .into_iter() + .map(|definition| { + ( + (definition.file_path, definition.line_start), + definition.callees, + ) + }) + .collect(); + // Display each function definition with its outgoing calls for (i, func) in definitions.iter().enumerate() { if definitions.len() > 1 { @@ -1306,10 +1310,13 @@ async fn query_function_or_macro_to_writer_with_options( )?; } display_function_to_writer_with_options(func, writer, true)?; - // Get and display calls (outgoing) for each function definition - let calls = db - .get_function_callees_git_aware(&func.name, git_sha) - .await?; + // The callees of this definition, not of the name. A row the + // callee query did not return answers with nothing rather than + // with another definition's calls. + let calls = per_definition + .get(&(func.file_path.clone(), func.line_start)) + .cloned() + .unwrap_or_default(); display_call_relationships_with_options( &func.name, &calls, diff --git a/src/types.rs b/src/types.rs index 8cb35c6..fb7b75e 100644 --- a/src/types.rs +++ b/src/types.rs @@ -69,6 +69,121 @@ pub struct CalleeDefinition { pub is_definition: bool, } +/// Where one definition of a name was read. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct DefinitionSite { + pub file_path: String, + pub line_start: u32, +} + +/// The definition a command that must give one answer is about, beside the +/// definitions of the same name it set aside. +/// +/// A command that walks a chain or lists callers needs one starting point, so +/// it cannot report every definition the way a callee query does. Carrying the +/// others with the choice lets it name them, which is the difference between +/// picking one and hiding that there was a choice. +#[derive(Debug, Clone)] +pub struct ChosenDefinition { + pub function: FunctionInfo, + pub others: Vec, +} + +impl ChosenDefinition { + /// The only definition of the name, so there was no choice to report. + pub fn only(function: FunctionInfo) -> Self { + ChosenDefinition { + function, + others: Vec::new(), + } + } + + /// What to tell a reader before an answer about one of several definitions. + /// + /// `None` where the name has one definition: a note on every answer would + /// be noise, and noise is skipped rather than read. + pub fn ambiguity_note(&self) -> Option { + if self.others.is_empty() { + return None; + } + let mut sites: Vec = self + .others + .iter() + .map(|site| format!("{}:{}", site.file_path, site.line_start)) + .collect(); + sites.sort(); + // A name with a hundred definitions would otherwise print a hundred + // paths, and a note too long to read is not read. The count is the + // part that cannot be cut: it says an answer was chosen. + const SHOWN: usize = 8; + let listed = if sites.len() > SHOWN { + format!( + "{}, and {} more ('func {}' lists them all)", + sites[..SHOWN].join(", "), + sites.len() - SHOWN, + self.function.name + ) + } else { + sites.join(", ") + }; + Some(format!( + "'{}' is defined {} times in this revision. This answer is about \ + {}:{}; the others are {}. Which one a call site reaches depends on \ + the file it is written in and on the configuration the tree is \ + built with, and neither is recorded here.", + self.function.name, + self.others.len() + 1, + self.function.file_path, + self.function.line_start, + listed + )) + } +} + +/// Whether a path holds a program other than the one an audit is about. +/// +/// A source tree can build more than one program. Linux builds host tools from +/// every directory named `tools` -- the top-level one and `arch/x86/tools`, +/// `arch/arm64/tools`, `drivers/comedi/drivers/ni_routing/tools` and nine more +/// -- example code from `samples`, and prose from `Documentation`. Those +/// programs define names the kernel also defines: of nine definitions of +/// `pr_warn`, eight are outside the kernel image. +/// +/// A path component, not a prefix: the definition that made this necessary is +/// `arch/x86/tools/insn_decoder_test.c`, which no prefix of `tools/` matches. +/// Every directory named `tools` in that tree holds a host program, so the +/// component is the signal. +/// +/// This orders a choice between definitions. It never drops one: a name defined +/// only under `tools` still answers, and the choice is reported either way, so +/// a tie-break that goes the wrong way is visible rather than silent. +/// +/// `scripts` and `usr` are deliberately absent, though they read as though they +/// belong: both hold code that ends up in the built image. `scripts/module-common.c` +/// is compiled into every `.ko` (`scripts/Makefile.modfinal:28`) and +/// `usr/initramfs_data.S` is linked in, so the directory name does not imply +/// another program there the way it does for `tools`. +pub fn path_is_other_program(file_path: &str) -> bool { + const OTHER_PROGRAMS: [&str; 3] = ["tools", "samples", "Documentation"]; + file_path + .split('/') + .any(|component| OTHER_PROGRAMS.contains(&component)) +} + +/// The language a path's extension names, for grouping definitions of one name. +/// +/// Coarse on purpose: the question is only whether two definitions of a name +/// are written in the same language, not which dialect. An unrecognised +/// extension is its own group rather than being folded into one of these, so a +/// file type nobody has thought about does not silently join C. +pub fn path_language(file_path: &str) -> &str { + match file_path.rsplit_once('.') { + Some((_, "c" | "h" | "cc" | "cpp" | "cxx" | "hh" | "hpp" | "inc")) => "c", + Some((_, extension)) => extension, + None => "", + } +} + /// Whether stored text declares a function without defining it. /// /// The row's own text is the only thing that separates the two: a prototype diff --git a/tests/ambiguous_single_answer.rs b/tests/ambiguous_single_answer.rs new file mode 100644 index 0000000..b26baa6 --- /dev/null +++ b/tests/ambiguous_single_answer.rs @@ -0,0 +1,406 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +// +// What a command that must give ONE answer says when the tree defines the name +// more than once. A callee query reports every definition; `callers`, `func` +// and `callchain` start from one, so what they owe the reader is the choice. +use semcode::{git, DatabaseManager}; +use std::path::Path; +use std::process::Command; +use std::sync::Arc; + +fn git_run(repo: &Path, args: &[&str]) { + let status = Command::new("git") + .args(args) + .current_dir(repo) + .env("GIT_AUTHOR_NAME", "Semcode Test") + .env("GIT_AUTHOR_EMAIL", "semcode@example.com") + .env("GIT_COMMITTER_NAME", "Semcode Test") + .env("GIT_COMMITTER_EMAIL", "semcode@example.com") + .status() + .unwrap(); + assert!(status.success(), "git {args:?} failed"); +} + +/// The shape of `pr_warn` in Linux, reduced: the definition the tree's own code +/// calls sits in a header, a host tool under `tools/` defines the same name in +/// a `.c` file, and a second language defines a method that is stored under the +/// bare name. +async fn tree_shaped_like_pr_warn() -> (tempfile::TempDir, Arc, String) { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("arch/x86/tools")).unwrap(); + std::fs::create_dir_all(repo.join("include/linux")).unwrap(); + std::fs::create_dir_all(repo.join("rust/kernel")).unwrap(); + + // What the tree's own code calls. + std::fs::write( + repo.join("include/linux/printk.h"), + "static inline int report(int level)\n{\n\treturn emit(level);\n}\n", + ) + .unwrap(); + // A host program that happens to share the tree. Note the path: `tools` is + // a component, not a prefix, so a prefix test does not see it. + std::fs::write( + repo.join("arch/x86/tools/decoder_test.c"), + "int report(int level)\n{\n\treturn fprintf(stderr, \"%d\", level);\n}\n", + ) + .unwrap(); + // A method in another language, stored under its bare name. + std::fs::write( + repo.join("rust/kernel/device.rs"), + "impl Device {\n pub fn report(&self, level: i32) {\n self.printk(level);\n }\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("driver.c"), + "#include \n\nint probe(void)\n{\n\treturn report(3);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run( + repo, + &["commit", "-q", "-m", "three definitions of one name"], + ); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string(), "rs".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + (dir, db, sha) +} + +#[tokio::test] +async fn a_single_answer_names_the_definitions_it_set_aside() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap_or_else(|| panic!("report should resolve")); + + // Every other definition is named, so the reader can ask again about one. + assert_eq!(chosen.others.len(), 2, "{:?}", chosen.others); + let note = chosen + .ambiguity_note() + .unwrap_or_else(|| panic!("three definitions, so there is a choice to report")); + assert!(note.contains("defined 3 times"), "{note}"); + assert!(note.contains("arch/x86/tools/decoder_test.c"), "{note}"); + assert!(note.contains("rust/kernel/device.rs"), "{note}"); +} + +#[tokio::test] +async fn the_answer_is_not_another_program_in_the_same_tree() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + + // Ranked by the older ladder this is decoder_test.c: a `.c` file beats a + // header. It is a host tool, and nothing in the tree it is asked about + // reaches it. + assert_eq!( + chosen.function.file_path, "include/linux/printk.h", + "chose {}:{}", + chosen.function.file_path, chosen.function.line_start + ); +} + +#[tokio::test] +async fn the_chain_lists_the_callees_of_the_definition_it_names() { + // The header and the callee list are two answers about one function, and + // they were ranked in two places. The header preferred the program being + // audited and the callee list preferred a long `.c` body, so a chain named + // one definition and then walked another. + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let named = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + let walked = db + .get_function_callees_git_aware("report", &sha) + .await + .unwrap(); + + assert_eq!(named.function.file_path, "include/linux/printk.h"); + assert!(walked.contains(&"emit".to_string()), "{walked:?}"); + assert!( + !walked.contains(&"fprintf".to_string()), + "walked the host tool's body while naming {}: {walked:?}", + named.function.file_path + ); +} + +#[tokio::test] +async fn a_name_defined_once_reports_no_choice() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("probe", &sha) + .await + .unwrap() + .unwrap(); + assert!(chosen.others.is_empty(), "{:?}", chosen.others); + // A note on every answer is noise, and noise is skipped rather than read. + assert!(chosen.ambiguity_note().is_none()); +} + +#[tokio::test] +async fn every_definition_is_listed_in_the_same_order_twice() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let first = db + .find_all_functions_git_aware("report", &sha) + .await + .unwrap(); + let again = db + .find_all_functions_git_aware("report", &sha) + .await + .unwrap(); + let paths = |list: &[semcode::FunctionInfo]| -> Vec { + list.iter().map(|f| f.file_path.clone()).collect() + }; + // The candidate files are resolved through a hash map, so two runs used to + // answer in different orders, which reads as the tree having changed. + assert_eq!(paths(&first), paths(&again), "{:?}", paths(&first)); + assert!(paths(&first).windows(2).all(|pair| pair[0] <= pair[1])); +} + +#[tokio::test] +async fn each_definition_answers_with_its_own_callees() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + // This is what the `func` command joins on: one row per definition, keyed + // by where it was read. Asking by name once per definition returns the + // same preferred answer every time, which attributed a host tool's callees + // to the header the kernel calls. + let definitions = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap(); + let callees_at = |needle: &str| -> Vec { + definitions + .iter() + .find(|d| d.file_path.ends_with(needle)) + .unwrap_or_else(|| panic!("no row for {needle}: {definitions:?}")) + .callees + .clone() + }; + assert!(callees_at("include/linux/printk.h").contains(&"emit".to_string())); + assert!(callees_at("arch/x86/tools/decoder_test.c").contains(&"fprintf".to_string())); + assert!(!callees_at("include/linux/printk.h").contains(&"fprintf".to_string())); +} + +#[tokio::test] +async fn the_listing_shows_each_definition_its_own_calls() { + // The defect this pins is in what `func` prints, not in what the database + // returns: it looped over the definitions and asked by name inside the + // loop, so all three blocks carried one definition's calls. On Linux that + // printed the callees of arch/x86/tools/insn_decoder_test.c under + // include/linux/printk.h, nine times. + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let mut out: Vec = Vec::new(); + semcode::search::query_function_or_macro_to_writer_verbose(&db, "report", &sha, &mut out, true) + .await + .unwrap(); + // Colour is written unconditionally and stripped by the stream on the way + // to a terminal, so a writer sees the escapes and a plain `find` misses + // every heading. + let printed = strip_colour(&String::from_utf8(out).unwrap()); + + // Each block runs from its file heading to the next one, so a call listed + // in the wrong block fails here rather than being found somewhere. + let block_for = |needle: &str| -> String { + let start = printed + .find(needle) + .unwrap_or_else(|| panic!("no block for {needle} in:\n{printed}")); + let rest = &printed[start..]; + let end = rest[needle.len()..] + .find("File: ") + .map(|offset| offset + needle.len()) + .unwrap_or(rest.len()); + rest[..end].to_string() + }; + + let header = block_for("File: include/linux/printk.h"); + assert!(header.contains("emit"), "{header}"); + assert!(!header.contains("fprintf"), "{header}"); + + let host = block_for("File: arch/x86/tools/decoder_test.c"); + assert!(host.contains("fprintf"), "{host}"); + assert!(!host.contains("emit"), "{host}"); +} + +#[tokio::test] +async fn a_use_of_the_name_is_not_an_answer_about_it() { + // Some rows are neither a definition nor a declaration. In Linux, + // arch/x86/xen/suspend_hvm.c:22 is `BUG_ON(xen_set_upcall_vector(cpu));` + // -- a call stored under the name it calls -- and being a `.c` file in the + // tree being audited it outranked every real definition, so `callers + // BUG_ON` answered about a use of BUG_ON and counted nine definitions + // where a callee query counted eight. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("include/asm-generic")).unwrap(); + std::fs::write( + repo.join("include/asm-generic/bug.h"), + "#define CHECK(cond) do { if (cond) report(); } while (0)\n", + ) + .unwrap(); + // The shape of the artefact: a statement that names CHECK, stored as a row + // of its own, ending in a semicolon with no braces of its own. + std::fs::write( + repo.join("use.c"), + "void start(void)\n{\n\tCHECK(ready());\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "a macro and a use of it"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + let chosen = db + .find_function_git_aware_reporting("CHECK", &sha) + .await + .unwrap() + .unwrap(); + // Whatever else is stored under this name, the answer is the definition. + assert_eq!( + chosen.function.file_path, "include/asm-generic/bug.h", + "answered about {}:{}", + chosen.function.file_path, chosen.function.line_start + ); + // And the count matches what a callee query reports, because both now ask + // the row's own text whether it defines the name. + let definitions = db + .get_function_callees_by_definition_git_aware("CHECK", &sha) + .await + .unwrap(); + let defining = definitions.iter().filter(|d| d.is_definition).count(); + assert_eq!(chosen.others.len() + 1, defining, "{definitions:?}"); +} + +#[tokio::test] +async fn two_languages_one_definition_each_is_not_a_majority() { + // The boundary the majority rung has to get right and the only one the + // languages this indexes can reach: C and Rust are the two groups a path + // extension can fall into, so a skew short of a majority needs a third + // language that is not parsed. One each is a tie, no majority, and the + // lower rungs decide -- the same way twice, which is the property worth + // pinning. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("rust/kernel")).unwrap(); + std::fs::write( + repo.join("driver.c"), + "int solo(int level)\n{\n\treturn emit(level);\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("rust/kernel/thing.rs"), + "impl Thing {\n pub fn solo(&self, level: i32) {\n self.record(level);\n }\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "one each"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string(), "rs".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + let first = db + .find_function_git_aware_reporting("solo", &sha) + .await + .unwrap() + .unwrap(); + let again = db + .find_function_git_aware_reporting("solo", &sha) + .await + .unwrap() + .unwrap(); + assert_eq!(first.function.file_path, again.function.file_path); + assert_eq!(first.others.len(), 1, "{:?}", first.others); + let note = first.ambiguity_note().unwrap(); + assert!(note.contains("defined 2 times"), "{note}"); +} + +fn strip_colour(text: &str) -> String { + let mut out = String::with_capacity(text.len()); + let mut chars = text.chars(); + while let Some(c) = chars.next() { + if c != '\u{1b}' { + out.push(c); + continue; + } + for escape in chars.by_ref() { + if escape == 'm' { + break; + } + } + } + out +} + +#[test] +fn a_path_component_names_another_program_where_a_prefix_does_not() { + use semcode::path_is_other_program; + + // The definition that made this necessary. Every directory named `tools` + // in Linux holds a host program, including twelve under `arch/`. + assert!(path_is_other_program("arch/x86/tools/insn_decoder_test.c")); + assert!(path_is_other_program("tools/lib/bpf/relo_core.c")); + assert!(path_is_other_program("samples/bpf/sockex1_kern.c")); + assert!(path_is_other_program("Documentation/tools/whatever.c")); + + assert!(!path_is_other_program("include/linux/printk.h")); + assert!(!path_is_other_program("kernel/fork.c")); + // A name is not a component: this is kernel code. + assert!(!path_is_other_program("drivers/tty/toolsomething.c")); + assert!(!path_is_other_program("mm/mytools.c")); +} From 749913aa6cd3132922dfb81b08f8003caf2ae14d Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Fri, 11 Sep 2026 16:29:26 -0700 Subject: [PATCH 2/6] query: take the types from the definition the answer names `callchain` names one definition, lists that definition's callees, and reported the types of whichever definition a third ranking ladder preferred: get_function_types_with_manifest() # before matches.max_by_key(|(path, start, end, _)| { (if path.ends_with(".h") { 0 } else { 1 }, end - start) }) That is the ladder a callee lookup used before it was made to ask where the chosen definition was read, and it is wrong the same way: it prefers a long `.c` body, so a report could name a definition in a header, list a second definition's callees and a third's types, with nothing saying they were about different functions. The commit survey in `query.rs` pairs the two calls directly, one ranked each way. The row is now looked up by the file and line the chosen definition was read from, so there is one place that decides which definition an answer is about. The test gives each definition a type of its own, because a first version gave them none: both answers were empty, and it passed whichever definition it read. With the ladder restored it fails, reporting `["host_ctx"]` under a header that names include/linux/printk.h. Also recorded, at the one resolver left that picks a definition without saying so: a callee query does that deliberately. Where the name has more than one definition it reports all of them and returns before anything names a single file, so nothing there chooses on a reader's behalf -- but the guard is an early return twenty lines further down, and moving it would restore the defect silently. The comment says which line that is. $ regress.sh # against the base commit functions 1054808 -> 1054808 +0 call_edges 3197541 -> 3197541 +0 VERDICT: ok PASS Nothing in the index changes. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/callchain.rs | 5 +++ src/database/connection.rs | 23 ++++++----- tests/ambiguous_single_answer.rs | 66 ++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/callchain.rs b/src/callchain.rs index cd2d0b9..1c13249 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -823,6 +823,11 @@ pub async fn show_callees_to_writer( writeln!(writer, "{search_msg}")?; // Search for function - macros are now stored as functions + // The silent resolver is what this wants: where the name has more than one + // definition the answer below is every definition and this returns before + // reaching anything that names a single file, so nothing here picks one on + // a reader's behalf. Moving that early return above this line would change + // that. let func_opt = db.find_function_git_aware(name, git_sha).await?; match func_opt { diff --git a/src/database/connection.rs b/src/database/connection.rs index f212a23..04b0a0b 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -2463,16 +2463,19 @@ impl DatabaseManager { return Ok(Vec::new()); } - // Select best match (prefer implementation over declaration) - let best_match = matches - .into_iter() - .max_by_key(|(file_path, line_start, line_end, _)| { - let line_count = line_end.saturating_sub(*line_start); - let is_header = file_path.ends_with(".h"); - (if is_header { 0 } else { 1 }, line_count) - }); - - match best_match { + // The definition the rest of the answer is about, looked up by where it + // was read. Ranking here as well is what let a chain name one + // definition and list another's callees; the types beside them were + // ranked by this copy of the older ladder and could disagree with both. + let Some(chosen) = self + .find_function_with_manifest_reporting(name, git_manifest) + .await? + else { + return Ok(Vec::new()); + }; + match matches.into_iter().find(|(file_path, line_start, _, _)| { + *file_path == chosen.function.file_path && *line_start == chosen.function.line_start + }) { Some((_, _, _, Some(types))) => Ok(types), _ => Ok(Vec::new()), } diff --git a/tests/ambiguous_single_answer.rs b/tests/ambiguous_single_answer.rs index b26baa6..c2d6e41 100644 --- a/tests/ambiguous_single_answer.rs +++ b/tests/ambiguous_single_answer.rs @@ -251,6 +251,72 @@ async fn the_listing_shows_each_definition_its_own_calls() { assert!(!host.contains("emit"), "{host}"); } +#[tokio::test] +async fn the_types_belong_to_the_definition_that_was_named() { + // Third part of one answer, third place it was ranked: the types beside a + // function were picked by a copy of the older ladder, so a report could + // name one definition, list a second one's callees and a third one's + // types. Nothing said they were about different functions. + // Each definition has to name a DIFFERENT type, or the two answers are + // both empty and the test passes whichever definition it read. The first + // version of this test did exactly that. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("arch/x86/tools")).unwrap(); + std::fs::create_dir_all(repo.join("include/linux")).unwrap(); + std::fs::write( + repo.join("include/linux/printk.h"), + "struct kdev { int id; };\n\ + static inline int report(struct kdev *dev)\n{\n\treturn dev->id;\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("arch/x86/tools/decoder_test.c"), + "struct host_ctx { int fd; };\n\ + int report(struct host_ctx *ctx)\n{\n\treturn ctx->fd;\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "two definitions, two types"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + let manifest = db.git_manifest_cached(&sha).await.unwrap(); + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + let types = db + .get_function_types_with_manifest("report", &manifest) + .await + .unwrap(); + + assert_eq!(chosen.function.file_path, "include/linux/printk.h"); + assert!( + types.iter().any(|t| t == "kdev" || t == "struct kdev"), + "named include/linux/printk.h and reported types {types:?}" + ); + assert!( + !types.iter().any(|t| t.contains("host_ctx")), + "types came from the host tool's definition: {types:?}" + ); +} + #[tokio::test] async fn a_use_of_the_name_is_not_an_answer_about_it() { // Some rows are neither a definition nor a declaration. In Linux, From b13286da0ddf7898b089dfcbd80adc5d3df1bfac Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Fri, 11 Sep 2026 17:20:09 -0700 Subject: [PATCH 3/6] mcp, lsp: stop answering about one definition of a name without saying so The command line reports which definition an answer is about and lists the ones it set aside. The two surfaces an editor and an agent use did not, and they are the surfaces where a reader has no other way to find out: an agent gets the text and nothing else, and a jump lands in a file without comment. Four tool paths resolved the name with the resolver that picks silently. Each now reports the choice it made, verified by driving the server over stdio against an indexed Linux tree at 0595459f: find_callers pr_warn Ambiguous: 'pr_warn' is defined 9 times in this revision. This answer is about include/linux/printk.h:563; the others are ... find_calls pr_warn the same note, then 2 callees find_callchain pr_warn the same note `find_calls` was also reading a macro's callees off the row the resolver returned rather than asking for the callees of the definition it names. Before this it answered `fprintf, va_end, va_start, vfprintf` for `pr_warn` -- the body of arch/x86/tools/insn_decoder_test.c:48, a host build tool. It now answers `pr_fmt, printk`. Going to the definition of a name with nine of them jumped to one of the nine. The protocol has nowhere to say a choice was made, but it takes a list of locations and an editor offers a list to the reader, so all nine are returned. Probed the same way, before and after: textDocument/definition on pr_warn in mm/slub.c:4295 before: 1 location include/linux/printk.h after: 9 locations, include/linux/printk.h first The definition the other commands answer about is first, because a client that jumps to the first entry rather than offering the list should land on the same one those commands describe. Functions and macros are stored in one table, so the two lookups the jump made were the same question asked twice and the second answer was unreachable. One lookup now. Still picking silently, and not addressed here: the file and line printed beside a neighbour's name in a caller or callee list, at four sites. That is one pick per row of a list thousands of rows long, so what to print there is a different question from this one. $ regress.sh functions 1054808 -> 1054808 +0 call_edges 3197541 -> 3197541 +0 VERDICT: ok PASS Nothing in the index changes. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/bin/semcode-lsp.rs | 141 +++++++++++++++++++++++++---------------- src/bin/semcode-mcp.rs | 57 +++++++++++------ 2 files changed, 123 insertions(+), 75 deletions(-) diff --git a/src/bin/semcode-lsp.rs b/src/bin/semcode-lsp.rs index 14e2bb7..aa120ae 100644 --- a/src/bin/semcode-lsp.rs +++ b/src/bin/semcode-lsp.rs @@ -129,71 +129,99 @@ impl SemcodeLspBackend { } } - async fn find_function_definition(&self, identifier_name: &str) -> Option { + /// Every place the identifier is defined, in the order a listing reports. + /// + /// One location where a name has several definitions is a guess, and the + /// protocol has nowhere to say so -- but it does take a list, and an editor + /// offers a list to the reader. `pr_warn` has nine definitions in Linux and + /// this used to jump to whichever one a heuristic preferred. + async fn find_function_definitions(&self, identifier_name: &str) -> Vec { if self.index_is_stale().await { - return None; + return Vec::new(); } let db_guard = self.database.lock().await; - let db = db_guard.as_ref()?; + let Some(db) = db_guard.as_ref() else { + return Vec::new(); + }; // Try git-aware lookup first if we have a git SHA let git_sha_guard = self.git_sha.lock().await; - let (func_result, macro_result, type_result, typedef_result) = - if let Some(git_sha) = git_sha_guard.as_ref() { - // Use git-aware lookup to find function, macro, type, and typedef at current commit - let func = db.find_function_git_aware(identifier_name, git_sha).await; - let mac = db.find_function_git_aware(identifier_name, git_sha).await; - let typ = db.find_type_git_aware(identifier_name, git_sha).await; - let typedef = db.find_typedef_git_aware(identifier_name, git_sha).await; - (func, mac, typ, typedef) - } else { - // Fall back to non-git-aware lookup - let func = db.find_function(identifier_name).await; - let mac = db.find_function(identifier_name).await; - let typ = db.find_type(identifier_name).await; - let typedef = db.find_typedef(identifier_name).await; - (func, mac, typ, typedef) - }; + // Functions and macros live in one table, so one lookup answers for + // both; asking twice was the same question twice. + let (functions, type_result, typedef_result) = if let Some(git_sha) = git_sha_guard.as_ref() + { + let mut functions = db + .find_all_functions_git_aware(identifier_name, git_sha) + .await + .unwrap_or_default(); + // The best guess leads. A client that offers a picker offers all of + // them either way; one that jumps to the first entry without asking + // should land on the definition the other commands answer about, + // not on whichever file sorts first. + if let Ok(Some(chosen)) = db + .find_function_git_aware_reporting(identifier_name, git_sha) + .await + { + functions.sort_by_key(|func| { + func.file_path != chosen.function.file_path + || func.line_start != chosen.function.line_start + }); + } + let typ = db.find_type_git_aware(identifier_name, git_sha).await; + let typedef = db.find_typedef_git_aware(identifier_name, git_sha).await; + (functions, typ, typedef) + } else { + // Fall back to non-git-aware lookup + let functions = db + .find_all_functions(identifier_name) + .await + .unwrap_or_default(); + let typ = db.find_type(identifier_name).await; + let typedef = db.find_typedef(identifier_name).await; + (functions, typ, typedef) + }; drop(git_sha_guard); // Prioritize: function > macro > type > typedef - let (file_path, line_start) = match (func_result, macro_result, type_result, typedef_result) - { - (Ok(Some(func)), _, _, _) => (func.file_path, func.line_start), - (_, Ok(Some(mac)), _, _) => (mac.file_path, mac.line_start), - (_, _, Ok(Some(typ)), _) => (typ.file_path, typ.line_start), - (_, _, _, Ok(Some(typedef))) => (typedef.file_path, typedef.line_start), - _ => return None, + let places: Vec<(String, u32)> = match (&functions, type_result, typedef_result) { + (functions, _, _) if !functions.is_empty() => functions + .iter() + .map(|func| (func.file_path.clone(), func.line_start)) + .collect(), + (_, Ok(Some(typ)), _) => vec![(typ.file_path, typ.line_start)], + (_, _, Ok(Some(typedef))) => vec![(typedef.file_path, typedef.line_start)], + _ => return Vec::new(), }; - // Convert relative file path to absolute path using git repo path + // Convert relative file paths to absolute paths using git repo path let repo_path_guard = self.git_repo_path.lock().await; - let absolute_path = if let Some(repo_path) = repo_path_guard.as_ref() { - // Join repo path with relative file path from database - std::path::Path::new(repo_path).join(&file_path) - } else { - // Fallback to relative path (shouldn't happen if database connected) - std::path::PathBuf::from(&file_path) - }; + let repo_path = repo_path_guard.clone(); drop(repo_path_guard); - // Convert absolute file path to URI - let file_uri = Uri::from_file_path(&absolute_path)?; - - // Create position (LSP uses 0-based line numbers) - let position = Position { - line: line_start.saturating_sub(1), - character: 0, - }; - - Some(Location { - uri: file_uri, - range: Range { - start: position, - end: position, - }, - }) + places + .into_iter() + .filter_map(|(file_path, line_start)| { + let absolute_path = match &repo_path { + Some(repo_path) => std::path::Path::new(repo_path).join(&file_path), + // Fallback to relative path (shouldn't happen if database connected) + None => std::path::PathBuf::from(&file_path), + }; + let file_uri = Uri::from_file_path(&absolute_path)?; + // Create position (LSP uses 0-based line numbers) + let position = Position { + line: line_start.saturating_sub(1), + character: 0, + }; + Some(Location { + uri: file_uri, + range: Range { + start: position, + end: position, + }, + }) + }) + .collect() } async fn find_function_references(&self, function_name: &str) -> Vec { @@ -395,11 +423,14 @@ impl LanguageServer for SemcodeLspBackend { None => return Ok(None), }; - // Find the function definition in the database - if let Some(location) = self.find_function_definition(&function_name).await { - Ok(Some(GotoDefinitionResponse::Scalar(location))) - } else { - Ok(None) + // Find the definitions in the database. Several is the honest answer + // where the tree defines the name several times; the editor offers the + // list rather than this picking one. + let mut locations = self.find_function_definitions(&function_name).await; + match locations.len() { + 0 => Ok(None), + 1 => Ok(Some(GotoDefinitionResponse::Scalar(locations.remove(0)))), + _ => Ok(Some(GotoDefinitionResponse::Array(locations))), } } diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index bb25a11..71b884d 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -279,10 +279,18 @@ async fn mcp_show_callers( // Find function or macro - both are stored in the functions table // Macros are distinguished by having an empty return_type - let entity_opt = db.find_function_git_aware(function_name, git_sha).await?; + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - match entity_opt { - Some(entity) => { + match chosen_opt { + Some(chosen) => { + // Callers are found by name, and a name can belong to several + // functions. An agent reading this has no other way to learn that. + if let Some(note) = chosen.ambiguity_note() { + writeln!(buffer, "Ambiguous: {note}")?; + } + let entity = chosen.function; let is_macro = entity.return_type.is_empty(); let entity_type = if is_macro { "macro" } else { "function" }; @@ -444,20 +452,25 @@ async fn mcp_show_calls( // Find function or macro - both are stored in the functions table // Macros are distinguished by having an empty return_type - let entity_opt = db.find_function_git_aware(function_name, git_sha).await?; + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - match entity_opt { - Some(entity) => { + match chosen_opt { + Some(chosen) => { + if let Some(note) = chosen.ambiguity_note() { + writeln!(buffer, "Ambiguous: {note}")?; + } + let entity = chosen.function; let is_macro = entity.return_type.is_empty(); let entity_type = if is_macro { "Macro" } else { "Function" }; - // Get callees - for macros, use the calls field; for functions, use db lookup - let calls = if is_macro { - entity.calls.clone().unwrap_or_default() - } else { - db.get_function_callees_git_aware(function_name, git_sha) - .await? - }; + // The callees of the definition named above. Reading them off the + // row for macros took them from whichever definition the resolver + // returned, which is the same silent choice one step earlier. + let calls = db + .get_function_callees_git_aware(function_name, git_sha) + .await?; if calls.is_empty() { writeln!( @@ -1708,12 +1721,11 @@ async fn mcp_show_callchain_with_limits( // by temporarily redirecting stdout to capture the output // First, check if function exists - let func_exists = db - .find_function_git_aware(function_name, git_sha) - .await? - .is_some(); + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - if !func_exists { + if chosen_opt.is_none() { writeln!( buffer, "Error: Function '{function_name}' not found in database at git SHA {git_sha}" @@ -1733,8 +1745,13 @@ async fn mcp_show_callchain_with_limits( // Use a simplified but functional approach that mimics the efficient implementation // This calls the underlying database method directly but captures output - // Get the function info first - if let Some(func) = db.find_function_git_aware(function_name, git_sha).await? { + // Get the function info first. One chain starts at one definition, so the + // chain says which one rather than reading as the tree having one. + if let Some(chosen) = chosen_opt { + if let Some(note) = chosen.ambiguity_note() { + writeln!(buffer, "Ambiguous: {note}")?; + } + let func = chosen.function; writeln!(buffer, "\n=== Function Information ===")?; writeln!( buffer, From 56e4ab8c25ee32577eb8e18d381eae79a4d4e637 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Fri, 11 Sep 2026 17:48:42 -0700 Subject: [PATCH 4/6] registrations: report where a handover goes for every definition of the name Following a function into the member it is installed in walked one definition of each name on the way, chosen by a heuristic, and reported the member it found as the answer. `call_rcu` has three definitions: kernel/rcu/tiny.c:158 head->func = func; kernel/rcu/tree.c:3299 __call_rcu_common(head, func, enable_rcu_lazy); tools/testing/shared/linux/radix-tree.h:24 The first stores the parameter itself; the second hands it to a wrapper that stores it; the third belongs to a test harness. Which one a caller reaches is a configuration question, and the route reported was whichever definition the heuristic preferred, with nothing said about the others: installs it in rcu_head::func, called later through call_rcu(func) -> __call_rcu_common(func) Every definition is walked now, each hop records the file and line its body was read from, and claims that reach the same member are one claim: installs it in rcu_head::func, called later through call_rcu(func) at kernel/rcu/tiny.c:158 (2 definitions agree) Two things change for a reader. The route is checkable, because it says which definition it went through. And a member two definitions agree on is a stronger claim than one read out of a single body, so the agreement is stated rather than being invisible either way. Where definitions disagree about where the parameter goes, both are reported under a line saying they disagree, instead of one of them being the answer. No name in the tree does: over 50 registered callbacks sampled from mm/, fs/, kernel/ and net/, 28 resolved to a member and every one of those had at least two definitions agreeing, 22 reached no member, and none disagreed. The exposure was total and the damage was nil -- which is worth stating plainly, because this fixes a mechanism that could produce a wrong claim rather than a wrong claim someone found. A branch dropped for taking an integer at that position now drops that branch instead of the whole answer. One definition of a name taking an int where another takes a function pointer does not settle the question for both. Two budgets, because reading bodies and following wrapper branches are two costs. One counter for both let the first eat the second: forty definitions of one hop spent the whole branch budget, and a claim two hops further on -- or a disagreement, which is the thing this reports -- stopped being found, silently. A test walks past a hop with forty definitions and fails without the split. A name and a parameter index are expanded once. Forty definitions of a wrapper enqueue the same next call forty times, and each of those pops was reading the row for every definition of it again: 1,300 database lookups for one claim, sixteen seconds of them, down to one second. Candidate paths also resolve through the cached manifest rather than one git tree walk per lookup, which is what `find_function_with_manifest` already resolves against -- so the two paths now agree about which blob a path has at a revision. $ regress.sh functions 1054808 -> 1054808 +0 call_edges 3197541 -> 3197541 +0 VERDICT: ok PASS Nothing in the index changes. `examples/handover_probe.rs` counts the disagreeing positions so the number above can be re-measured. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- examples/handover_probe.rs | 19 ++- src/callchain.rs | 91 ++++++++----- src/database/connection.rs | 219 +++++++++++++++++++++++-------- src/types.rs | 34 +++++ tests/ambiguous_single_answer.rs | 191 +++++++++++++++++++++++++++ tests/indirect_calls.rs | 10 +- 6 files changed, 464 insertions(+), 100 deletions(-) diff --git a/examples/handover_probe.rs b/examples/handover_probe.rs index 74230bc..8c5c4d5 100644 --- a/examples/handover_probe.rs +++ b/examples/handover_probe.rs @@ -38,6 +38,7 @@ async fn main() -> Result<()> { let mut unresolved: Vec = Vec::new(); let mut resolved = 0usize; let mut resolved_sites = 0usize; + let mut disagreeing = 0usize; let mut sites = 0usize; let mut examples = Vec::new(); for ((callee, index), count) in ranked.iter().take(sample) { @@ -58,21 +59,30 @@ async fn main() -> Result<()> { } function_valued += 1; - if let Some(handover) = db.follow_handed_parameter(callee, *index, &git_sha).await? { + // Every definition of the name is walked now, so a name can produce + // more than one claim. Two claims about one handover is a finding in + // its own right, so they are counted apart from the agreeing ones. + let handovers = db.follow_handed_parameter(callee, *index, &git_sha).await?; + if !handovers.is_empty() { resolved += 1; resolved_sites += count; + if handovers.len() > 1 { + disagreeing += 1; + } if examples.len() < 10 { + let (handover, agreeing) = &handovers[0]; examples.push(match handover { semcode::Handover::StoredIn { path, container_type, member, } => format!( - "{callee}[{index}] -> {container_type}::{member} via {}", - path.join(" -> ") + "{callee}[{index}] -> {container_type}::{member} via {} ({agreeing} agree, {} claims)", + path.join(" -> "), + handovers.len() ), semcode::Handover::Invoked { path } => { - format!("{callee}[{index}] -> called, via {}", path.join(" -> ")) + format!("{callee}[{index}] -> called, via {} ({agreeing} agree)", path.join(" -> ")) } }); } @@ -86,6 +96,7 @@ async fn main() -> Result<()> { println!("of the {sample} busiest positions ({sites} call sites):"); println!(" take a function at that position: {function_valued}"); println!(" reach a member: {resolved} positions, {resolved_sites} sites"); + println!(" positions whose definitions disagree about where it goes: {disagreeing}"); for example in examples { println!(" {example}"); } diff --git a/src/callchain.rs b/src/callchain.rs index 1c13249..a8e48f7 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -691,7 +691,7 @@ pub async fn show_registrations_to_writer( // Where that call puts it, and by what route: the slot is a // claim about the registrar, not about this call site. - let handover = db + let handovers = db .follow_handed_parameter(&argument.callee, argument.argument_index, git_sha) .await?; @@ -701,45 +701,66 @@ pub async fn show_registrations_to_writer( // rcu_head::func, so the inode is the subject. `request_irq(..., // handler, ..., netdev->name, ...)` also passes a member, and the // handler has nothing to do with it. - if let ( - Some(subject_type), - Some(subject_member), - Some(Handover::StoredIn { container_type, .. }), - ) = (&argument.subject_type, &argument.subject_member, &handover) + if let (Some(subject_type), Some(subject_member)) = + (&argument.subject_type, &argument.subject_member) { - let holds = db - .member_aggregate_git_aware(subject_type, subject_member, git_sha) - .await?; - if holds.as_deref() == Some(container_type.as_str()) { - writeln!( - writer, - " attached to {}::{}", - subject_type.cyan(), - subject_member.cyan(), - )?; + for (handover, _) in &handovers { + let Handover::StoredIn { container_type, .. } = handover else { + continue; + }; + let holds = db + .member_aggregate_git_aware(subject_type, subject_member, git_sha) + .await?; + if holds.as_deref() == Some(container_type.as_str()) { + writeln!( + writer, + " attached to {}::{}", + subject_type.cyan(), + subject_member.cyan(), + )?; + break; + } } } - match handover { - Some(Handover::StoredIn { - path, - container_type, - member, - }) => writeln!( - writer, - " installs it in {}::{}, {} through {}", - container_type.cyan(), - member.cyan(), - "called later".yellow(), - path.join(" -> ").bright_black(), - )?, - Some(Handover::Invoked { path }) => writeln!( + // Definitions that disagree about where the parameter goes are two + // claims about two configurations, and reporting one of them reads + // as the tree having one answer. + if handovers.len() > 1 { + writeln!( writer, - " calls it {} through {}", - "before returning".yellow(), - path.join(" -> ").bright_black(), - )?, - None => {} + " {} the definitions of {} disagree about where it goes:", + "Ambiguous:".bold().yellow(), + argument.callee.cyan(), + )?; + } + for (handover, agreeing) in &handovers { + let agreement = match agreeing { + 0 | 1 => String::new(), + count => format!(" ({count} definitions agree)"), + }; + match handover { + Handover::StoredIn { + path, + container_type, + member, + } => writeln!( + writer, + " installs it in {}::{}, {} through {}{}", + container_type.cyan(), + member.cyan(), + "called later".yellow(), + path.join(" -> ").bright_black(), + agreement.bright_black(), + )?, + Handover::Invoked { path } => writeln!( + writer, + " calls it {} through {}{}", + "before returning".yellow(), + path.join(" -> ").bright_black(), + agreement.bright_black(), + )?, + } } } } diff --git a/src/database/connection.rs b/src/database/connection.rs index 04b0a0b..58fca97 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -1389,7 +1389,7 @@ impl DatabaseManager { .await } - /// Where a call puts the function it is handed. + /// Where a call puts the function it is handed, per definition of the name. /// /// `request_irq(irq, nic_intr, ...)` installs nothing by itself: the /// wrapper hands its parameter to `request_threaded_irq`, which stores it @@ -1397,6 +1397,16 @@ impl DatabaseManager { /// in a member, and report the path taken, because a two-hop claim that /// reads like a one-hop fact is worse than no answer. /// + /// Every definition of a name on the way is walked, and each hop carries + /// the file and line its body was read from. One definition was walked + /// before, chosen by a heuristic: `call_rcu` has three, and the claim + /// `rcu_head::func` was read out of kernel/rcu/tree.c with nothing said + /// about kernel/rcu/tiny.c, which reaches the same member by storing the + /// parameter itself instead of handing it to `__call_rcu_common`. Which one + /// a caller reaches is a configuration question this does not answer, so + /// where definitions agree the claim is reported once and where they + /// disagree both are returned rather than one of them. + /// /// Bounded: a wrapper chain that has not reached a member within a few /// hops is not one, and a cycle must not spin. pub async fn follow_handed_parameter( @@ -1404,80 +1414,161 @@ impl DatabaseManager { callee: &str, argument_index: u32, git_sha: &str, - ) -> Result> { + ) -> Result> { const MAX_HOPS: usize = 4; + // Two budgets, because they bound two different things. MAX_BRANCHES + // bounds how many wrapper branches are followed, which is what stops a + // body handing its parameter to a dozen calls from turning into a + // search. MAX_DEFINITIONS bounds how many bodies are read, which is + // what stops a name with seventeen definitions from being expensive. + // Sharing one counter let the second eat the first: seventeen + // definitions of one hop exhausted the branch budget, and a claim two + // hops further on -- or a disagreement, the thing this reports -- + // silently stopped being found. const MAX_BRANCHES: usize = 32; + const MAX_DEFINITIONS: usize = 256; let mut seen = std::collections::HashSet::new(); + let mut expanded: std::collections::HashSet<(String, u32)> = + std::collections::HashSet::new(); // Breadth first: a body often hands the same parameter to more than // one call, an error path among them, and the first is not the one // that registers. let mut queue = std::collections::VecDeque::new(); queue.push_back((callee.to_string(), argument_index, Vec::::new())); - let mut visited = 0usize; + let mut branches = 0usize; + let mut definitions_read = 0usize; + let mut claims: Vec = Vec::new(); while let Some((current, index, path)) = queue.pop_front() { - if path.len() >= MAX_HOPS || visited >= MAX_BRANCHES { + if path.len() >= MAX_HOPS + || branches >= MAX_BRANCHES + || definitions_read >= MAX_DEFINITIONS + { continue; } - visited += 1; - if !seen.insert((current.clone(), index)) { + branches += 1; + // A name and a parameter index are expanded once. Forty + // definitions of one wrapper enqueue the same next call forty + // times, and this stops each of those forty pops from reading the + // row for every definition of it again: 1,300 database lookups for + // one claim, sixteen seconds of them. The per-definition check + // below is still needed, for a name reached by two routes. + if !expanded.insert((current.clone(), index)) { continue; } - let Some(function) = self.find_function_git_aware(¤t, git_sha).await? else { - continue; - }; - let Some(parameter) = function.parameters.get(index as usize) else { - continue; - }; - if parameter.name.is_empty() { - continue; - } - // An integer handed into an integer member is not a function - // being installed, and following it reports `nla_put_u32` - // installing something in `nlattr::nla_type`. - if path.is_empty() - && !self - .type_is_function_pointer(¶meter.type_name, git_sha) - .await? - { - return Ok(None); - } - - let mut path = path.clone(); - path.push(format!("{current}({})", parameter.name)); - - let fates = crate::TreeSitterAnalyzer::parameter_fate(&function.body, ¶meter.name); - for fate in &fates { - match fate { - crate::ParameterFate::StoredIn { - container_type, - member, - } if !container_type.is_empty() => { - return Ok(Some(crate::types::Handover::StoredIn { - path, - container_type: container_type.clone(), - member: member.clone(), - })); - } - crate::ParameterFate::Invoked => { - return Ok(Some(crate::types::Handover::Invoked { path })); - } - _ => {} + // Every definition of the name, not the one a heuristic prefers. + // A definition keyed by its own file and line, so two definitions + // of one name are two branches and not one. + // + // A missing name answers with no definitions rather than an error, + // so `?` here reports a database failure and does not abandon the + // walk for a hop the index has never seen. + for function in self.find_all_functions_git_aware(¤t, git_sha).await? { + if definitions_read >= MAX_DEFINITIONS { + break; } - } - for fate in &fates { - if let crate::ParameterFate::HandedOn { - callee, - argument_index, - } = fate + definitions_read += 1; + if !seen.insert(( + current.clone(), + index, + function.file_path.clone(), + function.line_start, + )) { + continue; + } + let Some(parameter) = function.parameters.get(index as usize) else { + continue; + }; + if parameter.name.is_empty() { + continue; + } + // An integer handed into an integer member is not a function + // being installed, and following it reports `nla_put_u32` + // installing something in `nlattr::nla_type`. One definition + // taking an integer there does not settle it for the rest, so + // this drops the branch rather than the whole answer. + if path.is_empty() + && !self + .type_is_function_pointer(¶meter.type_name, git_sha) + .await? { - queue.push_back((callee.clone(), *argument_index, path.clone())); + continue; + } + + let mut path = path.clone(); + // The file and line make the claim checkable. Without them a + // reader cannot tell which of three definitions of `call_rcu` + // the route was read from. + path.push(format!( + "{current}({}) at {}:{}", + parameter.name, function.file_path, function.line_start + )); + + let fates = + crate::TreeSitterAnalyzer::parameter_fate(&function.body, ¶meter.name); + let mut settled = false; + for fate in &fates { + match fate { + crate::ParameterFate::StoredIn { + container_type, + member, + } if !container_type.is_empty() => { + claims.push(crate::types::Handover::StoredIn { + path: path.clone(), + container_type: container_type.clone(), + member: member.clone(), + }); + settled = true; + break; + } + crate::ParameterFate::Invoked => { + claims.push(crate::types::Handover::Invoked { path: path.clone() }); + settled = true; + break; + } + _ => {} + } + } + if settled { + continue; + } + for fate in &fates { + if let crate::ParameterFate::HandedOn { + callee, + argument_index, + } = fate + { + queue.push_back((callee.clone(), *argument_index, path.clone())); + } } } } - Ok(None) + // Two definitions reaching the same member by different routes are one + // claim, and the shortest route is the one printed. That can be a route + // through a definition few builds use -- `call_rcu` is reached in one + // hop through kernel/rcu/tiny.c and in two through kernel/rcu/tree.c, + // and tiny.c is printed -- so the route names its file and the count + // says how many definitions agreed. Which one a build uses is a + // configuration this does not know. Two definitions that reach + // different members are two claims, which is the thing a single answer + // used to hide. + claims.sort_by_key(|claim| claim.path().len()); + let mut distinct: Vec<(crate::types::Handover, usize)> = Vec::new(); + for claim in claims { + match distinct + .iter_mut() + .find(|(kept, _)| kept.same_conclusion_as(&claim)) + { + // How many routes reached it is worth saying: a conclusion two + // definitions agree on is stronger than one read out of a + // single body, and only one of the routes gets printed. + Some((_, agreeing)) => *agreeing += 1, + None => distinct.push((claim, 1)), + } + } + Ok(distinct) } /// Everything installed in one member of one type. @@ -2506,10 +2597,22 @@ impl DatabaseManager { name ); - // Step 2: Resolve file paths to git hashes at target commit - let resolved_hashes = self - .resolve_git_file_hashes(&unique_file_paths, git_sha) - .await?; + // Step 2: Resolve file paths to git hashes at target commit, through + // the cached manifest. Resolving them against the repository on every + // call cost one git tree walk per lookup: following a handover through + // a name with forty definitions did forty of them and took fourteen + // seconds, where the manifest is read once. It is also what + // `find_function_with_manifest` already resolves against, so the two + // paths now agree about which blob a path has at a revision. + let git_manifest = self.git_manifest_cached(git_sha).await?; + let resolved_hashes: Vec<(String, String)> = unique_file_paths + .iter() + .filter_map(|file_path| { + git_manifest + .hash_of(file_path) + .map(|hash| (file_path.clone(), hash.to_string())) + }) + .collect(); if resolved_hashes.is_empty() { return self .functions_not_at_revision(name, git_sha, self.why_nothing_resolved(git_sha)) diff --git a/src/types.rs b/src/types.rs index fb7b75e..d244301 100644 --- a/src/types.rs +++ b/src/types.rs @@ -502,6 +502,40 @@ pub enum Handover { Invoked { path: Vec }, } +impl Handover { + pub fn path(&self) -> &[String] { + match self { + Handover::StoredIn { path, .. } => path, + Handover::Invoked { path } => path, + } + } + + /// Whether two routes end in the same place. + /// + /// `call_rcu` has three definitions and two of them reach + /// `rcu_head::func`, one storing the parameter itself and one handing it to + /// `__call_rcu_common`. That is one fact about where a callback goes, + /// reported twice; the route differs and the conclusion does not. + pub fn same_conclusion_as(&self, other: &Handover) -> bool { + match (self, other) { + ( + Handover::StoredIn { + container_type, + member, + .. + }, + Handover::StoredIn { + container_type: other_type, + member: other_member, + .. + }, + ) => container_type == other_type && member == other_member, + (Handover::Invoked { .. }, Handover::Invoked { .. }) => true, + _ => false, + } + } +} + impl Handover { /// Whether the call happens after the handover returns. /// diff --git a/tests/ambiguous_single_answer.rs b/tests/ambiguous_single_answer.rs index c2d6e41..a69233f 100644 --- a/tests/ambiguous_single_answer.rs +++ b/tests/ambiguous_single_answer.rs @@ -436,6 +436,197 @@ async fn two_languages_one_definition_each_is_not_a_majority() { assert!(note.contains("defined 2 times"), "{note}"); } +/// A tree where one registrar name has two definitions, as `call_rcu` does. +async fn tree_with_two_registrars( + second_member: &str, +) -> (tempfile::TempDir, Arc, String) { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("kernel/rcu")).unwrap(); + std::fs::write( + repo.join("head.h"), + "struct cb_head { void (*func)(struct cb_head *); struct cb_head *next; };\n\ + struct other_head { void (*other)(struct other_head *); };\n", + ) + .unwrap(); + // One definition stores the parameter itself. + std::fs::write( + repo.join("kernel/rcu/tiny.c"), + "#include \"head.h\"\nvoid queue_cb(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\thead->func = func;\n}\n", + ) + .unwrap(); + // The other hands it on, and the wrapper it hands it to stores it -- in + // the same member, or in a different one, depending on the caller. + std::fs::write( + repo.join("kernel/rcu/tree.c"), + format!( + "#include \"head.h\"\n\ + static void common_queue(struct cb_head *head, void (*func)(struct cb_head *))\n{{\n\thead->{second_member} = func;\n}}\n\n\ + void queue_cb(struct cb_head *head, void (*func)(struct cb_head *))\n{{\n\tcommon_queue(head, func);\n}}\n" + ), + ) + .unwrap(); + std::fs::write( + repo.join("user.c"), + "#include \"head.h\"\n\ + static void my_callback(struct cb_head *h)\n{\n\t(void)h;\n}\n\n\ + void start(struct cb_head *head)\n{\n\tqueue_cb(head, my_callback);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "two registrars"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + (dir, db, sha) +} + +#[tokio::test] +async fn agreeing_definitions_of_a_registrar_are_one_claim() { + // `call_rcu` has three definitions in Linux; two reach `rcu_head::func`, + // one storing the parameter and one handing it to `__call_rcu_common`. + // Walking a single silently-chosen definition reported one route as the + // fact. Same member, two routes, so it is one claim -- and that two + // definitions agree is worth saying, because it holds whichever is built. + let (_dir, db, sha) = tree_with_two_registrars("func").await; + + let claims = db + .follow_handed_parameter("queue_cb", 1, &sha) + .await + .unwrap(); + assert_eq!(claims.len(), 1, "{claims:?}"); + let (claim, agreeing) = &claims[0]; + assert_eq!(*agreeing, 2, "{claims:?}"); + match claim { + semcode::Handover::StoredIn { + container_type, + member, + path, + } => { + assert_eq!(member, "func", "{claim:?}"); + assert!(container_type.contains("cb_head"), "{claim:?}"); + // The route names the file it was read from, or the reader cannot + // tell which of the two definitions produced it. + assert!( + path.iter().any(|hop| hop.contains("kernel/rcu/")), + "{path:?}" + ); + } + other => panic!("{other:?}"), + } +} + +#[tokio::test] +async fn definitions_that_disagree_are_both_reported() { + // The case a single answer hid: two definitions of one registrar putting + // the callback in different members. Picking either one states a fact + // about a configuration the reader did not choose. + let (_dir, db, sha) = tree_with_two_registrars("next").await; + + let claims = db + .follow_handed_parameter("queue_cb", 1, &sha) + .await + .unwrap(); + let mut members: Vec = claims + .iter() + .filter_map(|(claim, _)| match claim { + semcode::Handover::StoredIn { member, .. } => Some(member.clone()), + _ => None, + }) + .collect(); + members.sort(); + assert_eq!( + members, + vec!["func".to_string(), "next".to_string()], + "{claims:?}" + ); +} + +#[tokio::test] +async fn a_hop_with_many_definitions_does_not_starve_the_rest_of_the_walk() { + // Walking every definition of every name shared one budget with following + // wrapper branches, so a hop with many definitions spent the whole budget + // and the claim two hops further on stopped being found -- silently, and + // the disagreement this reports would have gone with it. + // + // `noisy` here has 40 definitions, more than the 32 branches the walk will + // follow. The claim is three hops past it. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::write( + repo.join("head.h"), + "struct cb_head { void (*func)(struct cb_head *); };\n", + ) + .unwrap(); + // The intermediate, defined many times over, each definition handing the + // parameter on to the same next wrapper. + std::fs::create_dir_all(repo.join("drivers")).unwrap(); + for i in 0..40 { + std::fs::write( + repo.join(format!("drivers/d{i}.c")), + "#include \"head.h\"\nstatic void deep_store(struct cb_head *, void (*)(struct cb_head *));\n\ + static void noisy(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\tdeep_store(head, func);\n}\n", + ) + .unwrap(); + } + // The last hop, which actually stores it. + std::fs::write( + repo.join("store.c"), + "#include \"head.h\"\nvoid deep_store(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\thead->func = func;\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("entry.c"), + "#include \"head.h\"\n\ + static void noisy(struct cb_head *, void (*)(struct cb_head *));\n\ + void register_cb(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\tnoisy(head, func);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "a noisy intermediate"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + let claims = db + .follow_handed_parameter("register_cb", 1, &sha) + .await + .unwrap(); + assert!( + claims.iter().any(|(claim, _)| matches!( + claim, + semcode::Handover::StoredIn { member, .. } if member == "func" + )), + "the claim past the noisy hop was not found: {claims:?}" + ); +} + fn strip_colour(text: &str) -> String { let mut out = String::with_capacity(text.len()); let mut chars = text.chars(); diff --git a/tests/indirect_calls.rs b/tests/indirect_calls.rs index 17ea0b9..f92ad0c 100644 --- a/tests/indirect_calls.rs +++ b/tests/indirect_calls.rs @@ -392,10 +392,14 @@ async fn a_function_handed_to_a_call_is_recorded() { ); // Both hops stay visible: a two-hop claim that reads like a one-hop fact - // is worse than no answer. + // is worse than no answer. Each hop names the file and line its body was + // read from, because a name can have several definitions and a route + // through one of them is only checkable if it says which. assert!( - output.contains("request_irq(handler) -> request_threaded_irq(handler)"), - "the route was not reported:\n{output}" + output.contains( + "request_irq(handler) at irq.c:8 -> request_threaded_irq(handler) at irq.c:2" + ), + "the route was not reported with the definitions it was read from:\n{output}" ); // Installing is not calling: the handler runs when an interrupt arrives, From 0a0b8ad2afb8c32430b250567003e8e990ca5b8f Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Fri, 11 Sep 2026 18:35:38 -0700 Subject: [PATCH 5/6] query: ask one question about whether a row defines a function Three functions answered "does this row define the function it names", and they answered differently. A count printed beside an answer said how many definitions a name has, and invited the reader to `func` to see them, where a different one of the three counted: kfree reported 6 definitions, listed 5 container_of reported 12, listed 11 BUG_ON reported 9, listed 8 A body-length threshold ("more than 50 bytes") dropped the short definitions; requiring braces in a header dropped every macro defined in one, which is most of them; and a third test read the stored text. Of 41 names sampled across the tree, three commands agreed about 33 and disagreed about 6. `row_defines_the_function` is now the one test, and it is the one that was already reasoned about: a macro has no return type and is a definition however it is written, and otherwise the row's own stored text decides. They agree about all 41 now, 39 with several definitions and 2 with one. One caller cannot use it. A callee query reads the stored text by content hash and has no return type beside it, so it tests the text directly; the comment there says why, and the two agree wherever both can answer, since a macro's text begins with `#` and is not a prototype either way. The test asserts the three counts against each other rather than against a number, so the next predicate that diverges fails rather than being measured later. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/bin/semcode-mcp.rs | 4 ++-- src/database/connection.rs | 28 +++++++++++++++++----------- src/lib.rs | 8 ++++---- src/search.rs | 26 +++----------------------- src/types.rs | 18 ++++++++++++++++++ tests/ambiguous_single_answer.rs | 29 +++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 40 deletions(-) diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index 71b884d..cf16501 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -3,7 +3,7 @@ use anyhow::Result; use clap::Parser; use semcode::{ git, lore_writers::decode_email_body, pages::PageCache, process_database_path, - search::is_function_definition, search::LoreSearchOptions, DatabaseManager, LoreEmailFilters, + row_defines_the_function, search::LoreSearchOptions, DatabaseManager, LoreEmailFilters, }; use serde_json::{json, Value}; use std::io::Write; @@ -47,7 +47,7 @@ async fn mcp_query_function_or_macro( // Filter to only keep actual definitions (not declarations or call sites) let definitions: Vec<_> = all_matches .into_iter() - .filter(is_function_definition) + .filter(|func| row_defines_the_function(&func.return_type, &func.body)) .collect(); let result = if definitions.is_empty() { diff --git a/src/database/connection.rs b/src/database/connection.rs index 58fca97..056ae8b 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -2681,13 +2681,7 @@ impl DatabaseManager { fn filter_implementations_only(&self, functions: Vec) -> Vec { functions .into_iter() - .filter(|func| { - // Macros (empty return_type) are always implementations - if func.return_type.is_empty() { - return true; - } - !func.body.is_empty() && !crate::types::text_is_prototype(&func.body) - }) + .filter(|func| crate::types::row_defines_the_function(&func.return_type, &func.body)) .collect() } @@ -2755,8 +2749,8 @@ impl DatabaseManager { // under the name it calls. It is a .c file in the tree being // audited, so every rung below this one ranked it first, and a // question about BUG_ON was answered from a use of it. - let a_defines = !crate::types::text_is_prototype(&a.body); - let b_defines = !crate::types::text_is_prototype(&b.body); + let a_defines = crate::types::row_defines_the_function(&a.return_type, &a.body); + let b_defines = crate::types::row_defines_the_function(&b.return_type, &b.body); if a_defines != b_defines { return b_defines.cmp(&a_defines); } @@ -2826,7 +2820,9 @@ impl DatabaseManager { let mut matches = matches.into_iter(); let function = matches.next().unwrap(); let others = matches - .filter(|candidate| !crate::types::text_is_prototype(&candidate.body)) + .filter(|candidate| { + crate::types::row_defines_the_function(&candidate.return_type, &candidate.body) + }) .map(|candidate| crate::types::DefinitionSite { file_path: candidate.file_path, line_start: candidate.line_start, @@ -4103,7 +4099,10 @@ impl DatabaseManager { line_start: function.line_start, line_end: function.line_end, callees: function.calls.clone().unwrap_or_default(), - is_definition: !crate::types::text_is_prototype(&function.body), + is_definition: crate::types::row_defines_the_function( + &function.return_type, + &function.body, + ), }) .collect() }) @@ -5787,6 +5786,13 @@ impl DatabaseManager { line_start, line_end, callees: calls.unwrap_or_default(), + // The one place that cannot use `row_defines_the_function`: + // this reads the stored text by content hash and has no return + // type beside it. The two agree wherever both can answer -- a + // macro's text starts with `#`, which is not a prototype + // either way -- and the counts are checked against each other + // on the tree, so a divergence here would show up as two + // commands reporting different numbers. is_definition: !text.is_empty() && !crate::types::text_is_prototype(&text), }); } diff --git a/src/lib.rs b/src/lib.rs index f6b2229..83e86c9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -39,10 +39,10 @@ pub use text_utils::preprocess_code; pub use treesitter_analyzer::{ParameterFate, TreeSitterAnalyzer}; pub use types::Handover; pub use types::{ - path_is_other_program, path_language, ArgumentFunction, ChosenDefinition, DefinitionSite, - DispatchKind, DispatchSite, FieldInfo, FunctionInfo, GitCommitInfo, GitFileEntry, - GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, ParameterInfo, Registration, - RegistrationKind, TypeInfo, TypedefInfo, + path_is_other_program, path_language, row_defines_the_function, ArgumentFunction, + ChosenDefinition, DefinitionSite, DispatchKind, DispatchSite, FieldInfo, FunctionInfo, + GitCommitInfo, GitFileEntry, GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, + ParameterInfo, Registration, RegistrationKind, TypeInfo, TypedefInfo, }; pub use vectorizer::CodeVectorizer; pub use workdir::WorkdirIndex; diff --git a/src/search.rs b/src/search.rs index 62eb015..e30737f 100644 --- a/src/search.rs +++ b/src/search.rs @@ -1,4 +1,5 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 +use crate::types::row_defines_the_function; use crate::{CodeVectorizer, DatabaseManager}; use anstream::stdout; use anyhow::Result; @@ -1225,27 +1226,6 @@ pub async fn query_function_or_macro_to_writer_verbose( query_function_or_macro_to_writer_with_options(db, name, git_sha, writer, verbose).await } -/// Check if a function is actually a definition (has implementation) vs just a declaration -/// -/// The row's own text decides, the same test a callee query and the definition -/// chooser use. Requiring braces in a header decided it before, which dropped -/// every macro defined in one: `container_of` was listed 11 times where a -/// callee query and the ambiguity note both counted 12. Three commands then -/// disagreed about how many definitions a name has, and the note sends the -/// reader here to see them. -pub fn is_function_definition(func: &crate::FunctionInfo) -> bool { - if func.body.is_empty() { - return false; // Empty body is definitely a declaration - } - - // Macros have empty return_type and are always definitions (never just declarations) - if func.return_type.is_empty() { - return true; - } - - !crate::types::text_is_prototype(&func.body) -} - async fn query_function_or_macro_to_writer_with_options( db: &DatabaseManager, name: &str, @@ -1268,7 +1248,7 @@ async fn query_function_or_macro_to_writer_with_options( // Found functions only - filter out declarations and display only definitions let definitions: Vec<_> = func_results .iter() - .filter(|func| is_function_definition(func)) + .filter(|func| row_defines_the_function(&func.return_type, &func.body)) .collect(); if definitions.len() > 1 { @@ -1399,7 +1379,7 @@ async fn query_function_or_macro_to_writer_with_options( // Filter out declarations and show only definitions let regex_definitions: Vec<_> = regex_functions .iter() - .filter(|func| is_function_definition(func)) + .filter(|func| row_defines_the_function(&func.return_type, &func.body)) .collect(); for func in ®ex_definitions { display_function_to_writer_with_options(func, writer, true)?; diff --git a/src/types.rs b/src/types.rs index d244301..27012d3 100644 --- a/src/types.rs +++ b/src/types.rs @@ -184,6 +184,24 @@ pub fn path_language(file_path: &str) -> &str { } } +/// Whether a row defines the function it names, rather than declaring it. +/// +/// The one test for the question, so that two commands cannot answer it two +/// ways. Three of them used to: a body-length threshold listed five of the six +/// definitions of `kfree`, and requiring braces in a header dropped every macro +/// defined in one, so `container_of` was listed eleven times where the same +/// tree reported twelve. A macro is a definition however it is written. +pub fn row_defines_the_function(return_type: &str, body: &str) -> bool { + if body.is_empty() { + return false; + } + // A macro has no return type, and is a definition however it is written. + if return_type.is_empty() { + return true; + } + !text_is_prototype(body) +} + /// Whether stored text declares a function without defining it. /// /// The row's own text is the only thing that separates the two: a prototype diff --git a/tests/ambiguous_single_answer.rs b/tests/ambiguous_single_answer.rs index a69233f..1f89afb 100644 --- a/tests/ambiguous_single_answer.rs +++ b/tests/ambiguous_single_answer.rs @@ -661,3 +661,32 @@ fn a_path_component_names_another_program_where_a_prefix_does_not() { assert!(!path_is_other_program("drivers/tty/toolsomething.c")); assert!(!path_is_other_program("mm/mytools.c")); } + +#[tokio::test] +async fn the_three_commands_count_the_same_definitions() { + // The note tells the reader to run `func` to see the definitions it set + // aside, so the two have to agree about how many there are. They did not: + // three predicates answered "does this row define the function" three + // ways, and `kfree` was reported six times and listed five. + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + let listed = db + .find_all_functions_git_aware("report", &sha) + .await + .unwrap(); + let by_callee_query = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap() + .iter() + .filter(|definition| definition.is_definition) + .count(); + + assert_eq!(chosen.others.len() + 1, listed.len(), "note vs listing"); + assert_eq!(listed.len(), by_callee_query, "listing vs callee query"); +} From c8e8651f2b4b5e4dc86dbfdc53c75e3364249366 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Fri, 11 Sep 2026 18:36:09 -0700 Subject: [PATCH 6/6] query: mark a listed name whose file and line is one definition of several A caller or callee list prints each name with the file and line it is defined at. Where the tree defines that name more than once, that is one of them, picked the way every other single answer used to be picked, and a row of a list had no way to say so: callers -v kfree # before void (drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.c:389) struct dpp (drivers/gpu/drm/amd/display/dc/resource/dcn31/dcn31_resource.c:978) callers -v kfree # after void (.../sf/dev/dev.c:389) [1 of 2 definitions] struct dpp (.../dcn31_resource.c:978) [1 of 4 definitions] The count, not the paths. A caller list runs to thousands of rows -- 4,065 for `pr_warn` -- so the full note that a single answer carries would bury the list it annotates. Measured on this tree, 2 of 20 rows of one such list name something defined more than once, so the mark is rare enough to read and common enough to matter. A reader who wants the other definitions asks about that name, which now answers. Nothing is marked where the name has one definition, which is most rows. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/bin/query_impl/commands.rs | 26 +++++++++++++++++----- src/callchain.rs | 40 +++++++++++++++++++++++++++------- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index 98a86cc..4338ee7 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -286,12 +286,20 @@ async fn show_callchain_with_limits( println!("{}. {}", (i + 1).to_string().yellow(), caller.cyan()); // Show caller details if available - if let Ok(Some(caller_func)) = db.find_function_git_aware(caller, git_sha).await { + if let Ok(Some(chosen)) = db.find_function_git_aware_reporting(caller, git_sha).await { + // One of several definitions, where the name has several. A + // row of a list has no other room to say so. + let marker = match chosen.others.len() { + 0 => String::new(), + others => format!(" [1 of {} definitions]", others + 1), + }; + let caller_func = chosen.function; println!( - " └─ {} ({}:{})", + " └─ {} ({}:{}){}", caller_func.return_type.bright_black(), caller_func.file_path.bright_black(), - caller_func.line_start.to_string().bright_black() + caller_func.line_start.to_string().bright_black(), + marker.yellow() ); } @@ -347,12 +355,18 @@ async fn show_callchain_with_limits( println!("{}. {}", (i + 1).to_string().yellow(), callee.cyan()); // Show callee details if available - if let Ok(Some(callee_func)) = db.find_function_git_aware(callee, git_sha).await { + if let Ok(Some(chosen)) = db.find_function_git_aware_reporting(callee, git_sha).await { + let marker = match chosen.others.len() { + 0 => String::new(), + others => format!(" [1 of {} definitions]", others + 1), + }; + let callee_func = chosen.function; println!( - " └─ {} ({}:{})", + " └─ {} ({}:{}){}", callee_func.return_type.bright_black(), callee_func.file_path.bright_black(), - callee_func.line_start.to_string().bright_black() + callee_func.line_start.to_string().bright_black(), + marker.yellow() ); } diff --git a/src/callchain.rs b/src/callchain.rs index a8e48f7..6536a10 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -266,6 +266,21 @@ fn when_it_runs(level: &str) -> &'static str { .unwrap_or("runs at boot") } +/// What to add to a name's file and line where the tree defines the name more +/// than once. +/// +/// One row of a list is the wrong place for the full note: a caller list runs +/// to thousands of rows and `callers pr_warn` names 4,065 of them. Measured on +/// this tree, 2 of 20 rows of one such list have an ambiguous name, so the +/// count is worth carrying and the paths are not. The reader who wants them +/// asks about that name. +fn definition_marker(chosen: &crate::types::ChosenDefinition) -> String { + match chosen.others.len() { + 0 => String::new(), + others => format!(" [1 of {} definitions]", others + 1), + } +} + pub async fn show_callers_to_writer( db: &DatabaseManager, name: &str, @@ -420,15 +435,21 @@ pub async fn show_callers_to_writer( // Only perform extra lookups in verbose mode if verbose { // Get more info about the caller - if let Ok(Some(caller_func)) = - db.find_function_git_aware(caller, git_sha).await + if let Ok(Some(chosen)) = + db.find_function_git_aware_reporting(caller, git_sha).await { + // The file and line of a name with several + // definitions is one of them, and a row of a list + // has no other way to say so. + let marker = definition_marker(&chosen); + let caller_func = chosen.function; let info = format!( - " {} ({}:{}) [file SHA: {}]", + " {} ({}:{}) [file SHA: {}]{}", caller_func.return_type.bright_black(), caller_func.file_path.bright_black(), caller_func.line_start, - caller_func.git_file_hash.bright_black() + caller_func.git_file_hash.bright_black(), + marker.yellow() ); writeln!(writer, "{info}")?; } @@ -941,15 +962,18 @@ pub async fn show_callees_to_writer( // Only perform extra lookups in verbose mode if verbose { // Get more info about the callee - if let Ok(Some(callee_func)) = - db.find_function_git_aware(callee, git_sha).await + if let Ok(Some(chosen)) = + db.find_function_git_aware_reporting(callee, git_sha).await { + let marker = definition_marker(&chosen); + let callee_func = chosen.function; let info = format!( - " {} ({}:{}) [file SHA: {}]", + " {} ({}:{}) [file SHA: {}]{}", callee_func.return_type.bright_black(), callee_func.file_path.bright_black(), callee_func.line_start, - callee_func.git_file_hash.bright_black() + callee_func.git_file_hash.bright_black(), + marker.yellow() ); writeln!(writer, "{info}")?; }