Skip to content

Commit 64a9525

Browse files
committed
fix(python): apply the same call-graph fix to the parallel builder
`builder::build_graph` is a second implementation of the same job as `parser_impl::ir_to_graph` (the former is reached only through `parser.rs`'s `parse_source`). It carried the same two defects as the server path, so it is brought to parity here. It also had a defect of its own: the extractor lists a class's methods in both `ir.functions` and `ir.classes[].methods`, so every method was built twice — once as a stray module-level node and once as a real method node linked to its class. The duplicate left two nodes with the same name and split the call edges away from the node that name lookups found. Module function entries that are really class methods are now skipped (matched on name and line span, so a same-named module function is kept). The bare method name is registered alongside the qualified one so a call like `self.helper()` resolves within its own file instead of relying on the server's global name lookup, which cannot distinguish two classes that share a method name.
1 parent 9e586f6 commit 64a9525

1 file changed

Lines changed: 219 additions & 5 deletions

File tree

‎crates/codegraph-python/src/builder.rs‎

Lines changed: 219 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,37 @@ pub fn build_graph(graph: &mut CodeGraph, ir: &CodeIR, file_path: &str) -> Resul
2828
// Track entity name -> NodeId mappings for relationship building
2929
let mut entity_map: HashMap<String, NodeId> = HashMap::new();
3030

31+
// The extractor lists a class's methods in BOTH `ir.functions` and
32+
// `ir.classes[].methods` (see `apply_class_extraction`). Without this
33+
// filter every method was built twice — once as a stray module-level
34+
// function node and once as a real method node linked to its class. The
35+
// duplicate left the graph with two nodes carrying the same name, and the
36+
// call edges landed on the method node while lookups by name hit the
37+
// orphan, so callers/callees looked empty. Match on name AND line span so
38+
// a module-level function that merely shares a name with a method is kept.
39+
let method_spans: std::collections::HashSet<(&str, i64, i64)> = ir
40+
.classes
41+
.iter()
42+
.flat_map(|class| class.methods.iter())
43+
.map(|method| {
44+
(
45+
method.name.as_str(),
46+
method.line_start as i64,
47+
method.line_end as i64,
48+
)
49+
})
50+
.collect();
51+
3152
// Add all functions
3253
for func in &ir.functions {
54+
// This function is a class method — the class pass below owns it.
55+
if method_spans.contains(&(
56+
func.name.as_str(),
57+
func.line_start as i64,
58+
func.line_end as i64,
59+
)) {
60+
continue;
61+
}
3362
let mut props = PropertyMap::new()
3463
.with("name", func.name.clone())
3564
.with("signature", func.signature.clone())
@@ -110,16 +139,53 @@ pub fn build_graph(graph: &mut CodeGraph, ir: &CodeIR, file_path: &str) -> Resul
110139
// Track methods with qualified name for call relationships
111140
let qualified = format!("{}.{}", class.name, method.name);
112141
entity_map.insert(qualified, method_id);
142+
// Also register the bare method name so a call like
143+
// `self.helper()` inside the same file resolves directly, instead
144+
// of relying on the server's global name lookup (which cannot
145+
// distinguish two classes that share a method name).
146+
entity_map.entry(method.name.clone()).or_insert(method_id);
113147
}
114148
}
115149

