fix(php): keep the $ sigil on closure-binding nodes so locals stop colliding (#2693)

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:<file>: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:<file>:$save and the function stays Function:<file>: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 <id>` 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.
This commit is contained in:
Gergo Magyar 2026-07-26 06:21:20 +00:00
parent 5645d6592c
commit aeb559b25e
3 changed files with 47 additions and 6 deletions

View file

@ -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.

View file

@ -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:<file>: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
`;

View file

@ -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:<file>: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',
'<?php\nfunction save($x) { return $x; }\n' +
'function run() {\n $save = fn($x) => $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',
'<?php\nfunction save($x) { return $x; }\nfunction run() { return save(1); }\n',
);
expect(targets).toEqual(['Function:f.php:save']);
});
it('JavaScript: a `var` closure binding is a Function, like const/let', async () => {
// `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