Skip to content

Commit cf959dd

Browse files
gadievronclaude
andcommitted
fix(parsers/c): stop same-(file,name) functions collapsing to one func_id
`func_id = "<file>:<name>"` carries no signature or preprocessor condition, and the store was an unguarded `self.functions[func_id] = ...` (keep-last). Two functions sharing (file, name) silently overwrote each other, corrupting the inventory AND the call graph: on tree-sitter, 14 real wasm_store.c implementations were replaced by their `#else` no-op stubs; C++ overloads collapsed onto one node so a call into one overload reached the other's callee. New `_store_function` disambiguates collision-only (unique names keep the exact `path:name` id — the hundreds of hardcoded id literals in the suite are untouched): - SAME signature (#ifdef/#else, ODR-dup): keep ONE node, prefer the larger body — the stub is shorter regardless of which arm tree-sitter emits first. - DIFFERENT signature (overload): both are real; fold a COLON-FREE signature discriminator into the func_id key only. `name` stays bare so the call-graph builder (which resolves by `name`) still finds both. Colon-free because downstream id splits rsplit on ':' (std::string -> std..string). Both store sites (process-function + lambda) route through the helper. Tests: tests/parsers/c/test_c_collision_discriminator.py (overloads survive, callees not conflated, #ifdef keeps larger body, ids colon-free, unique-name id byte-identical): $ pytest tests/parsers/c/test_c_collision_discriminator.py -q 5 passed in 0.47s $ pytest tests/parsers/c/ -q 71 passed in 5.71s Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7170285 commit cf959dd

2 files changed

Lines changed: 167 additions & 3 deletions

File tree

libs/openant-core/parsers/c/function_extractor.py

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,62 @@ def _get_parameters(self, node, source: bytes) -> List[str]:
230230
params.append('...')
231231
return params
232232

233+
@staticmethod
234+
def _signature_discriminator(parameters: List[str]) -> str:
235+
"""A deterministic, COLON-FREE signature string for the parameter list.
236+
237+
Colon-free is mandatory: the func_id is split on ':' downstream
238+
(`core/diff_filter.py`, `utilities/agentic_enhancer/repository_index.py`
239+
rsplit on ':' to recover the file), so any colon a C++ type carries
240+
(`std::string`, a ternary default arg) is mapped to '.' here.
241+
"""
242+
joined = ','.join(p.strip() for p in parameters)
243+
joined = ' '.join(joined.split()) # collapse newlines/runs of whitespace
244+
return joined.replace(':', '.')
245+
246+
@staticmethod
247+
def _same_signature(a: dict, b: dict) -> bool:
248+
"""Two definitions share a signature iff their parameter lists match.
249+
250+
Identical params => a redefinition of the SAME function under a different
251+
preprocessor branch (#ifdef/#else) or an ODR-duplicate. Different params
252+
=> a genuine C++ overload.
253+
"""
254+
return a.get('parameters', []) == b.get('parameters', [])
255+
256+
def _store_function(self, func_id: str, func_data: dict) -> None:
257+
"""Store a function, disambiguating same-(file,name) collisions instead
258+
of silently overwriting (Bug 1: #ifdef/#else stubs and C++ overloads
259+
collapsed onto one node — see reachability-bugs.md / bug-3482.md).
260+
261+
Collision-only: when func_id is free, store byte-identically (the 299
262+
hardcoded id literals across the suite depend on unique names keeping the
263+
plain `path:name` id). On a collision:
264+
- SAME signature -> keep ONE node, prefer the larger body. The #else
265+
stub is shorter than the real implementation regardless of which
266+
preprocessor arm tree-sitter emits first, so this is order- and
267+
direction-independent.
268+
- DIFFERENT signature -> a genuine overload; both are real. Mint a
269+
distinct key by folding the colon-free signature into the func_id.
270+
The `name` field is left bare (`area`, not `area(int,int)`) because
271+
the call-graph builder resolves calls by `name` (functions_by_name),
272+
so both overloads stay findable. (Contrast bug [39], which folds the
273+
template-arg discriminator into the NAME — correct there because a
274+
template call is written `g<int>`; an overload call is written bare.)
275+
"""
276+
existing = self.functions.get(func_id)
277+
if existing is None:
278+
self.functions[func_id] = func_data
279+
return
280+
if self._same_signature(existing, func_data):
281+
if len(func_data.get('code', '')) > len(existing.get('code', '')):
282+
self.functions[func_id] = func_data
283+
return
284+
overload_id = f"{func_id}({self._signature_discriminator(func_data.get('parameters', []))})"
285+
prior = self.functions.get(overload_id)
286+
if prior is None or len(func_data.get('code', '')) > len(prior.get('code', '')):
287+
self.functions[overload_id] = func_data
288+
233289
def _find_function_declarator(self, node):
234290
"""Find the function_declarator within a declarator tree."""
235291
if node.type == 'function_declarator':
@@ -520,7 +576,7 @@ def _process_function_node(self, node, source: bytes, relative_path: str,
520576
'class_name': class_name,
521577
}
522578

