fix(php): import decomposition, enum cases, anonymous class scope — F53,F54,F55 (#1931)#1989
Conversation
|
@prajapatisparsh is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
✨ PR AutofixFound fixable formatting / unused-import issues across 14 changed lines. Comment |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10586 tests passed 16 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
left a comment
There was a problem hiding this comment.
🔍 Tri-Review — PHP import / enum / anonymous-class coverage (F53 / F54 / F55)
Methods & engines. GitNexus swarm (risk, test/CI) + Compound-Engineering personas (correctness, adversarial, maintainability, testing) + Codex. Engine breakdown: 6 Claude reviewer lanes + Codex (the only independent engine). Codex investigated the PR but — its sandboxed shell was blocked, forcing GitHub-fetch-only analysis — it did not return a final structured verdict. Its interim conclusions corroborated the grammar shape (namespace_use_declaration = commaSep1(namespace_use_clause)) and the anonymous-class path, but the headline finding below rests on a Claude lane plus my own end-to-end reproduction, not an independent Codex confirmation. Two of the methods are Claude under different personas (correlated), so agreement here is "consistent across personas," not three independent confirmations.
Bottom line. The F53/F54/F55 changes are correct and well-validated. I reproduced correct output through the real tree-sitter-php parser (use A, B, C; → 3 imports; use Foo\Bar, Baz\Qux as Quux, X\Y; → 3 with the alias resolved; enum cases and anonymous-class scopes captured), and the new behavior is byte-enforced by the golden-captures digest, which CI passes. No regression to existing PHP resolution. The actionable items are one pre-existing P1 bug in the same function being edited (a one-line fix worth folding in) plus minor cleanup and a test-strength nit.
| Final verdict | production-ready with minor follow-ups — F53/F54/F55 are correct and CI-validated; the P1 is pre-existing and non-blocking; the rest is P2/P3 cleanup. |
| Branch hygiene | Clean — the PR's own change is 7 files, all scoped to PHP coverage, mergeable: MERGEABLE. The branch carries merge-from-main commits (head 3741ee1d); the most recent merge pulled in unrelated #1990 (cpp ADL) but touched no PHP file — all reviewed PHP sources/tests/golden are byte-identical to the tree I reviewed. |
| Merge state | All functional CI checks green; the only non-passing check is Vercel (“Authorization required to deploy” — a deploy-auth gate, not a code test). mergeStateStatus: UNSTABLE is attributable to that Vercel gate alone. |
Note: the PR head advanced from
8309a533to3741ee1d(a main merge) while this review ran; the merge brought in unrelated #1990 work only — every PHP file under review is unchanged, so all findings and line anchors below remain valid.
🔴 Headline — P1, pre-existing, strongly recommend fixing here
Grouped use imports silently drop their namespace prefix. decomposeGrouped() (import-decomposer.ts:195) reads the group prefix with findNamedChild(stmtNode, 'qualified_name') ?? findNamedChild(stmtNode, 'name'), but tree-sitter-php 0.23.x emits that prefix as a namespace_name node — so both lookups return null, prefix = '', and every inner clause loses its namespace.
- Reproduced (real parser; coordinator + adversarial lane):
use App\Models\{User, Post as P};→source="User"/source="Post"(should beApp\Models\User/App\Models\Post);use A\B\C\{D};→source="D". - Downstream impact (reproduced by the adversarial lane): with two same-simple-name classes in different namespaces, the prefix-stripped bare name resolves to the wrong file via the PSR-4 suffix fallback. The existing
php-grouped-importsfixture hides this because its class names (User/Repo/Main) are globally unique. - Not introduced by this PR (
decomposeGroupedis untouched) and it does not block F53/F54/F55. But it lives in the exact function being rewritten, and the new code already identifies thenamespace_namenode type — so the fix is one line at 195:… ?? findNamedChild(stmtNode, 'namespace_name'). Add a fixture with two same-simple-name classes in different namespaces so the suffix fallback can't mask a regression. (Inline note left at the nearest in-diff line.)
🟡 Inline findings (verified, anchored in-diff)
import-decomposer.ts:66— dead code + misleading grammar comment (P2). The new "First child:namespace_name" block never executes. A directnamespace_namechild ofnamespace_use_declarationappears only in the grouped form (use Prefix\{…}, wherenamespace_nameis the prefix) — and that form is detected and returned earlier (lines 54–57,namespace_use_group). Every declaration that reaches this non-grouped block therefore has onlynamespace_use_clausechildren, sofindNamedChild(stmtNode, 'namespace_name')is alwaysnullhere (confirmed by real parse across 12 probes — every input with a directnamespace_namechild also hadnamespace_use_group; Codex independently derived the non-grouped grammar ascommaSep1(namespace_use_clause)). The clause loop below does all the work, so output is correct, but lines 64–79 are unreachable and the comment ("the grammar puts the first import path as anamespace_namechild") is wrong for the non-grouped form it describes. (Note the symmetry with the headline: this dead block looks for anamespace_nameprefix node that only the grouped path has — yet the grouped path at line 195 fails to read it.)php-coverage.test.ts:82/89— F55 test strength (P2). The first anonymous-class test proves the new rule (counterfactual: removing(anonymous_class) @scope.class→ 0 class scopes → fails). The second (fnScopes ≥ 1) asserts pre-existingmethod_declaration → @scope.function, which holds regardless of the new rule. PinclassScopestotoBe(1)and document the second as a method-scoping regression guard.
🟢 Lower-priority / notes
parseSingleUseClause(lines 115–134) is now dead code — its only caller was replaced by the rewrite. ESLint warns; CI still exits 0 (no--max-warnings). Remove it. (P3)- Duplicated alias-detection between
parseUseClause(155–168) andparseInnerClause(248–261) — extractable to one helper. (P2, maintainability) - Enum
implements Iemits no inheritance edge — the heritage synthesizer skipsenum_declaration; pre-existing but adjacent to the new enum work, so a reader might assume enum heritage is now handled. (P3) - Aliased
use function Foo\f as g;collapses tokind='alias', dropping thefunctionqualifier — harmless (interpret.tsnormalizes alias/function/const identically) and pre-existing; not a defect.
✅ Validated / refuted suspicions (credit where due)
- Double-count from the dead block — refuted:
use A, B, C;→ exactly 3. detectQualifiermis-firing onuse functionFoo\bar;— refuted: the regex requires a trailing\sword boundary.- Trailing comma
use A, B,;/ leading separator — refuted: no phantom imports. - Nested anonymous classes — refuted: two distinct, correctly-nested scopes.
enum_case@declaration.constcolliding with class constants — refuted: PHP has no class-const capture, so enum cases are the sole producer; no shadowing.
Coverage & caveats
Focused 7-file PHP PR; the full source diff was read and exercised through the real tree-sitter-php parser. Test gaps noted (no use function f1, f2; KIND-propagation assertion — though I verified that path works; no grouped-prefix fixture). Codex did not return a final verdict, so cross-engine confidence is lower than a clean three-method run — weight the Claude-lane agreement as "consistent," not independently confirmed.
Automated multi-tool digest (GitNexus swarm + CE personas + Codex). Verify before acting.
3741ee1 to
313186a
Compare
| $service = new class { | ||
| public function execute(): void { | ||
| echo "running"; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Can you test if the $service->execute(); will connect the CALLS edge properly to the right nodes? 🙏
There was a problem hiding this comment.
CALLS edge won't resolve. $service has no type binding (anonymous_class doesn't match the constructor pattern). The scope/capture-level fix is what F55 was about; full CALLS resolution for anonymous class instances is a separate gap.
|
okay i am working on it , but i have a doubt regarding grouped prefix bug , it is pre-existing, not from this PR. Do you still want the fixture added here, or should that be a separate issue? |
You’re correct! The grouped prefix bug is pre-existing on main, not from F53/F54/F55. Please keep the one-line |
d5657a0 to
562a870
Compare
|
@magyargergo should i re apply the wiped fixes ? |
You pushed changes that were conflicting so i removed that commit, and give you the option to properly rebase. |
okay okay |
F53: iterate all namespace_use_clause + namespace_name children
F54: enum_case captured as @declaration.const
F55: anonymous_class @scope.class
F56 closed by #1937
7 new tests, golden regenerated.
Refs #1931