From fcb0e30dd17739295cead429653a1e974b05adaa Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 12 May 2026 16:03:17 +0100 Subject: [PATCH] fix(php): PSR-4-compliant fixture + pre-existing test debt cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 69786b16 (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 af9af4a9 (PR #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 #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. --- .../app/{BProvider.php => Models/User.php} | 0 .../test/unit/registry-primary-flag.test.ts | 15 ++++--- .../overload-narrowing.test.ts | 19 +++++++- .../unit/scope-resolution/registries.test.ts | 45 +++++++++++++++++-- 4 files changed, 68 insertions(+), 11 deletions(-) rename gitnexus/test/fixtures/cross-file-binding/php-consumer-before-provider/app/{BProvider.php => Models/User.php} (100%) diff --git a/gitnexus/test/fixtures/cross-file-binding/php-consumer-before-provider/app/BProvider.php b/gitnexus/test/fixtures/cross-file-binding/php-consumer-before-provider/app/Models/User.php similarity index 100% rename from gitnexus/test/fixtures/cross-file-binding/php-consumer-before-provider/app/BProvider.php rename to gitnexus/test/fixtures/cross-file-binding/php-consumer-before-provider/app/Models/User.php diff --git a/gitnexus/test/unit/registry-primary-flag.test.ts b/gitnexus/test/unit/registry-primary-flag.test.ts index 9e8be3455..864754b7f 100644 --- a/gitnexus/test/unit/registry-primary-flag.test.ts +++ b/gitnexus/test/unit/registry-primary-flag.test.ts @@ -148,17 +148,20 @@ describe('primaryLanguages', () => { it('returns exactly the flipped languages (env opts in unmigrated, opts out migrated)', () => { // Migrated languages are default-on; each must be opted out here when - // testing explicit env overrides. Java (unmigrated) opts in; Go stays off. - process.env['REGISTRY_PRIMARY_PYTHON'] = 'false'; - process.env['REGISTRY_PRIMARY_CSHARP'] = 'false'; - process.env['REGISTRY_PRIMARY_TYPESCRIPT'] = 'false'; - process.env['REGISTRY_PRIMARY_GO'] = 'false'; - process.env['REGISTRY_PRIMARY_C'] = 'false'; + // testing explicit env overrides. Java (unmigrated) opts in. + // Opt out every member of MIGRATED_LANGUAGES dynamically so this test + // does not have to be updated each time a new language ships its + // Ring 3 migration (PHP joined the set in commit 69786b16; future + // Ring 3 additions land here without test churn). + for (const lang of MIGRATED_LANGUAGES) { + process.env[envVarNameFor(lang)] = 'false'; + } process.env['REGISTRY_PRIMARY_JAVA'] = '1'; const enabled = primaryLanguages(); expect(enabled.has(SupportedLanguages.Python)).toBe(false); expect(enabled.has(SupportedLanguages.CSharp)).toBe(false); expect(enabled.has(SupportedLanguages.Go)).toBe(false); + expect(enabled.has(SupportedLanguages.PHP)).toBe(false); expect(enabled.has(SupportedLanguages.Java)).toBe(true); // Only Java is on: migrated defaults overridden off, Java explicitly on. expect(enabled.size).toBe(1); diff --git a/gitnexus/test/unit/scope-resolution/overload-narrowing.test.ts b/gitnexus/test/unit/scope-resolution/overload-narrowing.test.ts index 810c3a79b..9a14fdddd 100644 --- a/gitnexus/test/unit/scope-resolution/overload-narrowing.test.ts +++ b/gitnexus/test/unit/scope-resolution/overload-narrowing.test.ts @@ -69,11 +69,26 @@ describe('narrowOverloadCandidates — arity filtering', () => { expect(result.map((d) => d.nodeId)).toEqual(['v:1']); }); - it('falls back to the full overload list when arity filter empties it', () => { + it('returns empty when arity filter empties the set AND every candidate had definite bounds', () => { // argCount=5 doesn't match any overload (none variadic, all have max < 5). + // Post-commit af9af4a9 (PR #1497 / U1): the empty result is now authoritative + // because every rejected candidate had defined `parameterCount` / + // `requiredParameterCount`. The old "always fall back to full list" rescue + // was deliberately removed so resolvers actually drop calls that are + // definitively arity-incompatible (e.g., PHP `f(int $req, ...$rest)` + // called with zero args). const result = narrowOverloadCandidates([add1, add2, add3], 5, undefined); - expect(result.map((d) => d.nodeId)).toEqual(['add:1', 'add:2', 'add:3']); + expect(result.map((d) => d.nodeId)).toEqual([]); }); + + // Note: the `anyUnknownBounds ? overloads : []` branch in + // narrowOverloadCandidates is structurally unreachable in this caller's + // shape — a candidate with both `parameterCount` and `requiredParameterCount` + // undefined always passes the arity filter (neither `argCount > max` nor + // `argCount < min` can fire), so `arityMatches.length` is always > 0 + // whenever `anyUnknownBounds` is true. The branch is preserved in the + // source as a defensive guard for future refactors that might add + // additional rejection criteria in the filter. }); describe('narrowOverloadCandidates — type narrowing', () => { diff --git a/gitnexus/test/unit/scope-resolution/registries.test.ts b/gitnexus/test/unit/scope-resolution/registries.test.ts index b204287b5..2681af69f 100644 --- a/gitnexus/test/unit/scope-resolution/registries.test.ts +++ b/gitnexus/test/unit/scope-resolution/registries.test.ts @@ -250,7 +250,13 @@ describe('Step 5: arity filter', () => { ); }); - it('keeps incompatible candidates when no compatible candidate exists (soft penalty)', () => { + it('drops every candidate when ALL are incompatible AND none unknown (hard rejection)', () => { + // Post-commit af9af4a9 (PR #1497 / U1): the old soft-penalty fallback + // that kept incompatible candidates with `arityMatchIncompatible` + // weight was deliberately removed at this layer too. When every + // candidate is definitively arity-incompatible, the registry returns + // no resolution — matching the PHP variadic case `f(int $req, ...$rest)` + // called with zero args. const save3 = mkDef({ nodeId: 'def:save-three', type: 'Method', @@ -268,8 +274,41 @@ describe('Step 5: arity filter', () => { const results = buildMethodRegistry(ctx).lookup('save', 'scope:m', { callsite: { arity: 1 }, }); - expect(results).toHaveLength(1); - expect(evidenceOfKind(results[0]!, 'arity-match')?.weight).toBe( + expect(results).toHaveLength(0); + }); + + it('keeps incompatible candidates when at least one verdict is unknown (soft penalty)', () => { + // The soft-rescue path is still active when at least one candidate's + // arity verdict is 'unknown' — that signals missing metadata rather + // than a definitive mismatch, so all candidates (including incompatible + // ones) are preserved with their evidence weights for downstream + // tie-breaking. + const save3 = mkDef({ + nodeId: 'def:save-three', + type: 'Method', + qualifiedName: 'User.save', + parameterCount: 3, + }); + const saveUnknown = mkDef({ + nodeId: 'def:save-unknown', + type: 'Method', + qualifiedName: 'User.save', + }); + const mod = mkScope({ + id: 'scope:m', + parent: null, + bindings: { save: [mkBinding(save3, 'local'), mkBinding(saveUnknown, 'local')] }, + }); + const ctx = makeCtx([mod], [save3, saveUnknown], { + arity: (_callsite, def) => (def.nodeId === 'def:save-unknown' ? 'unknown' : 'incompatible'), + }); + const results = buildMethodRegistry(ctx).lookup('save', 'scope:m', { + callsite: { arity: 1 }, + }); + expect(results).toHaveLength(2); + const incompat = results.find((r) => r.def.nodeId === 'def:save-three'); + expect(incompat).toBeDefined(); + expect(evidenceOfKind(incompat!, 'arity-match')?.weight).toBe( EvidenceWeights.arityMatchIncompatible, ); });