523-
self.functions[func_id] = func_data
579+
self._store_function(func_id, func_data)
524580
self.stats['total_functions'] += 1
525581

526582
if is_static:
@@ -559,7 +615,7 @@ def _process_lambda_declaration(self, node, source: bytes, relative_path: str,
559615
full_name = (namespace_prefix + name
560616
if namespace_prefix and '::' not in name else name)
561617
func_id = f"{relative_path}:{full_name}"
562-
self.functions[func_id] = {
618+
self._store_function(func_id, {
563619
'name': full_name,
564620
'file_path': relative_path,
565621
'start_line': node.start_point[0] + 1,
@@ -572,7 +628,7 @@ def _process_lambda_declaration(self, node, source: bytes, relative_path: str,
572628
'is_inline': False,
573629
'unit_type': 'lambda',
574630
'class_name': None,
575-
}
631+
})
576632
self.stats['total_functions'] += 1
577633
self.stats['by_type']['lambda'] = self.stats['by_type'].get('lambda', 0) + 1
578634

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
"""Bug 1 regression: same-(file,name) C/C++ functions must not collapse to one
2+
`func_id`. Two trigger families, two policies (see reachability-bugs.md Bug 1):
3+
4+
* C++ overloads — `area(int)` and `area(int,int)` are BOTH real. They must
5+
survive as two distinct func_ids, and each must keep its OWN call edges
6+
(the conflation bug routed a call to overload-1 into overload-2's callee).
7+
The discriminator is folded into the func_id KEY only; the `name` field
8+
stays bare (`area`) so name-based call resolution still finds both.
9+
* `#ifdef`/`#else` redefinition — same name, same signature, different body.
10+
Only one is the compiled implementation; keep a SINGLE node and prefer the
11+
larger body (the `#else` stub is shorter regardless of source order).
12+
13+
Plus a regression guard: a uniquely-named function's id is byte-identical to the
14+
pre-fix `path:name` form (the collision-only contract — non-colliding ids never
15+
change, so the 299 hardcoded id literals across the suite are untouched).
16+
"""
17+
import sys
18+
import tempfile
19+
from pathlib import Path
20+
21+
import pytest
22+
23+
_CORE = Path(__file__).resolve().parents[3]
24+
if str(_CORE) not in sys.path:
25+
sys.path.insert(0, str(_CORE))
26+
27+
pytest.importorskip("tree_sitter_c")
28+
29+
from parsers.c.function_extractor import FunctionExtractor # noqa: E402
30+
from parsers.c.call_graph_builder import CallGraphBuilder # noqa: E402
31+
32+
33+
def _extract(filename: str, source: str) -> dict:
34+
repo = Path(tempfile.mkdtemp()).resolve()
35+
fp = repo / filename
36+
fp.write_text(source)
37+
ex = FunctionExtractor(str(repo))
38+
ex.process_file(fp)
39+
return ex.functions
40+
41+
42+
def test_cpp_overloads_both_survive():
43+
fns = _extract("ovl.cpp",
44+
"int area(int s){return s*s;}\n"
45+
"int area(int w,int h){return w*h;}\n")
46+
areas = [fid for fid, d in fns.items() if d["name"] == "area"]
47+
assert len(areas) == 2, f"both overloads must survive; got {areas}"
48+
assert len({fid for fid in areas}) == 2, "overload func_ids must be distinct"
49+
50+
51+
def test_overload_ids_are_colon_free_in_name_part():
52+
# The discriminator must not introduce a colon into the name part of the id
53+
# (downstream diff_filter / repository_index rsplit on ':').
54+
fns = _extract("ovl.cpp",
55+
"int f(int a){return a;}\n"
56+
"int f(const char* s,int n){return n;}\n")
57+
for fid, d in fns.items():
58+
if d["name"] != "f":
59+
continue
60+
name_part = fid.split(":", 1)[1] # everything after the path colon
61+
assert ":" not in name_part, f"colon leaked into discriminated id: {fid!r}"
62+
63+
64+
def test_ifdef_else_keeps_larger_body_not_stub():
65+
# tree-sitter parses BOTH preprocessor arms; the #else stub comes last and,
66+
# pre-fix, overwrote the real implementation. Prefer the larger body.
67+
src = (
68+
"#ifdef FEATURE\n"
69+
"int run(int n){int t=0;for(int i=0;i<n;i++){t+=i*i;}return t;}\n"
70+
"#else\n"
71+
"int run(int n){(void)n;return 0;}\n"
72+
"#endif\n"
73+
)
74+
fns = _extract("if.c", src)
75+
runs = [d for d in fns.values() if d["name"] == "run"]
76+
assert len(runs) == 1, f"#ifdef/#else same-sig must keep ONE node; got {len(runs)}"
77+
assert "for" in runs[0]["code"], f"kept the stub, not the real impl: {runs[0]['code']!r}"
78+
79+
80+
def test_overload_callees_not_conflated():
81+
# The real consequence of the bug: a call into one overload must reach ITS
82+
# callee, not the other overload's. Mirrors /tmp/bugrepro/conf.cpp.
83+
src = (
84+
"void alpha(){}\n"
85+
"void beta(){}\n"
86+
"int pick(int a){alpha();return a;}\n"
87+
"int pick(int a,int b){beta();return a+b;}\n"
88+
)
89+
fns = _extract("conf.cpp", src)
90+
b = CallGraphBuilder({"functions": fns, "includes": {}})
91+
b.build_call_graph()
92+
out = b.build() if hasattr(b, "build") else b.export()
93+
cg = out.get("call_graph", {})
94+
pick_ids = [fid for fid, d in fns.items() if d["name"] == "pick"]
95+
assert len(pick_ids) == 2, f"both pick overloads must exist; got {pick_ids}"
96+
all_pick_edges = {e for fid in pick_ids for e in cg.get(fid, [])}
97+
alpha_id = next(fid for fid, d in fns.items() if d["name"] == "alpha")
98+
beta_id = next(fid for fid, d in fns.items() if d["name"] == "beta")
99+
assert alpha_id in all_pick_edges, f"alpha() edge lost (conflation): {all_pick_edges}"
100+
assert beta_id in all_pick_edges, f"beta() edge lost (conflation): {all_pick_edges}"
101+
102+
103+
def test_unique_name_id_is_byte_identical():
104+
# Collision-only contract: a uniquely-named function keeps the exact
105+
# `path:name` id — no discriminator appended when there is no collision.
106+
fns = _extract("u.c", "int solo(int x){return x+1;}\n")
107+
ids = [fid for fid, d in fns.items() if d["name"] == "solo"]
108+
assert ids == ["u.c:solo"], f"unique-name id changed (would break 299 literals): {ids}"

0 commit comments

Comments
 (0)