Skip to content

fix(graph): address Qodo code-review findings on REFERENCES edges #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
prom3theu5 merged 1 commit into from
Jun 1, 2026
Merged

fix(graph): address Qodo code-review findings on REFERENCES edges #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions src/graph/ladybug_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,22 +357,29 @@ impl GraphStore for LadybugGraphStore {
fn symbol_type_relations(&self, symbol_name: &str) -> Result<Vec<RelatedItem>> {
let _guard = self.lock.lock().unwrap();
let conn = self.conn()?;
// Four directed traversals; each returns (file_path, reason).
// Four directed traversals; each returns (file_path, reason). Name
// matching is case-insensitive (lowercased param vs `lower(name)`) to
// align with the case-insensitive seed used by pack/related.
let name_lc = symbol_name.to_ascii_lowercase();
let queries = [
(
"MATCH (s:Symbol {name: $n})-[:INHERITS]->(t:Symbol) RETURN DISTINCT t.filePath",
"MATCH (s:Symbol)-[:INHERITS]->(t:Symbol) WHERE lower(s.name) = $n \
RETURN DISTINCT t.filePath",
"base type (inherits)",
),
(
"MATCH (s:Symbol)-[:INHERITS]->(t:Symbol {name: $n}) RETURN DISTINCT s.filePath",
"MATCH (s:Symbol)-[:INHERITS]->(t:Symbol) WHERE lower(t.name) = $n \
RETURN DISTINCT s.filePath",
"subtype (inherits)",
),
(
"MATCH (s:Symbol {name: $n})-[:IMPLEMENTS]->(t:Symbol) RETURN DISTINCT t.filePath",
"MATCH (s:Symbol)-[:IMPLEMENTS]->(t:Symbol) WHERE lower(s.name) = $n \
RETURN DISTINCT t.filePath",
"implemented interface/trait",
),
(
"MATCH (s:Symbol)-[:IMPLEMENTS]->(t:Symbol {name: $n}) RETURN DISTINCT s.filePath",
"MATCH (s:Symbol)-[:IMPLEMENTS]->(t:Symbol) WHERE lower(t.name) = $n \
RETURN DISTINCT s.filePath",
"implementor",
),
];
Expand All @@ -382,10 +389,7 @@ impl GraphStore for LadybugGraphStore {
.prepare(cypher)
.map_err(|e| anyhow!("preparing symbol_type_relations: {e}"))?;
let result = conn
.execute(
&mut stmt,
vec![("n", Value::String(symbol_name.to_string()))],
)
.execute(&mut stmt, vec![("n", Value::String(name_lc.clone()))])
.map_err(|e| anyhow!("executing symbol_type_relations: {e}"))?;
for row in result {
out.push(RelatedItem {
Expand All @@ -403,16 +407,17 @@ impl GraphStore for LadybugGraphStore {
fn symbol_references(&self, symbol_name: &str) -> Result<Vec<RelatedItem>> {
let _guard = self.lock.lock().unwrap();
let conn = self.conn()?;
// Incoming REFERENCES edges = files that reference this symbol.
let cypher =
"MATCH (s:Symbol)-[:REFERENCES]->(t:Symbol {name: $n}) RETURN DISTINCT s.filePath";
// Incoming REFERENCES edges = files that reference this symbol. Name
// matching is case-insensitive to align with the seed lookup.
let cypher = "MATCH (s:Symbol)-[:REFERENCES]->(t:Symbol) WHERE lower(t.name) = $n \
RETURN DISTINCT s.filePath";
let mut stmt = conn
.prepare(cypher)
.map_err(|e| anyhow!("preparing symbol_references: {e}"))?;
let result = conn
.execute(
&mut stmt,
vec![("n", Value::String(symbol_name.to_string()))],
vec![("n", Value::String(symbol_name.to_ascii_lowercase()))],
)
.map_err(|e| anyhow!("executing symbol_references: {e}"))?;
let reason = format!("references {symbol_name}");
Expand Down Expand Up @@ -564,13 +569,17 @@ impl GraphStore for LadybugGraphStore {
{
let _guard = self.lock.lock().unwrap();
let conn = self.conn()?;
let cypher =
"MATCH (s:Symbol {name: $name}) RETURN DISTINCT s.filePath ORDER BY s.filePath";
// Case-insensitive to align with the seed lookup.
let cypher = "MATCH (s:Symbol) WHERE lower(s.name) = $name \
RETURN DISTINCT s.filePath ORDER BY s.filePath";
let mut stmt = conn
.prepare(cypher)
.map_err(|e| anyhow!("preparing related query: {e}"))?;
let result = conn
.execute(&mut stmt, vec![("name", Value::String(symbol.to_string()))])
.execute(
&mut stmt,
vec![("name", Value::String(symbol.to_ascii_lowercase()))],
)
.map_err(|e| anyhow!("executing related query: {e}"))?;
for row in result {
out.push(RelatedItem {
Expand Down
102 changes: 50 additions & 52 deletions src/graph/memory_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,36 @@ fn ci_contains(haystack: &str, needle: &str) -> bool {
.contains(&needle.to_ascii_lowercase())
}

/// Files that reference `symbol_name` via incoming `REFERENCES` edges (callers
/// / instantiation sites), deterministic and de-duplicated by path. Name match
/// is case-insensitive to align with the seed lookup. Lock-free so it can be
/// called from methods that already hold the inner mutex (the single source of
/// truth for both `symbol_references` and `related_to_symbol`'s reference half).
fn collect_references(g: &Inner, symbol_name: &str) -> Vec<RelatedItem> {
let target_ids: BTreeSet<&str> = g
.symbols
.values()
.filter(|s| s.name.eq_ignore_ascii_case(symbol_name))
.map(|s| s.id.as_str())
.collect();
let reason = format!("references {symbol_name}");
let mut out: Vec<RelatedItem> = Vec::new();
for (from, to) in &g.sym_references {
if target_ids.contains(to.as_str())
&& let Some(s) = g.symbols.get(from)
{
out.push(RelatedItem {
path: s.file_path.clone(),
reason: reason.clone(),
depth: 1,
});
}
}
out.sort_by(|a, b| a.path.cmp(&b.path));
out.dedup_by(|a, b| a.path == b.path);
out
}

impl GraphStore for MemoryGraphStore {
fn initialize_schema(&self) -> Result<()> {
Ok(())
Expand All @@ -72,14 +102,16 @@ impl GraphStore for MemoryGraphStore {
// Drop the file's IMPORTS_PACKAGE edges and project membership.
g.file_pkgs.retain(|(f, _)| f != &fid);
g.project_files.retain(|(_, f)| f != &fid);
// Drop symbol-level edges originating from this file's symbols.
let sym_prefix = format!("sym:{path}#");
g.sym_inherits
.retain(|(from, _)| !from.starts_with(&sym_prefix));
g.sym_implements
.retain(|(from, _)| !from.starts_with(&sym_prefix));
g.sym_references
.retain(|(from, _)| !from.starts_with(&sym_prefix));
// Drop symbol-level edges whose *either* endpoint no longer exists. The
// symbols are already removed above, so retaining only edges with both
// endpoints live cleans up references in both directions — including
// edges that merely *point at* a symbol declared in the removed file.
// (A `from`-only sweep would leak those and inflate `referenceEdges`.)
let live: BTreeSet<String> = g.symbols.keys().cloned().collect();
let both_live = |(from, to): &(String, String)| live.contains(from) && live.contains(to);
g.sym_inherits.retain(both_live);
g.sym_implements.retain(both_live);
g.sym_references.retain(both_live);
Ok(())
}

Expand Down Expand Up @@ -165,38 +197,17 @@ impl GraphStore for MemoryGraphStore {

fn symbol_references(&self, symbol_name: &str) -> Result<Vec<RelatedItem>> {
let g = self.inner.lock().unwrap();
// Symbol ids declaring this name (the reference targets).
let ids: BTreeSet<&str> = g
.symbols
.values()
.filter(|s| s.name == symbol_name)
.map(|s| s.id.as_str())
.collect();
let reason = format!("references {symbol_name}");
let mut out: Vec<RelatedItem> = Vec::new();
for (from, to) in &g.sym_references {
if ids.contains(to.as_str())
&& let Some(s) = g.symbols.get(from)
{
out.push(RelatedItem {
path: s.file_path.clone(),
reason: reason.clone(),
depth: 1,
});
}
}
out.sort_by(|a, b| a.path.cmp(&b.path));
out.dedup_by(|a, b| a.path == b.path);
Ok(out)
Ok(collect_references(&g, symbol_name))
}

fn symbol_type_relations(&self, symbol_name: &str) -> Result<Vec<RelatedItem>> {
let g = self.inner.lock().unwrap();
// Symbol ids declaring this name.
// Symbol ids declaring this name (case-insensitive, to align with the
// seed lookup used by pack/related).
let ids: BTreeSet<&str> = g
.symbols
.values()
.filter(|s| s.name == symbol_name)
.filter(|s| s.name.eq_ignore_ascii_case(symbol_name))
.map(|s| s.id.as_str())
.collect();
let file_of = |id: &str| g.symbols.get(id).map(|s| s.file_path.clone());
Expand Down Expand Up @@ -293,11 +304,9 @@ impl GraphStore for MemoryGraphStore {
fn related_to_symbol(&self, symbol: &str, _depth: usize) -> Result<Vec<RelatedItem>> {
let g = self.inner.lock().unwrap();
let mut out = Vec::new();
// Files declaring an exactly-named symbol are depth 0.
let mut target_ids: BTreeSet<&str> = BTreeSet::new();
// Files declaring a symbol of this name (case-insensitive) are depth 0.
for s in g.symbols.values() {
if s.name == symbol {
target_ids.insert(s.id.as_str());
if s.name.eq_ignore_ascii_case(symbol) {
out.push(RelatedItem {
path: s.file_path.clone(),
reason: "exact symbol declaration".to_string(),
Expand All @@ -306,21 +315,10 @@ impl GraphStore for MemoryGraphStore {
}
}
// Files that reference the symbol via incoming REFERENCES edges are
// depth 1 (callers/instantiation sites). This traversal is what makes
// usages visible to `pack`/`related`. Mirror the lock-free inline form
// of `symbol_references` to avoid re-locking the mutex.
let reason = format!("references {symbol}");
for (from, to) in &g.sym_references {
if target_ids.contains(to.as_str())
&& let Some(s) = g.symbols.get(from)
{
out.push(RelatedItem {
path: s.file_path.clone(),
reason: reason.clone(),
depth: 1,
});
}
}
// depth 1 (callers/instantiation sites) — the traversal that makes
// usages visible to `pack`/`related`. Shares `collect_references` (the
// single source of truth) while still holding the lock once.
out.extend(collect_references(&g, symbol));
// Deterministic; declaration (depth 0) wins over reference (depth 1)
// when the same file both declares and references the symbol.
out.sort_by(|a, b| a.path.cmp(&b.path).then(a.depth.cmp(&b.depth)));
Expand Down
91 changes: 63 additions & 28 deletions src/indexer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,16 @@ fn resolve_imports(
Ok(edges)
}

/// Whether `candidate_path` lives in the same project directory as a file whose
/// parent directory is `project_dir`. Segment-safe: `src` does not match
/// `src2/foo` — the match must fall on a `/` boundary (or be the dir itself).
fn same_project_dir(candidate_path: &str, project_dir: &str) -> bool {
!project_dir.is_empty()
&& candidate_path
.strip_prefix(project_dir)
.is_some_and(|rest| rest.starts_with('/'))
}

/// Resolve collected supertype relationships into `INHERITS`/`IMPLEMENTS` edges.
/// Returns the number of edges created.
///
Expand Down Expand Up @@ -378,9 +388,7 @@ fn resolve_supertypes(
} else {
let same_proj: Vec<_> = targets
.iter()
.filter(|s| {
!project_prefix.is_empty() && s.file_path.starts_with(project_prefix)
})
.filter(|s| same_project_dir(&s.file_path, project_prefix))
.cloned()
.collect();
if !same_proj.is_empty() {
Expand Down Expand Up @@ -427,36 +435,66 @@ fn resolve_references(
store: &dyn GraphStore,
pending: &[(String, Vec<tree_sitter::Reference>)],
) -> Result<usize> {
use crate::graph::model::SymbolSearchQuery;
use crate::graph::model::{IndexedSymbol, SymbolSearchQuery};
use std::collections::HashMap;

// Cache of target candidates keyed by referenced name, shared across every
// file — references to the same type recur constantly, so this turns an
// O(#refs) query pattern into O(#distinct names). Each entry holds the full
// exact-name candidate set (any file); same-file/same-project narrowing is
// applied per reference below.
let mut to_cache: HashMap<String, Vec<IndexedSymbol>> = HashMap::new();

let mut edges = 0;
for (file, refs) in pending {
let project_prefix = file.rsplit_once('/').map(|(d, _)| d).unwrap_or("");

// Preload every declaration in this file once, indexed by name, for the
// `from` (enclosing-declaration) lookup — one query per file instead of
// one per reference. Candidates are sorted so selection is deterministic
// when a file has multiple same-named declarations (overloads, etc.).
let mut from_by_name: HashMap<&str, Vec<&IndexedSymbol>> = HashMap::new();
let file_symbols = store.symbols_matching(&SymbolSearchQuery {
file: Some(file.clone()),
..Default::default()
})?;
for sym in &file_symbols {
from_by_name.entry(sym.name.as_str()).or_default().push(sym);
}
for list in from_by_name.values_mut() {
list.sort_by(|a, b| {
a.start_line
.cmp(&b.start_line)
.then(a.end_line.cmp(&b.end_line))
.then(a.id.cmp(&b.id))
});
}

for r in refs {
// The referring symbol must be a declaration in this file.
if r.from.is_empty() {
continue;
}
let from_candidates = store.symbols_matching(&SymbolSearchQuery {
name: Some(r.from.clone()),
file: Some(file.clone()),
..Default::default()
})?;
let Some(from) = from_candidates.into_iter().find(|s| s.name == r.from) else {
let Some(from) = from_by_name.get(r.from.as_str()).and_then(|v| v.first()) else {
continue;
};

// Candidate target symbols (exact name match, any file). An empty
// set means the name isn't a declared symbol (e.g. a local var) —
// no edge, which is the false-positive guard.
let mut targets: Vec<_> = store
.symbols_matching(&SymbolSearchQuery {
name: Some(r.to.clone()),
..Default::default()
})?
.into_iter()
.filter(|s| s.name == r.to && s.id != from.id)
.collect();
// Candidate target symbols (exact name match, any file), cached
// across files. An empty set means the name isn't a declared symbol
// (e.g. a local var) — no edge, the false-positive guard.
if !to_cache.contains_key(&r.to) {
let candidates = store
.symbols_matching(&SymbolSearchQuery {
name: Some(r.to.clone()),
..Default::default()
})?
.into_iter()
.filter(|s| s.name == r.to)
.collect();
to_cache.insert(r.to.clone(), candidates);
}
let mut targets: Vec<&IndexedSymbol> =
to_cache[&r.to].iter().filter(|s| s.id != from.id).collect();
if targets.is_empty() {
continue;
}
Expand All @@ -466,25 +504,22 @@ fn resolve_references(
let same_file: Vec<_> = targets
.iter()
.filter(|s| s.file_path == *file)
.cloned()
.copied()
.collect();
if !same_file.is_empty() {
targets = same_file;
} else {
let same_proj: Vec<_> = targets
.iter()
.filter(|s| {
!project_prefix.is_empty() && s.file_path.starts_with(project_prefix)
})
.cloned()
.filter(|s| same_project_dir(&s.file_path, project_prefix))
.copied()
.collect();
if !same_proj.is_empty() {
targets = same_proj;
}
}

// Deterministic order when an ambiguous name produces multiple
// edges (sort by target file path).
// Deterministic order when an ambiguous name produces multiple edges.
targets.sort_by(|a, b| a.file_path.cmp(&b.file_path).then(a.id.cmp(&b.id)));
for target in targets {
store.link_symbol_references(&from.id, &target.id)?;
Expand Down
Loading
Loading