Skip to content

feat(php): migrate PHP to scope-based resolution model (#938) [supersedes #1124]#1497

Merged
magyargergo merged 36 commits into
abhigyanpatwari:mainfrom
magyargergo:lang/php-migration-v2
May 12, 2026
Merged

feat(php): migrate PHP to scope-based resolution model (#938) [supersedes #1124]#1497
magyargergo merged 36 commits into
abhigyanpatwari:mainfrom
magyargergo:lang/php-migration-v2

Conversation

@magyargergo

Copy link
Copy Markdown
Collaborator

Summary

This PR supersedes #1124 (stale, conflicting with main). Rebased cleanly onto current origin/main (10+ commits ahead of the original merge-base), preserving all new hooks added by Go (#1302), range-binding, and structural-interface rings.

RFC #909 Ring 3 LANG-php — third language migrated to the scope-based resolver pipeline after Python and C#.

What's in this PR

PHP module (gitnexus/src/core/ingestion/languages/php/): 13 new files

  • scope-resolver.tsScopeResolver implementation: PHP MRO with trait-use via IMPLEMENTS edges, namespace-sibling population, arity compatibility, merge-binding strategy, allowGlobalFreeCallFallback for unimported same-namespace calls
  • query.ts — tree-sitter-php patterns for all PHP constructs (classes, traits, interfaces, enums, namespaces, use/use function/use const, member/free/nullsafe/static calls, PHPDoc)
  • captures.tsemitPhpScopeCaptures: import decomposition, receiver-binding synthesis, arity metadata, PHPDoc extraction
  • interpret.tsinterpretPhpImport / interpretPhpTypeBinding
  • import-decomposer.ts — 1:N decomposer for all use declaration forms including grouped use Foo\{A, B as C}
  • import-target.ts — PSR-4 + composer.json resolution adapter
  • namespace-siblings.ts — cross-file same-namespace visibility (mirrors C# implementation)
  • arity-metadata.ts, arity.ts — PHP arity computation and arityCompatibility
  • merge-bindings.ts — binding precedence: local > import/namespace > wildcard
  • receiver-binding.ts$this / parent type-binding synthesis
  • simple-hooks.tsbindingScopeFor hoisting for return types and constructor-promoted properties
  • cache-stats.ts — tree-sitter parse cache metrics

Registry wiring (2 files, minimal):

  • pipeline/registry.ts — adds phpScopeResolver alongside existing Go/Python/C#/TS entries
  • registry-primary-flag.ts — adds PHP to MIGRATED_LANGUAGES

Shared pipeline improvements (PHP-motivated, safe for all languages):

  • graph-bridge/node-lookup.ts — adds 'Trait' to isLinkableLabel so PHP (and Rust) MRO builders can bridge trait defs to graph node IDs via IMPLEMENTS edges; Go is unaffected
  • passes/free-call-fallback.ts — adds arity-narrowing to pickUniqueGlobalCallable: when multiple global candidates share a name, narrows by parameterCount/requiredParameterCount before giving up; fixes registry-primary languages where the semantic model is not populated by the legacy call-processor

P0 staleness fix (Kieran review finding)

The original PR #1124 deleted 6 hooks from ScopeResolver that were added on main by Go (#1302) and other rings (expandsWildcardTo, populateWorkspaceOwners, detectInterfaceImplementations, mirrorNamespaceTypeBindings, populateRangeBindings, allowGlobalFreeCallFallback). This PR restores all shared-pipeline files to origin/main and applies PHP changes on top.

Known limitation (follow-up)

save($entity: mixed)->getId() — member calls on mixed-typed parameters have no TypeRef in scope and cannot be resolved by the current contract. The stale PR used a non-contract emitUnresolvedReceiverEdges hook. The 1 affected test is skipped in registry-primary mode with a comment; a postResolutionFallback contract hook is tracked as a follow-up to RFC #909 Ring 3.

Test results

Suite Flag=1 (scope-resolver) Flag=0 (legacy)
PHP 159/160 passed, 1 skipped 160/160 passed
C# 788/788
Python included above
TypeScript included above
Go included above
Typecheck clean (npx tsc --noEmit)

Go resolver tests are fully green — no regressions from the shared pipeline changes.

Files changed

20 files: 13 new PHP module files, php.ts (modified), registry.ts, registry-primary-flag.ts, node-lookup.ts, free-call-fallback.ts, php.test.ts.

Closes #938 (RFC #909 Ring 3 LANG-php)

…ri#938)

RFC abhigyanpatwari#909 Ring 3 LANG-php. Third language after Python and C# to be
migrated to the scope-based resolver pipeline.

PHP module (gitnexus/src/core/ingestion/languages/php/):
- scope-resolver.ts: ScopeResolver implementation with PHP-specific
  MRO builder (traits via IMPLEMENTS edges), namespace-sibling
  population, arity compatibility, and merge-binding strategy
- query.ts: tree-sitter-php .scm patterns for all PHP constructs
- captures.ts: emitPhpScopeCaptures with import decomposition,
  receiver-binding synthesis, arity metadata, and PHPDoc extraction
- interpret.ts: interpretPhpImport / interpretPhpTypeBinding
- import-decomposer.ts: 1:N decomposer for use/use function/use const
  including grouped use declarations
- import-target.ts: PSR-4 + composer.json resolution adapter
- namespace-siblings.ts: cross-file same-namespace visibility
- arity-metadata.ts, arity.ts: PHP arity computation and compatibility
- merge-bindings.ts: PHP binding precedence (local > import > wildcard)
- receiver-binding.ts: $this / parent type-binding synthesis
- simple-hooks.ts: bindingScopeFor hoisting for return types and
  constructor-promoted properties
- cache-stats.ts: tree-sitter parse cache metrics

Registry wiring:
- pipeline/registry.ts: add phpScopeResolver alongside existing Go entry
- registry-primary-flag.ts: add PHP to MIGRATED_LANGUAGES

Shared pipeline improvements (PHP-motivated, safe for all languages):
- graph-bridge/node-lookup.ts: add Trait to isLinkableLabel so PHP
  and Rust MRO builders can bridge trait defs to graph node ids
- passes/free-call-fallback.ts: add arity-narrowing to
  pickUniqueGlobalCallable; when multiple global candidates exist,
  narrow by parameterCount/requiredParameterCount before giving up;
  fixes registry-primary languages where the semantic model is empty

Test results:
- 159/160 PHP tests pass with REGISTRY_PRIMARY_PHP=1 (scope-resolver)
- 160/160 PHP tests pass with REGISTRY_PRIMARY_PHP=0 (legacy path)
- 788/788 C#/Python/TypeScript/Go tests green
- Typecheck clean
- 1 test skipped in registry-primary mode: save->getId on mixed-typed
  receiver; requires a postResolutionFallback hook in the contract
  (follow-up to RFC abhigyanpatwari#909 Ring 3)
@vercel

vercel Bot commented May 11, 2026

Copy link
Copy Markdown

@magyargergo is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

Comment thread gitnexus/src/core/ingestion/languages/php/scope-resolver.ts Fixed
# Conflicts:
#	gitnexus/src/core/ingestion/registry-primary-flag.ts
#	gitnexus/src/core/ingestion/scope-resolution/passes/free-call-fallback.ts
#	gitnexus/src/core/ingestion/scope-resolution/pipeline/registry.ts
…mic languages

Adds an optional post-resolution pass on the ScopeResolver contract:
when a member-call receiver cannot be typed by the scope chain (no
TypeRef), the language may emit CALLS edges via a workspace-wide
unique-name lookup. Runs after emitReceiverBoundCalls and before
emitFreeCallFallback, gated per-language.

PHP wires the hook to recover member calls on mixed/untyped parameters
(e.g. save(mixed $entity) calling $entity->getId()), restoring parity
with the legacy DAG. Re-enables the previously skipped save → getId
test; PHP suite is now 160/160 with no skips in both flag states.
@github-actions

github-actions Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
8808 8807 0 1 409s

✅ All 8807 tests passed

1 test(s) skipped — expand for details
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 77.64% 27127/34936 N/A% 🟢 ███████████████░░░░░
Branches 66.09% 17104/25878 N/A% 🟢 █████████████░░░░░░░
Functions 82.37% 2735/3320 N/A% 🟢 ████████████████░░░░
Lines 80.81% 24515/30336 N/A% 🟢 ████████████████░░░░

📋 View full run · Generated by CI

@magyargergo magyargergo self-assigned this May 11, 2026
…production review

Fixes four PHP-semantic defects identified by the production-readiness
review. The bar is "graph edges must reflect what PHP actually does",
not "matches legacy DAG" — parity with the legacy DAG is not a
correctness criterion when the legacy DAG itself has the same defect.

U1: Variadic requiredParameterCount
  arity-metadata.ts:49 now subtracts the variadic slot from required
  count: total - optionalCount - (hasVariadic ? 1 : 0). f(int $req,
  ...$rest) requires 1 arg, not undefined. Adds overload-narrowing and
  lookup-core arity-filter changes so resolvers actually drop candidates
  that are definitively arity-incompatible (was silently rescuing
  empty filter sets even when bounds were known).

U2: True transitive trait MRO
  buildPhpMro now uses a BFS worklist (collectTransitiveTraits) to
  flatten the trait-of-trait DAG to fixpoint instead of expanding one
  level. Adds (trait_declaration body (use_declaration ...)) heritage
  query so trait-uses-trait IMPLEMENTS edges are emitted at all. Fixes
  3+ level trait chains silently dropping methods.

U3: parent:: bypasses composed traits
  ScopeResolver gains optional buildExtendsOnlyMro hook; PHP returns
  the unaugmented EXTENDS-only chain via buildPhpExtendsOnlyMro.
  MethodDispatchIndex gains optional extendsOnlyMroFor accessor wired
  through buildPopulatedMethodDispatch. Super-branch dispatch in
  receiver-bound-calls now walks extendsOnlyMroFor when present, so
  parent::method() routes to the parent class even when a composed
  trait shadows the same name. Other languages leave the hook
  undefined and fall back to mroFor unchanged.

U4: Namespace-aware free-call fallback
  ScopeResolver gains optional isCallableVisibleFromCaller predicate;
  pickUniqueGlobalCallable applies it to filter cross-namespace
  candidates the caller can't reach without a use-function import.
  PHP impl checks same-namespace OR explicit use-function presence,
  using a side-channel namespace cache populated by
  populatePhpNamespaceSiblings. Fixes the legacy DAG's namespace-
  blind false-positive emissions. Existing php-calls fixture
  updated: write_audit now correctly imports its targets via
  use-function rather than relying on the false-positive name-only
  match.

Tests:
  - 175/175 PHP both flag states (REGISTRY_PRIMARY_PHP=0 and =1)
  - 794/794 C#/Python/TypeScript/Go/C (no cross-language regression)
  - typecheck clean
  - 4 new fixtures: php-variadic-arity-minimum, php-transitive-traits,
    php-parent-vs-trait, php-namespace-fallback-isolation

Plan: docs/plans/2026-05-11-001-fix-php-resolver-semantic-defects-plan.md
These were stripped by the PHP migration commit (69786b1) as
collateral damage during the rebase, but they have nothing to do
with scope resolution. They feed cluster/community detection and
Laravel route-attribute analysis at the higher analysis layers.

Restored verbatim from 69786b1~1:
- entryPointPatterns: 17 PHP idioms (Controller, handle/execute/
  boot/register, REST verbs, Service/Repository, find/save/delete)
- astFrameworkPatterns: Laravel routing block with 3.0x multiplier
  for Route::get / Route::post / #[Route(...)] attribute, etc.

PHP 175/175 still green; typecheck clean.
CI scope-parity / php parity was failing under REGISTRY_PRIMARY_PHP=0 on
three assertions added by commit af9af4a (U1 arity-narrowing, U3 trait
shadows parent). Per that commit's stance — "parity with the legacy DAG
is not a correctness criterion when the legacy DAG itself has the same
defect" — backporting these fixes to the legacy resolver is out of scope.

Adopt the existing sibling pattern (csharp/typescript/python use the
same helper):
- php.test.ts: switch to `const it = createResolverParityIt('php')` so
  expected-failure assertions skip under legacy mode.
- helpers.ts: register three test names in
  LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.php with the rationale.

Verified locally: 172 passed + 3 skipped under REGISTRY_PRIMARY_PHP=0,
175 passed under REGISTRY_PRIMARY_PHP=1, tsc clean.
…npatwari#1497)

Fixture php-fqn-cross-namespace declares two `User` classes — `App\Models\User`
and `App\Other\User` — and a Service.php that imports the Models simple-name
via `use App\Models\User` but uses a fully-qualified `\App\Other\User` in a
parameter annotation. Adds three assertions:
- Both User classes are detected as Class nodes in distinct files.
- The FQN-parameter method's `$u->record()` resolves to app/Other/User.php
  (currently FAILS — Codex review finding 1).
- The simple-name-parameter method's `$u->record()` resolves to
  app/Models/User.php (control; still passes pre-fix).

This is the test-first gate Codex required: "Block merge until the PHP FQN
fixture fails before the fix and passes after it." Verified failing on
HEAD before U2-U3 land the fix.

Also sweeps every `toBeGreaterThanOrEqual` in php.test.ts to exact
`.toBe(N)` per DoD.md §2.7 ("avoid bounds-only assertions that mask
regressions"). Touched 10 assertions across the U2/U3 trait MRO blocks,
the namespace-aware free-call fallback block, and the new FQN block.
All previously-green tests stay green with exact counts — no latent
over-emission bugs were hidden by `>= 1`.

The ambiguous-overload finding (Codex finding 2) cannot be exercised by a
PHP integration fixture — PHP does not support method overloading, so
`model.methods.lookupAllByOwner` returns at most 1 entry per (class, name)
pair. That regression test ships as a unit test against
`pickImplicitThisOverload` in U4 once the function is exported.

Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U1)
Stop collapsing `\App\Models\User` to `User` in normalizePhpType (step 6).
Canonicalize the leading backslash off and preserve the qualified path
on TypeRef.rawName so downstream PHP receiver resolution can distinguish
the FQN target from a same-simple-name class reachable via `use`.

- Step 6 rewritten: `\App\Models\User` → `App\Models\User`,
  `App\Models\User` → unchanged, `User` → unchanged.
- Final validation regex relaxed from /^\w+$/ to /^\w+(?:\\w+)*$/ to
  accept qualified PHP identifiers while still rejecting empty segments,
  trailing backslashes, and non-identifier characters.
- Step 5 (single-arg generic strip) already passes qualified inner types
  through via its existing /^\w[\w\]*/ pattern — no change needed.

U3 will wire the qualified-name lookup path; this commit alone is a
no-op for resolution (the lookup chain still keys on simple names).
PHP suite remains green at 177 passed; the only failing test is the
intentional FQN regression from U1.

Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U2)
…ri#1497 finding 1)

Extend populatePhpNamespaceSiblings with Step 3b: for every PHP file's
Module scope, inject a binding entry keyed by the fully-qualified
class name (`App\Models\User`) for every class-like def in the
workspace. This routes FQN-receivers like `\App\Other\User` to the
exact namespace-qualified class regardless of which simple-name `User`
the caller's `use` imports shadowed.

Why module-scope bindingAugmentations instead of mutating def.qualifiedName:
the shared QualifiedNameIndex consumes def.qualifiedName at finalize time,
but PHP class defs need to remain keyed by simple name throughout the rest
of the pipeline (MRO, method-dispatch-index, namespace-siblings step 3).
An earlier attempt to rewrite def.qualifiedName to namespace-prefixed form
cascaded into 32 unrelated test failures across receiver-binding, MRO, and
heritage. The bindingAugmentations channel is purpose-built for adding
post-finalize visibility without mutating shared semantic state, and
`findClassBindingInScope`'s scope-chain walk already consumes it via
`lookupBindingsAt` — wiring is zero-touch.

Cost: O(PHP files × class-like defs) augmentation entries. Typical PHP
project: hundreds × hundreds = bounded.

Verified locally:
- Registry-primary: 178/178 PHP tests pass (including the new FQN regression).
- Legacy DAG: 174 passed, 3 existing skips, 1 failure (the FQN test, expected
  — legacy DAG has no namespace augmentation channel; U5 registers it in
  LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.php).
- Cross-language: 794/794 (C#, Python, TypeScript, Go, C) pass. The PHP-only
  augmentation does not touch shared resolution code.
- tsc clean.

Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U3)
…erload

Codex PR abhigyanpatwari#1497 review, finding 2: pickImplicitThisOverload returned
candidates[0] after narrowOverloadCandidates without checking
uniqueness. When two same-name methods on the same class shared
identical arity and the call site lacked disambiguating argument-type
info, narrowing left both compatible and the resolver emitted a
high-confidence CALLS edge whose target depended on registration
order rather than a defensible resolution.

Tighten the picker:
- candidates.length === 1 -> return candidates[0] (unchanged for the
  unambiguous case)
- candidates.length !== 1 (zero or multiple) -> return undefined
  (call left unresolved; no edge emitted)

This mirrors pickUniqueGlobalCallable's existing pattern in the same
file.

Export pickImplicitThisOverload so a unit test can exercise it with
synthetic stubs — PHP cannot produce the multi-overload failure shape
(no method overloading in PHP), and C# integration coverage would
entangle the unit's contract with the broader C# resolver. The unit
test pins five cases: sole overload, narrowing-disambiguated, the
ambiguous multi-candidate case (the bug regression), no-match, and
no-enclosing-class.

Verified: 972/972 across PHP + C# + Python + TypeScript + Go + C
resolver suites; no regression in any language. tsc clean.

Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U4)
…anpatwari#1497)

After U3 (FQN-keyed bindingAugmentations) lands, the FQN regression
passes under REGISTRY_PRIMARY_PHP=1 but still fails under =0. The
legacy DAG resolves receiver types via simple-name workspace lookup
and has no namespace-prefixed binding channel, so it cannot
distinguish `\App\Other\User` from a same-simple-name class
reachable via `use`. Per the established convention from commit
af9af4a ("parity with the legacy DAG is not a correctness
criterion when the legacy DAG itself has the same defect"), the
test registers in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.php
rather than blocking the migration.

The other two assertions in the new describe block (class detection
and the saveLocal control) pass in both modes and stay green.

Verified:
- REGISTRY_PRIMARY_PHP=0: 174 passed, 4 skipped (3 existing + 1 new).
- REGISTRY_PRIMARY_PHP=1: 178 passed.
- C# + Python + TypeScript + Go + C resolver matrix: 794 passed.
- pickImplicitThisOverload unit tests: 5 passed.
- resolver-parity-expected-failures unit test: 3 passed.

Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U5)
Comment on lines +300 to +304
if (
narrowOverloadCandidates([memberDef], site.arity, site.argumentTypes).length === 0
) {
memberDef = undefined;
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: MRO chain incorrectly falls through to parent on arity mismatch

The MRO chain for Case 2 is ordered most-derived-first: [Child, Parent, Grandparent, ...]. When Child::method() is found but arity-incompatible with the call site, this code clears memberDef and continues — walking to Parent::method() and potentially emitting a CALLS edge there if that ancestor happens to be arity-compatible.

This is incorrect. PHP always dispatches to the most-derived override. If the call is arity-incompatible with Child::method, PHP throws ArgumentCountError at runtime — it never silently redirects to an ancestor method. The resolver should emit no edge for this call.

The comment "a subclass may shadow with a different arity" is also inverted: the chain walks up toward ancestors (less-derived), not down toward subclasses.

The fix is to break instead of continue, so that discovering an arity-incompatible most-derived definition terminates the chain walk and emits no edge:

Suggested change
if (
narrowOverloadCandidates([memberDef], site.arity, site.argumentTypes).length === 0
) {
memberDef = undefined;
continue;
if (
narrowOverloadCandidates([memberDef], site.arity, site.argumentTypes).length === 0
) {
memberDef = undefined;
break;

Comment on lines +96 to +99
type: (_)
(property_element
name: (variable_name) @declaration.name)) @declaration.property

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: @declaration.variable pattern double-matches typed properties

In tree-sitter, a field constraint in one pattern does not exclude that field in another. The @declaration.property pattern (lines 91–95) requires a type: field. But this catch-all pattern has no guard against the presence of a type: field, so tree-sitter matches it against all property_declaration nodes — including typed ones already captured above.

For a typed property such as private UserRepo $repo;, tree-sitter emits two matches:

  1. @declaration.propertyProperty def named repo (after $-strip in captures.ts)
  2. @declaration.variableVariable def named $repo (no strip, different type key)

These are registered with different DefIds and different names, so both survive into the graph. The spurious Variable node can conflict with type-binding lookups that key on $-prefixed names, and captures.ts itself acknowledges the expected mutual exclusivity (lines 95–98: "Only applies to @declaration.property") — but the query does not enforce it.

Fix options:

  • Use a tree-sitter #not-has? predicate on this pattern (if supported by the tree-sitter-php version in use): (#not-has? @declaration.variable type)
  • Or filter in captures.ts: skip the @declaration.variable processing branch when grouped['@declaration.property'] is also present for the same anchor node.

@github-actions

github-actions Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 5m 1s —— View job


Dynamic PHP Resolution Review — PR #1497

Tasks

  • Parse request and plan review
  • Read query.ts — check dynamic member/call patterns
  • Read captures.ts — check capture emission for dynamic constructs
  • Read interpret.ts — check import/type-binding for dynamic names
  • Read receiver-binding.ts — check $this/parent synthesis
  • Read scope-resolver.ts — check resolution logic
  • Read simple-hooks.ts — check hoisting hooks
  • Read arity.ts + arity-metadata.ts — check arity computation
  • Read shared passes: receiver-bound-calls.ts, overload-narrowing.ts, free-call-fallback.ts, method-dispatch.ts
  • Read php.test.ts — check test coverage for dynamic constructs
  • Search for dynamic patterns across all PHP files
  • Synthesize findings and post review

Dynamic PHP Resolution Review

Core Safety Mechanism

All dynamic-call safety in this PR is enforced by a single tree-sitter constraint: every call and write reference capture in query.ts requires name: (name), which is a static identifier node. In tree-sitter-php, dynamic names ($method, {$method}) produce variable_name nodes, not name nodes, and are therefore never matched by any reference pattern.

This is a structural guarantee, not a runtime check.


Findings

Finding 1 — $obj->$method(), $obj->{$method}(), $className::$method()Not a blocker

Severity: Informational
File: query.ts:214–229
Evidence:

(member_call_expression
  object: (_) @reference.receiver
  name: (name) @reference.name) @reference.call.member    // requires `name` node, not `variable_name`

(scoped_call_expression
  scope: (_) @reference.receiver
  name: (name) @reference.name) @reference.call.member    // same

PHP dynamic method names ($method) are variable_name nodes in the tree-sitter-php grammar. Neither pattern matches them. These call sites produce zero captures and flow through the pipeline invisibly. No false edges are possible.
Risk: None.
Blocks merge: No.


Finding 2 — $obj->$prop and $obj->{$prop} property reads — Not a blocker

Severity: Informational
File: query.ts
Evidence: There are no read-access property capture patterns in the query at all. The only property reference capture is the static-write pattern (query.ts:275–280) which also requires name: (name). Dynamic property read expressions are invisible to the pipeline.
Risk: None.
Blocks merge: No.


Finding 3 — $className::method() with variable-typed receiver — Acceptable, minor edge case

Severity: Low
File: query.ts:226–229, interpret.ts:112–116, receiver-bound-calls.ts
Evidence: scope: (_) in the scoped_call_expression pattern matches variable_name nodes. So $className::method() IS captured, with @reference.receiver = '$className'. Resolution proceeds through the type-binding cases:

  • If $className has no type binding in scope → all cases fall through → no edge. Safe.
  • If $className = 'UserService' (string literal) → normalizePhpType('string') → null → no binding. Safe.
  • If $className = new UserService() → binding to UserService → resolves correctly. Correct.
  • Edge case: if $className = $typedVar (variable alias, interpret.ts:113 preserves $), the resolved type follows the chain. If that chain yields a non-class type, nothing resolves. If it yields a class, the static method call resolves against it — semantically defensible since PHP static dispatch on a typed variable is uncommon.

No mechanism exists to confuse a string literal holding a class name with the class itself — normalizePhpType correctly rejects 'string' as a primitive.
Risk: Negligible. No false positive edges in normal usage.
Blocks merge: No.


Finding 4 — call_user_func($callable, ...)Not a blocker

Severity: Informational
File: query.ts:208–211, free-call-fallback.ts
Evidence: call_user_func is captured as an ordinary free call (function: (name) matches the literal text call_user_func). The resolver then looks for a def named call_user_func in the workspace index. Since PHP built-ins have no workspace definition, findCallableBindingInScope and pickUniqueGlobalCallable both return undefined, and the site is left unresolved.

Critical point: The resolver is purely structural. It never inspects argument values to infer callable targets. call_user_func($callable, $arg) produces a reference site with name='call_user_func', arity=2, argumentTypes=['','']. There is no code path that reads $callable to find the indirect call target.
Risk: None.
Blocks merge: No.


Finding 5 — call_user_func_array($callable, $args) and unknown arity — Not a blocker

Severity: Informational
File: free-call-fallback.ts, overload-narrowing.ts
Evidence: Same as Finding 4. The call is treated as a 2-argument free call to the built-in call_user_func_array. No special handling for the unknown size of $args is needed because the resolver never attempts to trace through the callable argument. Arity narrowing applies to the call_user_func_array invocation itself (arity=2), not to any inferred indirect call.
Risk: None.
Blocks merge: No.


Finding 6 — String callables 'foo', 'App\\Utils\\foo', 'ClassName::method'Not a blocker

Severity: Informational
File: captures.ts:330–352 (inferPhpArgType)
Evidence: String literals passed as arguments are classified as argType = 'string' for overload narrowing. They are never interpreted as callable references. No dispatch through string-form callables occurs.
Risk: None.
Blocks merge: No.


Finding 7 — Array callables [$obj, 'method'], ['ClassName', 'method']Not a blocker

Severity: Informational
Evidence: Array expression arguments are never the function: child of a function_call_expression — they're argument values. The query only captures the function-name position. Array callables never reach any resolution path.
Risk: None.
Blocks merge: No.


Finding 8 — phpEmitUnresolvedReceiverEdges and dynamic-looking patterns — Low-risk known limitation

Severity: Low
File: scope-resolver.ts:256–327
Evidence: This hook fires for static-named member calls where the receiver has no type binding. For any call $factory->process() where $factory is untyped and process is the sole method named process in the entire workspace, it emits a CALLS edge at confidence 0.6 with reason: 'php-unresolved-receiver-fallback'. This is intentional behavior to recover from mixed-typed parameters.

The hook is bounded by: (1) static method name only — dynamic names are never captured, (2) unique workspace-wide name match, (3) arity compatibility. The confidence and reason string distinguish these edges from high-confidence resolution.
Risk: Real but bounded. Can produce incorrect 0.6-confidence edges for common method names that happen to be unique. A workspace with a single process() method definition and several untyped $handler->process() calls would emit one wrong edge per call site.
Recommended follow-up (not merge-blocking): Add a minimum-confidence threshold at the consumer level, or require arity to match exactly (not just ≥ requiredParameterCount) for this fallback path.
Blocks merge: No.


Finding 9 — '...' vs 'params' variadic marker discrepancy — Not a blocker (harmless)

Severity: Informational
Files: arity-metadata.ts:46, arity.ts:40–41, overload-narrowing.ts:47–48
Evidence:

  • arity-metadata.ts appends '...' to parameterTypes for PHP variadic methods
  • arity.ts::phpArityCompatibility correctly checks t === '...'
  • overload-narrowing.ts::narrowOverloadCandidates (shared) checks t === 'params' (C# convention)

The discrepancy is harmless in practice because PHP variadic methods have parameterCount = undefined. In narrowOverloadCandidates, the max-exceeded branch (if (max !== undefined && argCount > max)) is only reached when parameterCount is defined — which never happens for PHP variadics. The 'params' check is dead code for PHP. However, documenting this asymmetry for future contributors would prevent confusion.
Blocks merge: No.


Finding 10 — Missing negative test coverage for dynamic patterns — Should be addressed

Severity: Medium
File: gitnexus/test/integration/resolvers/php.test.ts, gitnexus/test/fixtures/lang-resolution/
Evidence: Searched the entire test suite for call_user_func, $method, ->{$, dynamic. No results. The test suite covers many positive resolution scenarios, but there are zero negative tests confirming that dynamic PHP constructs are correctly left unresolved.

Tests that should exist per the review request — and don't:

Test Currently exists
$obj->$method() with unknown $method emits no edges
call_user_func($unknown) does not bind to arbitrary function
call_user_func([$obj, 'known']) — no edge when $obj untyped
$obj->$prop does not create a static edge to property prop
$className::method() — no edge when $className is untyped variable

The guarantees exist in the grammar (Finding 1–2) but are not regression-tested. A future refactor that inadvertently adds a variable_name match to the query would break the safety invariant silently.
Recommended fix: Add a PHP fixture with dynamic call patterns and a test asserting the edge set does NOT contain certain calls. Fix this →
Blocks merge: No — but strongly recommended before a second language consumer relies on these guarantees.


Tests/Coverage Assessment

The positive test suite is thorough: 159/160 passed (1 documented skip for mixed-typed receiver). The fixture coverage for normal PHP resolution (PSR-4, traits, enums, arity, namespaces) is solid.

The critical gap is entirely in negative coverage: there is no test that confirms the tree-sitter name: (name) safety net actually prevents dynamic-pattern capture. The guarantee is structurally sound today, but untested safety nets erode.


Final Verdict

production-ready with minor follow-ups for this dynamic-call slice

The core mechanism is sound: tree-sitter grammar node-type constraints (name: (name)) prevent dynamic PHP expressions from ever entering the reference capture pipeline. call_user_func and array/string callables are handled correctly by inaction — the pipeline never inspects argument values for callable dispatch, so no false edges are created. The phpEmitUnresolvedReceiverEdges hook is the only source of non-zero-confidence edges from dynamic-looking sites, and it is bounded, low-confidence, and documented.

The one action item before depending on these guarantees in production is adding negative regression tests (Finding 10) so the grammar-level safety net does not silently regress.


· PR branch

When the class-name receiver pass (Case 2 in receiver-bound-calls.ts)
found a most-derived definition that was arity-incompatible with the
call site, the previous code used `continue` to fall through to the
next ancestor in the MRO chain. If an ancestor happened to be arity-
compatible, the resolver emitted a false CALLS edge to it.

This is incorrect: PHP dispatches to the most-derived override at
runtime and throws `ArgumentCountError` when arities don't match -
it never silently redirects to an ancestor. Replace the inner
`continue` with `break` so the chain walk terminates and no edge
is emitted for the site.

Adds php-mro-arity-mismatch fixture and 5 regression tests covering:
- the bug scenario (Child::method/2 + Parent::method/1, call with 1 arg)
- arity-compatible happy path (Child::compat/1)
- no-parent case (Orphan with arity mismatch)
- happy path most-derived call
- class detection sanity check
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.
The PR abhigyanpatwari#1497 adversarial review confirmed via grammar inspection that
dynamic PHP call shapes ($obj->$method(), $obj->{$method}(),
Class::$method(), $className::method(), call_user_func with variable
/ string / array callables, dynamic property reads) currently produce
zero captures and zero CALLS edges - because the member_call_expression
and scoped_call_expression query patterns constrain `name:` to
`(name)` rather than `(_)`, deliberately excluding variable_name
nodes.

That safety invariant was not regression-tested. A future query.ts
edit relaxing `name:` to `(_)` would silently emit false-positive
edges. Add a php-dynamic-calls fixture covering all 11 dynamic shapes
from Findings 1-7 plus a sanity-check static call that DOES emit an
edge - without the sanity check every zero-edge assertion would pass
even if the pipeline emitted no edges at all.

Adds inline SAFETY-INVARIANT comments above the three load-bearing
query patterns (member_call_expression, scoped_call_expression, and
the static-write property pattern) referencing this fixture as the
regression net.
…y (U4)

phpEmitUnresolvedReceiverEdges emits 0.6-confidence CALLS edges for
member-call sites whose receiver has no type binding (PHP 'mixed'-typed
parameters, untyped variables) when the called method name has a single
workspace-wide candidate. The existing first-stage gate accepted any
argCount in 'min..max' (or '>= min' with variadic), which over-emits
when the lone candidate has optional / defaulted parameters - the
fallback fires for calls that happen to fit the wider range but
weren't meant for that target.

Add an EXACT-required-arity gate for the 0.6-confidence path only:
for fixed-arity candidates, require argCount === requiredParameterCount.
Variadic candidates ('...' marker in parameterTypes) keep the relaxed
'>= required' semantics already enforced by the first-stage check.

Adds php-unresolved-receiver-arity fixture with seven scenarios covering
happy path, default-param exact match, default-param over-arity (the
narrowed case), variadic at/above/below required, and the class-
detection sanity check. Pre-fix the over-arity scenario emits a stray
0.6 edge; post-fix it does not. All existing PHP integration tests
continue to pass.
The shared narrowOverloadCandidates pass in scope-resolution/passes/
overload-narrowing.ts checks for 'params'/'params ' as the variadic
marker - the C# 'params' keyword convention. PHP uses '...' instead,
matching its source-language syntax. The discrepancy is harmless in
practice because PHP variadic methods set parameterCount=undefined,
which skips the 'max !== undefined && argCount > max' gate that hosts
the 'params' check entirely - so the C# branch is dead code for PHP.

Document the asymmetry at both sites with cross-references so a future
contributor reading either file lands on the full picture:

- overload-narrowing.ts:48 area: explains the 'params' branch is C#-
  specific and warns that adding new markers requires auditing each
  adapter's arity-metadata.ts.
- languages/php/arity-metadata.ts:46 area: explains PHP uses '...'
  intentionally and that the shared pass's 'params' branch is dead
  code for PHP because of the parameterCount=undefined setting.

No behavior change. Existing PHP variadic-resolution tests already
exercise the live '...' path.
Four tests added by U1 and U4 assert correctness wins that only the
scope-resolution pipeline delivers. The legacy DAG path has no
equivalent narrowing, so it emits the over-broad edges these tests
forbid:

- U1: MRO arity narrowing on class-name receivers (Case 2 in
  receiver-bound-calls.ts) — legacy has no arity check between MRO
  iterations, so 'Child::method(1)' falls through to Parent::method.
- U1: single-class arity gating — legacy resolves by name without
  arity, so an arity-incompatible call on a class with no parent
  still emits an edge.
- U4: 'phpEmitUnresolvedReceiverEdges' exact-required-arity gate —
  the legacy DAG has no equivalent unresolved-receiver fallback hook,
  so default-param over-arity and variadic-below-required shapes
  resolve via a different code path that over-emits.

Backporting these narrowing checks to the legacy DAG is out of scope,
matching the existing pattern in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES
for record/pad zero-args (commit af9af4a U1) and the FQN cross-
namespace test (Codex PR abhigyanpatwari#1497 finding 1).
Four tests were failing on lang/php-migration-v2 in the tests/ubuntu/coverage
job before this commit. Root causes and fixes:

1. cross-file-binding.test.ts: Consumer-Before-Provider PHP
   The fixture had BProvider.php at app/BProvider.php while declaring
   namespace App\Models — a PSR-4 violation that caused PHP's import-
   target resolver to return null for 'use function App\Models\getUser'.
   Without a resolved import target, no IMPORTS edge was emitted, the
   SCC graph had no edge between consumer and provider, and the shared
   propagateImportedReturnTypes pass never mirrored getUser's return
   type into AConsumer's scope. Result: $u (bound to 'getUser' by
   query.ts:167-169 type-binding.alias) never collapsed to User, so
   $u->save() went unresolved.

   Move BProvider.php to its PSR-4-compliant location at
   app/Models/User.php. The class+function were already in namespace
   App\Models; the rename just aligns the file path with the
   namespace prefix mapped by composer.json. After the move:
     - PSR-4 directory scan finds the file for 'use function
       App\Models\getUser' (lines 65-86 of php import-resolver).
     - IMPORTS edge AConsumer.php → app/Models/User.php emits.
     - SCC reverse-topological walk in propagateImportedReturnTypes
       mirrors getUser→User from User.php's module-scope typeBindings
       into AConsumer's, then chain-follows $u → getUser → User.
     - $u->save() resolves to User#save.

2. registry-primary-flag.test.ts: 'flipped languages' size === 1
   When PHP was added to MIGRATED_LANGUAGES at commit 69786b1 (the
   PHP scope-based resolution migration), the hard-coded opt-out list
   in this test did not include REGISTRY_PRIMARY_PHP. With PHP
   default-on and not opted out, enabled.size returned 2 (Java + PHP)
   instead of 1. Rewrite the opt-out loop to iterate
   MIGRATED_LANGUAGES dynamically so future Ring 3 additions land
   without test churn.

3. overload-narrowing.test.ts: 'falls back to the full overload list'
   Commit af9af4a (PR abhigyanpatwari#1497 production review fixes) deliberately
   tightened narrowOverloadCandidates to stop silently rescuing
   empty filter sets when every candidate had definite bounds — the
   call is genuinely arity-incompatible (e.g. PHP variadic with
   required-prefix called with too few args). The unit test still
   asserted the old soft-rescue behavior. Rename and update the
   assertion to match the intentional new semantics; document that
   the 'anyUnknownBounds' branch in the source is structurally
   unreachable from this caller shape (an unknown-bounds candidate
   always passes the arity filter, so arityMatches is always
   non-empty when anyUnknownBounds is true).

4. registries.test.ts: 'keeps incompatible candidates (soft penalty)'
   Same root cause as abhigyanpatwari#3 at the registries layer: lookup-core.ts now
   drops every candidate when all are 'incompatible' and none are
   'unknown'. The 'soft penalty' kept-with-evidence behavior survives
   only when at least one candidate's arity verdict is 'unknown' —
   that signal differentiates definite mismatch from missing metadata.
   Update the existing test to assert the new hard-rejection behavior
   and add a new test exercising the surviving soft-penalty path with
   one 'unknown' + one 'incompatible' candidate.

Verification:
- Originally-failing tests: 4 of 4 now pass (no skips).
- PHP integration suite: 205/205 in primary mode, 197+8 skipped in
  legacy mode (unchanged).
- Cross-language unit + integration: no regressions.
@github-actions

github-actions Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 6m 12s —— View job


Production Readiness Review — PHP Scope-Based Resolution (PR #1497)

Tasks

  • Parse request and plan review
  • Verify PR state (head SHA, diff, branch hygiene)
  • Read repo context files (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, ARCHITECTURE.md)
  • Read shared pipeline files (receiver-bound-calls.ts, overload-narrowing.ts, method-dispatch-index.ts)
  • Read PHP module files (query.ts, captures.ts, interpret.ts, scope-resolver.ts, arity.ts, arity-metadata.ts, import-target.ts)
  • Read test files (php.test.ts, helpers.ts legacy skip list)
  • Run hidden Unicode / bidi hygiene check
  • Synthesize findings and post full review

Production Readiness Review

Review Bar for This PR

  • No AST reads at resolution time; all PHP semantics modeled as scope captures / type bindings / imports
  • Confidence and provenance are explicit; no high-confidence edge on ambiguous or dynamic evidence
  • PHP namespace, FQN, PSR-4, trait/MRO, $this/parent, arity, and dynamic-call semantics are semantically correct or conservatively omitted
  • Shared pipeline changes (receiver-bound-calls.ts, overload-narrowing.ts, free-call-fallback.ts, method-dispatch-index.ts) do not regress Python, C#, TypeScript, Go, or C
  • Every key resolution risk has a positive and negative integration test
  • All CI checks pass on Linux/macOS/Windows; typecheck and lint clean

Current PR State

Head SHA verified: 713aa92 — confirmed locally.
Mergeability: Branch is merge-from-main (Merge branch 'main' into lang/php-migration-v2) at tip; cleanly merge-able against current main.
Checks verified: CI report (comment above) shows ✅ all checks passed, 8807/8808 tests, 1 skipped (Ruby block-parameter — unrelated). Typecheck clean.
Unresolved comments verified: Three review comments present from prior CI/bot runs. Two are from github-actions (U1 MRO fallthrough and @declaration.variable dedup bug) — both have been fixed in subsequent commits (29e45f1 and c82fa83 respectively). One is from GitHub Advanced Security (CodeQL "unused function" — false positive, see Finding A below).
Changed files verified: 71 files match the PR description; diff is consistent with what's in the repo.
Notion RFC access: Unavailable (404). Fallback used: GitHub issue #909 + AGENTS.md scope-resolution pipeline notes.
GitHub visibility limitations: No hidden/bidi Unicode controls found in any PHP source, test, or shared-pass file. Em dashes () in doc comments are visible ASCII-adjacent characters — acceptable.


Branch Hygiene Assessment

merge-from-main commit present but harmless and merge-safe

The branch contains 6+ Merge branch 'main' into lang/php-migration-v2 commits acquired over a multi-week development period. All are expected merge-syncs; the merged-in content (fix(lbug), feat(analyze), feat(embeddings), etc.) is already on main and introduces no unrelated churn to the PHP files. No shared-pipeline file was touched in a merge commit. PHP-specific commits are cleanly prefixed fix(php): / test(php): and fully isolated. Rebase is not required.


Understanding of the Change

The PHP scope-resolution pipeline works as follows:

  1. Tree-sitter capture phase (query.ts + captures.ts): emitPhpScopeCaptures runs the PHP tree-sitter query, groups captures per match, and synthesizes additional CaptureMatch streams for (a) decomposed use declarations (import-decomposer.ts), (b) $this/parent receiver bindings (receiver-binding.ts), (c) arity metadata on method/function declarations (arity-metadata.ts), (d) PHPDoc @param/@return annotations, and (e) foreach loop variable bindings. A cross-match pre-scan suppresses the @declaration.variable catch-all for typed property_declaration anchors already captured as @declaration.property.

  2. Scope/import/type-binding construction: The central extractor interprets capture matches via interpretPhpImport and interpretPhpTypeBinding, populating ParsedFile.parsedImports and ParsedFile.typeBindings. Import targets are resolved via PSR-4 (import-target.ts + resolvePhpImportInternal). Namespace siblings (same-PHP-namespace cross-file visibility) are injected by populatePhpNamespaceSiblings, including FQN-keyed module-scope bindings for fully-qualified type references.

  3. Finalization / index construction: The orchestrator builds the MethodDispatchIndex using buildPhpMro (trait-augmented MRO via IMPLEMENTS edges) and optionally buildPhpExtendsOnlyMro (EXTENDS-only, for parent:: dispatch). Traits become linkable nodes via the isLinkableLabel('Trait') addition in node-lookup.ts.

  4. Registry lookup / emission passes: emitReceiverBoundCalls dispatches through 7 cases (super→compound→namespace→class-name→dotted-type→chain-type→simple-type). The super branch uses extendsOnlyMroFor when provided so parent:: bypasses traits. Case 2 (class-name) now terminates the MRO loop on the first arity-incompatible most-derived definition. phpEmitUnresolvedReceiverEdges fires for untyped/mixed receivers, emitting confidence: 0.6 / reason: 'php-unresolved-receiver-fallback' edges only when there is exactly one workspace method candidate AND argCount === required (for fixed-arity) or argCount >= required (for variadic).

  5. Graph emission: CALLS / ACCESSES / IMPORTS edges are written to the KnowledgeGraph with confidence, reason, and provenance.


Findings

Finding A — CodeQL "Unused function phpEmitUnresolvedReceiverEdges" (False Positive) — Not a blocker

Severity: Informational
Evidence: GitHub Advanced Security flags phpEmitUnresolvedReceiverEdges as unused. The function is defined at scope-resolver.ts:256 and registered at scope-resolver.ts:418 as emitUnresolvedReceiverEdges: phpEmitUnresolvedReceiverEdges. CodeQL's TypeScript inter-procedural analysis does not track property-bag assignment as a "use" of the function in the same way an explicit call would register.
Risk: None. The function is correctly wired.
Recommended fix: Add a CodeQL suppression comment or dismiss the alert as a false positive. No code change required.
Blocks merge: No.


Finding B — PSR-4 Multiple-Directory-Per-Namespace Support (Known Limitation) — Not a blocker

Severity: Low
Evidence: import-target.ts:56:

const dir = Array.isArray(dirs) ? dirs[0] : dirs;

PSR-4 spec permits a single namespace prefix to map to multiple directories (uncommon but valid). Only the first directory is used.
Risk: A project with "App\\": ["src/", "lib/"] would fail to resolve imports for PHP files under lib/. Low frequency in practice; conservative behavior (null return → linkStatus: 'unresolved') rather than false-positive.
Recommended fix: Expand to resolve against all directories in the array and return the first match. Fix this →
Blocks merge: No — documented conservative behavior.


Finding C — Trait Alias/Conflict (as/insteadof) Not Implemented — Not a blocker

Severity: Low / Documented Limitation
Evidence: buildPhpMro in scope-resolver.ts augments the MRO by prepending traits in direct-use order (traits used by child first, then parent traits). PHP's as (alias) and insteadof (conflict resolution) directives are not modeled. If two used traits both define method log() and no insteadof is declared, the resolver picks whichever trait was registered first in directTraitUse iteration order — correct only if there is no ambiguity.
Risk: False-positive CALLS edge when two traits conflict on the same method name and the project relies on PHP's compile-time fatal-error behavior (undeclared conflict = compile error). In practice, well-formed PHP must use insteadof to resolve conflicts, so ambiguous trait workspaces are invalid PHP. The resolver emitting one edge in such a case is no worse than the legacy DAG. This is not a new regression.
Recommended fix: Document as known limitation in a code comment; track as follow-up to RFC #909 Ring 4. Low urgency.
Blocks merge: No.


Finding D — pickOverload Falls Back to overloads[0] on Non-Unique Narrowing (Shared Concern) — Not a blocker

Severity: Low
Evidence: receiver-bound-calls.ts:523:

const candidates = narrowOverloadCandidates(overloads, site.arity, site.argumentTypes);
return candidates[0] ?? overloads[0];

When narrowOverloadCandidates returns multiple arity-compatible candidates but none is uniquely narrowed by argument types, this falls back to overloads[0] — which is registration-order-dependent. This is a pre-existing shared behavior (not introduced by this PR). PHP has no true overloads by name (methods must have unique names per class), so this path is mostly relevant for the C# adapter. This PR does not worsen it.
Blocks merge: No.


PR-Specific Assessment Sections

RFC Contract / Pure Scope Model — ✅ Clean

All PHP resolution is performed against the semantic model at resolution time. No re-parsing occurs. emitPhpScopeCaptures is the only site that reads AST nodes, and it runs during the capture phase before any registry lookup. phpEmitUnresolvedReceiverEdges performs workspace-wide method-registry lookups (model.methods.lookupMethodByName) — pure semantic model access. The findNodeAtRange call in captures.ts is capture-phase only. No RFC contract violation found.

PHP Provider and Registry-Primary Wiring — ✅ Clean

MIGRATED_LANGUAGES includes PHP, phpScopeResolver is registered in pipeline/registry.ts, and the REGISTRY_PRIMARY_PHP env flag routes correctly via createResolverParityIt('php') in tests. Legacy and scope-primary modes are both tested via helpers.ts:createResolverParityIt. The 9-entry legacy skip list (helpers.ts:37-82) is narrowly scoped — each skip names the exact test and provides a precise reason why the legacy DAG diverges from the scope-resolver. No broad fixture-level skips.

Tree-Sitter Query / Capture Correctness — ✅ Clean (with note)

Dynamic method names ($method, {$method}) produce variable_name nodes in tree-sitter-php and are structurally excluded by the name: (name) constraint in all call patterns. This is verified by the php-dynamic-calls regression suite with a sanity-positive edge guard.

The @declaration.variable double-match dedup is implemented correctly. The pre-scan in captures.ts:91-99 collects property_declaration node IDs from @declaration.property captures. The main loop's dedup check at line 114-117 uses the same node — both patterns capture the same property_declaration anchor node, confirmed by reading query.ts:95-103. The cross-match approach is correct because it pre-scans all rawMatches before the processing loop, so it correctly identifies typed properties across separate match objects. ✅

Namespaces, FQNs, Imports, and PSR-4 — ✅ Clean

normalizePhpType preserves qualified form (App\Models\User not simplified to User) after stripping the leading backslash. FQN-keyed module-scope bindings are injected by populatePhpNamespaceSiblings Step 3b so \App\Other\User $u parameter annotations resolve to the Other/User.php class even when use App\Models\User is in scope. Test php-fqn-cross-namespace has both a positive (FQN → Other/User.php) and a negative control (simple-name → Models/User.php) assertion.

Grouped use statements, use function, use const, and aliased imports are all decomposed by import-decomposer.ts and produce correct ParsedImport shapes. Tests php-grouped-imports, php-alias-imports, php-use-function-const, and php-namespace-fallback-isolation all provide positive+negative coverage.

Minor PSR-4 limitation: multiple directories per namespace prefix — see Finding B.

Receiver/Type Binding — ✅ Clean

$this, self, static, and parent are synthesized by receiver-binding.ts. parent is recognized via isSuperReceiver: (text) => text.trim() === 'parent'. Constructor-promoted properties produce both @declaration.property and @type-binding.annotation captures. Typed properties are deduped (Finding U2, fixed). Nullable (?User), union single-non-null (User|null), and intersection (User&Loggable) types are handled conservatively in normalizePhpType. mixed, array, callable, and unknown types return null from normalizePhpType, correctly preventing overconfident binding.

Traits, Interfaces, MRO, and Dispatch — ✅ Clean

buildPhpMro uses BFS via collectTransitiveTraits with cycle detection (seen set). Transitivity is validated at depth-3 in php-transitive-traits. parent:: dispatch uses extendsOnlyMroFor (built by buildPhpExtendsOnlyMro = buildMro without trait augmentation), correctly bypassing composed traits. Test php-parent-vs-trait asserts parent::record → Base::record AND $this->record → Auditable::record (trait shadows parent). Trait is now linkable in node-lookup.ts, enabling MRO-based dispatch to trait-owned methods.

Trait alias/conflict (as/insteadof) is not implemented — documented limitation (Finding C).

Arity and Overload Narrowing — ✅ Clean

phpArityCompatibility returns 'unknown' for named-argument call sites (negative arity). Variadic methods have parameterCount = undefined, making the C# params branch in narrowOverloadCandidates dead code for PHP (correctly documented in arity-metadata.ts:46-56 as Finding U5). The variadic required count correctly subtracts the variadic slot: f(int $req, ...$rest) requires 1, not 2. Tests php-variadic-arity-minimum verify both above-required (pass) and below-required (no edge) cases.

The '...' vs 'params' asymmetry is harmless because PHP variadic detection in the fallback path (phpEmitUnresolvedReceiverEdges:317-318) uses t === '...' directly.

Dynamic Calls and Unresolved Receiver Fallback — ✅ Clean

Dynamic method names structurally excluded by name: (name) (confirmed). phpEmitUnresolvedReceiverEdges fires only for static-name call sites with no type binding in scope, when there is exactly one workspace method match. For fixed-arity candidates, argCount === required is now enforced (exact-required gate, scope-resolver.ts:319-327). Variadic candidates keep relaxed >= required semantics. Confidence is 0.6, reason is 'php-unresolved-receiver-fallback'. Test php-unresolved-receiver-arity provides full positive/negative coverage including variadic cases. Test php-dynamic-calls has a sanity-positive guard (sanityStaticallyNamedTarget) ensuring zero-edge assertions cannot pass silently.

Shared Pipeline Regression Risk — ✅ Clean

  • node-lookup.ts: Adding 'Trait' to isLinkableLabel is additive; no existing language uses Trait nodes in graph-bridge lookups, so existing languages are unaffected.
  • receiver-bound-calls.ts: The extendsOnlyMroFor fallback is guarded: const extendsOnly = scopes.methodDispatch.extendsOnlyMroFor; const ancestors = extendsOnly !== undefined ? extendsOnly(enclosingClass.nodeId) : scopes.methodDispatch.mroFor(enclosingClass.nodeId). Languages without buildExtendsOnlyMro receive undefined and fall back to mroFor — identical behavior to before this PR.
  • overload-narrowing.ts: The 'params' variadic-marker branch is unchanged and documented as C#-specific. PHP's '...' marker does not reach this branch because PHP variadics have parameterCount = undefined.
  • free-call-fallback.ts: Arity narrowing in pickUniqueGlobalCallable now applies PHP's phpIsCallableVisibleFromCaller visibility filter, preventing cross-namespace false positives. Other languages' isCallableVisibleFromCaller hooks are unaffected.
  • method-dispatch-index.ts: computeExtendsOnlyMro is optional at the shared contract level; all existing adapters that don't provide it continue to work unchanged.

The CI report shows Go, C#, Python, TypeScript passing under their respective registry-primary flags.

Legacy Parity and Expected Failures — ✅ Clean

9 PHP legacy-DAG parity skips in helpers.ts:37-82, each with an exact test name and documented reason. All skips are scope-resolver correctness wins (arity narrowing, trait-aware MRO, FQN resolution) that the legacy DAG structurally cannot support. No broad fixture-level skips. The createResolverParityIt wrapper correctly gates skips only when REGISTRY_PRIMARY_PHP=0, so scope-primary mode runs all tests.

Tests and CI — ✅ Strong

159 PHP integration tests pass (1 skipped: documented mixed-receiver limitation for save($entity: mixed)->getId()). Every critical semantic path has both a positive edge assertion and a negative edge assertion:

Risk area Fixture Positive Negative
MRO arity mismatch (U1) php-mro-arity-mismatch callCompatible → ChildModel callIncompatible → no edge
Typed property dedup (U2) php-typed-property-dedup ✅ exactly 1 repo Property ✅ zero $repo Variable
Dynamic calls (U3) php-dynamic-calls ✅ sanity static edge ✅ zero edges for all dynamic shapes
Fallback arity gate (U4) php-unresolved-receiver-arity ✅ exact-required matches ✅ beyond-required rejected
FQN cross-namespace php-fqn-cross-namespace ✅ FQN → Other/User.php ✅ not Models/User.php
parent:: bypass traits php-parent-vs-trait ✅ parent → Base.php ✅ parent not Auditable.php
Transitive traits php-transitive-traits ✅ depth-3 deepMethod N/A (pure positive)
Namespace isolation php-namespace-fallback-isolation ✅ same-ns matches ✅ cross-ns no edge

Hidden Unicode / Hygiene — ✅ Clean

No bidi control characters (U+202A–U+202E, U+2066–U+2069) found in any PHP source, test, shared-pass, or fixture file. Non-ASCII characters present are em dashes () in doc comments — acceptable visible punctuation.


Final Verdict

production-ready with minor follow-ups

All issues identified in prior reviews (U1 MRO fallthrough, U2 typed-property dedup, U3 dynamic-dispatch suppression, U4 unresolved-receiver fallback tightening, U5 variadic marker documentation, FQN preservation, transitive traits, parent:: bypass, namespace isolation) have been addressed with targeted fixes and regression tests. The shared pipeline changes are backward-compatible for all existing languages. The legacy parity skip list is narrowly scoped with precise justifications. CI passes cleanly across all platforms.

Two minor follow-ups are recommended but not merge-blocking: (1) expand PSR-4 array-directory support (Finding B), and (2) dismiss the CodeQL false positive for phpEmitUnresolvedReceiverEdges (Finding A). The undocumented as/insteadof trait conflict limitation (Finding C) should be tracked as a follow-up issue — it does not affect correct PHP code (which must declare conflicts explicitly).


· PR branch

@magyargergo magyargergo merged commit 8083c39 into abhigyanpatwari:main May 12, 2026
28 of 29 checks passed
@magyargergo magyargergo deleted the lang/php-migration-v2 branch May 12, 2026 15:56
magyargergo added a commit to SZU-WenjieHuang/GitNexus that referenced this pull request May 12, 2026
Resolve conflicts from PR abhigyanpatwari#1497 (PHP migration to scope-based resolution):
- registry-primary-flag.ts: include both CPlusPlus and PHP in MIGRATED_LANGUAGES
- scope-resolution/pipeline/registry.ts: register both cppScopeResolver and phpScopeResolver
- registry-primary-flag.test.ts: keep dynamic opt-out loop from main; assert both CPlusPlus and PHP are off
- call-processor.ts: keep HEAD's deferred-write 'line' field (used for unique ACCESSES edge IDs)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LANG-php: Migrate PHP to scope-based resolution

2 participants