116150
// Add call relationships
151+
//
152+
// A callee that lives in another file is not in this file's entity map, so
153+
// its edge cannot be drawn here. Those calls are parked on the caller node's
154+
// `unresolved_calls` property and connected later by the server's
155+
// cross-file resolution pass (GraphUpdater::resolve_cross_file_imports).
156+
// Same pattern as codegraph-fortran / codegraph-rust / codegraph-swift.
157+
let mut unresolved_calls: HashMap<String, Vec<String>> = HashMap::new();
158+
117159
for call in &ir.calls {
118-
if let (Some(&caller_id), Some(&callee_id)) =
119-
(entity_map.get(&call.caller), entity_map.get(&call.callee))
120-
{
121-
helpers::add_call(graph, caller_id, callee_id, call.call_site_line as i64)
122-
.map_err(|e| crate::error::ParseError::GraphError(e.to_string()))?;
160+
if let Some(&caller_id) = entity_map.get(&call.caller) {
161+
if let Some(&callee_id) = entity_map.get(&call.callee) {
162+
helpers::add_call(graph, caller_id, callee_id, call.call_site_line as i64)
163+
.map_err(|e| crate::error::ParseError::GraphError(e.to_string()))?;
164+
} else {
165+
unresolved_calls
166+
.entry(call.caller.clone())
167+
.or_default()
168+
.push(call.callee.clone());
169+
}
170+
}
171+
}
172+
173+
// Store unresolved calls on caller nodes for cross-file resolution
174+
for (caller_name, callees) in unresolved_calls {
175+
if let Some(&caller_id) = entity_map.get(&caller_name) {
176+
if let Ok(node) = graph.get_node(caller_id) {
177+
let mut all_callees: Vec<String> = node
178+
.properties
179+
.get_string_list_compat("unresolved_calls")
180+
.unwrap_or_default();
181+
for callee in &callees {
182+
if !all_callees.iter().any(|c| c == callee) {
183+
all_callees.push(callee.clone());
184+
}
185+
}
186+
let new_props = node.properties.clone().with("unresolved_calls", all_callees);
187+
let _ = graph.update_node_properties(caller_id, new_props);
188+
}
123189
}
124190
}
125191

@@ -214,4 +280,152 @@ mod tests {
214280
let result = build_graph(&mut graph, &ir, "test.py");
215281
assert!(result.is_ok());
216282
}
283+
284+
#[test]
285+
fn test_local_call_creates_edge() {
286+
let mut graph = CodeGraph::in_memory().unwrap();
287+
let mut ir = CodeIR::new(std::path::PathBuf::from("test.py"));
288+
289+
ir.add_function(FunctionEntity::new("caller", 1, 3));
290+
ir.add_function(FunctionEntity::new("callee", 5, 7));
291+
ir.add_call(CallRelation::new("caller", "callee", 2));
292+
293+
build_graph(&mut graph, &ir, "test.py").unwrap();
294+
295+
let funcs = graph
296+
.query()
297+
.node_type(NodeType::Function)
298+
.execute()
299+
.expect("query functions");
300+
assert_eq!(funcs.len(), 2);
301+
302+
let caller = funcs
303+
.iter()
304+
.find(|id| {
305+
graph
306+
.get_node(**id)
307+
.ok()
308+
.and_then(|n| n.properties.get_string("name").map(|n| n == "caller"))
309+
.unwrap_or(false)
310+
})
311+
.copied()
312+
.expect("caller node");
313+
let callee = funcs
314+
.iter()
315+
.find(|id| {
316+
graph
317+
.get_node(**id)
318+
.ok()
319+
.and_then(|n| n.properties.get_string("name").map(|n| n == "callee"))
320+
.unwrap_or(false)
321+
})
322+
.copied()
323+
.expect("callee node");
324+
325+
let edges = graph
326+
.get_edges_between(caller, callee)
327+
.expect("edges between caller and callee");
328+
assert!(
329+
edges.iter().any(|e| {
330+
graph
331+
.get_edge(*e)
332+
.map(|edge| edge.edge_type == EdgeType::Calls)
333+
.unwrap_or(false)
334+
}),
335+
"expected a Calls edge between caller and callee"
336+
);
337+
}
338+
339+
#[test]
340+
fn test_class_method_yields_single_node_and_call_edge() {
341+
// The extractor lists a class's methods in both `ir.functions` and
342+
// `ir.classes[].methods`. The builder must not build a stray
343+
// module-level copy of every method — two nodes with the same name
344+
// split the call edges away from the node that lookups find.
345+
let source = r#"
346+
class TradingEngine:
347+
def run_tick(self):
348+
self._emit_ops("x")
349+
350+
def _emit_ops(self, x):
351+
pass
352+
"#;
353+
let config = crate::config::ParserConfig::default();
354+
let ir = crate::extractor::extract(source, std::path::Path::new("test.py"), &config)
355+
.expect("extract source");
356+
357+
let mut graph = CodeGraph::in_memory().unwrap();
358+
build_graph(&mut graph, &ir, "test.py").unwrap();
359+
360+
let funcs = graph
361+
.query()
362+
.node_type(NodeType::Function)
363+
.execute()
364+
.expect("query functions");
365+
assert_eq!(
366+
funcs.len(),
367+
2,
368+
"each method must yield exactly one node, got {}",
369+
funcs.len()
370+
);
371+
372+
let find = |name: &str| {
373+
funcs
374+
.iter()
375+
.find(|id| {
376+
graph
377+
.get_node(**id)
378+
.ok()
379+
.and_then(|n| n.properties.get_string("name").map(|n| n == name))
380+
.unwrap_or(false)
381+
})
382+
.copied()
383+
.expect("node for method")
384+
};
385+
let caller = find("run_tick");
386+
let callee = find("_emit_ops");
387+
388+
let edges = graph
389+
.get_edges_between(caller, callee)
390+
.expect("edges between run_tick and _emit_ops");
391+
assert!(
392+
edges.iter().any(|e| {
393+
graph
394+
.get_edge(*e)
395+
.map(|edge| edge.edge_type == EdgeType::Calls)
396+
.unwrap_or(false)
397+
}),
398+
"expected a Calls edge from run_tick to _emit_ops"
399+
);
400+
}
401+
402+
#[test]
403+
fn test_unresolved_call_is_parked_for_cross_file_resolution() { let mut graph = CodeGraph::in_memory().unwrap();
404+
let mut ir = CodeIR::new(std::path::PathBuf::from("test.py"));
405+
406+
ir.add_function(FunctionEntity::new("caller", 1, 3));
407+
// The callee lives in another file, so it is not in this file's entity
408+
// map. It must be parked on the caller node as `unresolved_calls` so the
409+
// server's cross-file pass can connect it later.
410+
ir.add_call(CallRelation::new("caller", "partial_close", 2));
411+
412+
build_graph(&mut graph, &ir, "test.py").unwrap();
413+
414+
let funcs = graph
415+
.query()
416+
.node_type(NodeType::Function)
417+
.execute()
418+
.expect("query functions");
419+
assert_eq!(funcs.len(), 1);
420+
421+
let node = graph.get_node(funcs[0]).expect("caller node");
422+
let unresolved = node
423+
.properties
424+
.get_string_list_compat("unresolved_calls")
425+
.unwrap_or_default();
426+
assert!(
427+
unresolved.iter().any(|c| c == "partial_close"),
428+
"callee should be parked for cross-file resolution, got {unresolved:?}"
429+
);
430+
}
217431
}

0 commit comments

Comments
 (0)