From aeb559b25eed85cb674e76d5a22bfb5b74ee0320 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sun, 26 Jul 2026 06:21:20 +0000 Subject: [PATCH] fix(php): keep the $ sigil on closure-binding nodes so locals stop colliding (#2693) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PHP local closure whose name matched a file-level function got NO edge at all: function save($x) { return $x; } function run() { $save = fn($x) => $x * 2; return $save(1); // no CALLS edge } Both minted the id Function::save, so the closure's node was swallowed by the function's and the positional join found nothing at the binding's line. The fix is PHP's own semantics rather than a change to node identity across the graph. PHP holds variables and functions in SEPARATE namespaces — $save and save() cannot collide in the language — and the sigil is what separates them. Dropping it was the bug. The node rule now captures the whole variable_name, so the closure is Function::$save and the function stays Function::save. languages/php/query.ts already keeps the sigil on property declarations for the same reason, so this makes the two consistent. The positional join normalises a leading $/@ on both sides, matching what the scope layer and the callable-flow synthesizer already do, so the binding still matches its own declaration while its NODE stays distinct. local closure + same-named function -> Function:c.php:$save (the closure) calling the real function -> Function:f.php:save (unchanged) plain $max = 10 -> no node, no edge (unchanged) WHAT THIS DOES NOT FIX. The general problem is wider than PHP: GitNexus node ids are file-scoped, so a function-local symbol and a file-level one with the same name collapse in TypeScript, Python and Dart too, and Java/C# only escape by qualifying on the enclosing CLASS (so two same-named locals in different methods still collide). SCIP solves it with a separate `local ` keyspace that is document-scoped and never globally addressable. That is issue #2699 — it changes persisted ids for every function-local symbol and needs its own invalidation, so it is not bundled here. PHP is fixed on its own merits: the sigil belongs in the identity regardless of how locals are eventually scoped. --- .../passes/callable-value-flow.ts | 13 +++++++-- .../src/core/ingestion/tree-sitter-queries.ts | 12 ++++++-- .../closure-binding-labels.test.ts | 28 +++++++++++++++++-- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts index 7d6d35792..f0240a2d3 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts @@ -799,6 +799,15 @@ export function buildGraphTargetIndex( return out; } +/** + * Leading sigils are part of a name in some grammars and stripped in others: + * PHP keeps `$` on a variable_name node (deliberately — it is what separates + * PHP's variable and function namespaces, so `$save` does not collide with + * `save()`), while the scope layer and the callable-flow synthesizer both + * normalise it away. The positional join has to see both sides the same way. + */ +const withoutSigil = (name: string): string => name.replace(/^[$@]+/, ''); + /** * `file\0line\0name` for a value binding, matching the callable-node key built * in `buildGraphCallableIndexes`. Definition lines come from the def id and are @@ -809,7 +818,7 @@ function valueBindingPositionKey(def: SymbolDefinition): string | undefined { const line = def.nodeId.match(/#(\d+):(\d+):/)?.[1]; const name = simpleQualifiedName(def); if (line === undefined || name === undefined) return undefined; - return `${def.filePath}\0${line}\0${name}`; + return `${def.filePath}\0${line}\0${withoutSigil(name)}`; } /** @@ -980,7 +989,7 @@ function buildGraphCallableIndexes(graph: KnowledgeGraph): GraphCallableIndexes continue; } const oneBasedLine = zeroBasedLine + 1; - const positionKey = `${filePath}\0${oneBasedLine}\0${name}`; + const positionKey = `${filePath}\0${oneBasedLine}\0${withoutSigil(name)}`; const existing = callableByPosition.get(positionKey); // First wins would be order-dependent; mark the collision instead so an // ambiguous position admits nothing rather than something arbitrary. diff --git a/gitnexus/src/core/ingestion/tree-sitter-queries.ts b/gitnexus/src/core/ingestion/tree-sitter-queries.ts index 51504853b..380cb8656 100644 --- a/gitnexus/src/core/ingestion/tree-sitter-queries.ts +++ b/gitnexus/src/core/ingestion/tree-sitter-queries.ts @@ -1429,11 +1429,19 @@ export const PHP_QUERIES = ` ; line and name), which is what makes $handler(1) resolve. Overlap with the value ; rules above is collapsed by the parse-worker dedup, which ranks callable ; highest (#2687). +; Captures the whole variable_name, so the node keeps PHP's \`$\` sigil. That is +; not cosmetic: PHP holds variables and functions in SEPARATE namespaces, so +; \`$save\` and \`save()\` can never collide in the language — but dropping the +; sigil made both mint the id Function::save, and the local closure was +; swallowed by the function's node (no node, therefore no edge). The property +; rules in languages/php/query.ts already keep the sigil for the same reason. +; The positional join normalises leading sigils, so the binding still matches +; its own declaration. (assignment_expression - left: (variable_name (name) @name) + left: (variable_name) @name right: (arrow_function)) @definition.function (assignment_expression - left: (variable_name (name) @name) + left: (variable_name) @name right: (anonymous_function)) @definition.function `; diff --git a/gitnexus/test/integration/closure-binding-labels.test.ts b/gitnexus/test/integration/closure-binding-labels.test.ts index bbad6f8cc..b5ba0db3a 100644 --- a/gitnexus/test/integration/closure-binding-labels.test.ts +++ b/gitnexus/test/integration/closure-binding-labels.test.ts @@ -400,7 +400,7 @@ describeIfWorkerBuilt('closure bindings resolve in the remaining languages (#269 'function caller() {\n global $handler;\n return $handler(1);\n}\n', ); - expect(targets).toEqual(['Function:a.php:handler']); + expect(targets).toEqual(['Function:a.php:$handler']); }); it('PHP: an anonymous function binding resolves too', async () => { @@ -410,7 +410,7 @@ describeIfWorkerBuilt('closure bindings resolve in the remaining languages (#269 'function caller() {\n global $handler;\n return $handler(1);\n}\n', ); - expect(targets).toEqual(['Function:b.php:handler']); + expect(targets).toEqual(['Function:b.php:$handler']); }); it('TypeScript: a class-field arrow is a callable member, like Kotlin', async () => { @@ -439,6 +439,30 @@ describeIfWorkerBuilt('closure bindings resolve in the remaining languages (#269 expect(targets).toEqual(['Method:C.js:C.handler']); }); + it('PHP: a local closure sharing a name with a function resolves to the CLOSURE', async () => { + // PHP keeps variables and functions in SEPARATE namespaces, so `$save` and + // `save()` cannot collide in the language. Dropping the `$` made both mint + // Function::save, so the closure was swallowed by the function's node + // and the call got NO edge at all. Keeping the sigil restores PHP's own + // separation; the positional join normalises it when matching. + const targets = await callTargetsFor( + 'c.php', + ' $x * 2;\n return $save(1);\n}\n', + ); + + expect(targets).toEqual(['Function:c.php:$save']); + }); + + it('PHP: calling the real function still resolves to the function', async () => { + const targets = await callTargetsFor( + 'f.php', + ' { // `var` is a different grammar node than const/let, so it kept a Variable // label — and the CALLS edge that resolved through the declaration route