fix(php): dedup typed-property catch-all double-match in captures (U2)

The untyped @declaration.variable catch-all pattern in query.ts (lines
101-103) has no `type:` constraint and tree-sitter therefore also
matches it against typed property declarations - emitting a second
capture for the same property_declaration anchor. Graph-level def-id
collision currently masks the duplicate at the node-emit layer, but
the catch-all capture still flows through scope-binding and name-keyed
registries with a `$`-prefixed name that the typed branch's `$`-strip
never normalizes - a known vector for receiver-binding lookup pollution.

The two tree-sitter patterns produce separate rawMatches entries with
separate `grouped` maps, so the dedup must be cross-match. Pre-scan
rawMatches once to collect anchor node IDs already covered by
@declaration.property, then skip @declaration.variable matches whose
anchor is in that set.

Adds php-typed-property-dedup fixture covering typed property,
constructor-promoted typed parameter, and a mixed-untyped declaration
to verify the catch-all path still fires for untyped properties.
This commit is contained in:
Gergo Magyar 2026-05-12 12:21:46 +01:00
parent 29e45f1971
commit c82fa8369a
5 changed files with 128 additions and 0 deletions

View file

@ -75,6 +75,29 @@ export function emitPhpScopeCaptures(
const rawMatches = getPhpScopeQuery().matches(tree.rootNode);
const out: CaptureMatch[] = [];
// Pre-scan: collect anchor node IDs of property_declaration nodes already
// matched by the typed @declaration.property pattern (query.ts ~lines 95–98).
// The untyped @declaration.variable catch-all (query.ts ~lines 101–103) is
// intentionally loose — it has no `type:` constraint, so tree-sitter also
// matches it against typed property declarations and emits a second capture
// for the same property_declaration anchor. Graph-level def-id collision
// currently masks the duplicate at the node-emit layer, but the catch-all
// capture still flows through scope-binding / name-keyed registries with a
// `$`-prefixed name that the typed branch's `$`-strip never normalizes —
// a known vector for receiver-binding lookup pollution. The two patterns
// produce separate rawMatches entries with separate `grouped` maps, so the
// dedup has to be cross-match: build the set here, then skip
// @declaration.variable matches whose anchor is in it (loop below).
const typedPropertyAnchorIds = new Set<number>();
for (const m of rawMatches) {
for (const c of m.captures) {
if (c.name === 'declaration.property') {
typedPropertyAnchorIds.add(c.node.id);
break;
}
}
}
for (const m of rawMatches) {
// Group captures by their tag name. Tree-sitter strips the leading
// `@`; we put it back so the central extractor's prefix lookups work.
@ -85,6 +108,14 @@ export function emitPhpScopeCaptures(
}
if (Object.keys(grouped).length === 0) continue;
// Cross-match dedup for the typed-property double-match described above:
// skip @declaration.variable matches whose anchor was already captured as
// @declaration.property in an earlier match.
if (grouped['@declaration.variable'] !== undefined) {
const varCap = m.captures.find((c) => c.name === 'declaration.variable');
if (varCap !== undefined && typedPropertyAnchorIds.has(varCap.node.id)) continue;
}
// Normalize PHP property declarations: strip leading `$` from
// `@declaration.name` for @declaration.property matches. PHP stores
// field names WITHOUT the `$` sigil in the graph so that member access

View file

@ -0,0 +1,8 @@
<?php
namespace App\Models;
class UserRepo
{
public function save(): void {}
}

View file

@ -0,0 +1,23 @@
<?php
namespace App\Services;
use App\Models\UserRepo;
class Mixed
{
// Typed property: must emit exactly one Property def named `repo`,
// zero stray Variable defs.
private UserRepo $repo;
// Untyped property: must emit exactly one (legitimate) catch-all def
// for `$id`, and zero Property defs for it.
public $id;
// Constructor-promoted typed parameter: tree-sitter routes these
// through the same `property_element` shape, so the dedup must also
// suppress the stray Variable here.
public function __construct(private UserRepo $promotedRepo)
{
}
}

View file

@ -0,0 +1,5 @@
{
"autoload": {
"psr-4": { "App\\": "app/" }
}
}

View file

@ -2093,3 +2093,64 @@ describe('PHP MRO arity-mismatch fallthrough', () => {
);
});
});
// ---------------------------------------------------------------------------
// @declaration.variable double-match dedup on typed properties.
// Pre-fix, the catch-all property pattern in query.ts (no `type:` constraint)
// also matched typed property declarations and emitted a stray Variable def
// alongside the legitimate Property def. captures.ts now pre-scans rawMatches
// for @declaration.property anchors and suppresses the duplicate.
// ---------------------------------------------------------------------------
describe('PHP typed-property double-match dedup', () => {
let result: PipelineResult;
beforeAll(async () => {
result = await runPipelineFromRepo(path.join(FIXTURES, 'php-typed-property-dedup'), () => {});
}, 60000);
it('detects the Mixed class', () => {
expect(getNodesByLabel(result, 'Class')).toContain('Mixed');
});
it('emits exactly one Property def for the typed property `$repo`', () => {
const properties = getNodesByLabel(result, 'Property');
expect(properties.filter((n) => n === 'repo').length).toBe(1);
});
it('emits exactly one Property def for the constructor-promoted typed `$promotedRepo`', () => {
const properties = getNodesByLabel(result, 'Property');
expect(properties.filter((n) => n === 'promotedRepo').length).toBe(1);
});
it('emits zero stray Variable defs for typed property and promoted typed parameter', () => {
// Pre-fix: a Variable def named `$repo` and `$promotedRepo` (no `$` strip)
// would slip through the catch-all pattern. Post-fix: zero.
const variables = getNodesByLabel(result, 'Variable');
expect(variables.filter((n) => n === '$repo' || n === 'repo').length).toBe(0);
expect(variables.filter((n) => n === '$promotedRepo' || n === 'promotedRepo').length).toBe(0);
});
it('untyped property `$id` still emits its catch-all Property def (regression check)', () => {
// The untyped catch-all @declaration.variable pattern is the legitimate
// path for `public $id;`. Make sure the cross-match dedup does not
// over-suppress untyped declarations — they have no @declaration.property
// sibling, so their anchor is not in the typedPropertyAnchorIds set.
const properties = getNodesByLabel(result, 'Property');
expect(properties.filter((n) => n === 'id').length).toBe(1);
});
it('no `$`-prefixed Property or Variable defs leak from typed declarations', () => {
// The catch-all branch does NOT run the `$`-strip normalization, so any
// def it produces for a typed property carries a `$`-prefixed name —
// a known receiver-binding lookup pollution vector. Post-fix the
// catch-all is suppressed for typed property_declaration anchors, so
// no `$repo` / `$promotedRepo` def should appear at any label.
for (const n of result.graph.iterNodes()) {
const name = String(n.properties.name);
if (name === '$repo' || name === '$promotedRepo') {
throw new Error(`leaked $-prefixed def: ${n.label}|${name}|${n.id}`);
}
}
});
});