feat: ForbidSentinelFallbackOnNarrowingHelperRule (psr-0002) - #61
feat: ForbidSentinelFallbackOnNarrowingHelperRule (psr-0002)#61dmooibroek wants to merge 2 commits into
Conversation
Flags a literal sentinel fallback (?? '', ?? 0, ?: 'unknown') on the result of a narrowing helper — a first-party boundary helper taking mixed external data and returning a nullable scalar, where null means 'unreadable input'. The sentinel converts that failure signal into a plausible value that gets persisted or unlocks branches. Shape-keyed, never a name list: fires only when the resolved reflection has a configured-namespace declarer (narrowingHelperNamespacePrefixes, default ['App']), at least one MixedType param, and a nullable-scalar(-union) return checked via the Type API. ?? null, property/offset coalesces, non-literal fallbacks, and vendor calls never fire. Seed: tc-api PR #360 — Carbon::parse(null) DOB, ?? '' forceDelete unlock, empty-SET gender write.
dmooibroek
added
the
Agent Review Requested
There was a problem hiding this comment.
PR adds ForbidSentinelFallbackOnNarrowingHelperRule to catch literal sentinel fallbacks on narrowing helpers; headline concern: its own README documents ?? null as a caught error but the rule deliberately never flags that shape, so the doc overclaims coverage of the seed bug that motivated it.
Suggestions (non-blocking)
Standards-backed, below the gate. Not blocking this PR.
src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php:196-212(reviewer-correctness) — NullsafeMethodCall branch can only fire on a non-nullable receiver — advertised support is effectively dead and untestedsrc/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php:312-321(reviewer-correctness) —mixedparameter gate accepts amixed $defaultlookup parameter, where?? ''is idiomatic rather than a bug
| A **narrowing helper** is a boundary helper: `mixed` external data in (an XML leaf, a CSV cell, a decoded JSON node), a nullable scalar out, where `null` means "this input was unreadable". A sentinel fallback throws that signal away: | ||
|
|
||
| ```php | ||
| $dob = Carbon::parse($this->text($leaf) ?? null); // ERROR — a missing node parses as now() |
There was a problem hiding this comment.
[medium] README claims ?? null sentinel is flagged as ERROR, rule never fires on it
README's new example marks $dob = Carbon::parse($this->text($leaf) ?? null); // ERROR as caught, but describeSentinel() returns null for a literal null ConstFetch (src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php:189) and processNode() bails at line 152 before any reflection — the rule never emits this error. The class docblock and tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullFallback.php confirm ?? null is deliberately NOT flagged, contradicting the README. This is seed bug #1 of the three tc-api #360 bugs cited as motivation (docblock 48-54, CHANGELOG) — consumers adopting the rule on the README's word believe this exact shape is guarded when it is not. Fix: move the ?? null example out of the ERROR block in README (list it under never-flagged) and clarify in CHANGELOG that seed bug #1 is out of scope, or extend the rule to cover the real sink if intended.
There was a problem hiding this comment.
Fixed in e1c4ccd. Carbon::parse(text($leaf) ?? null) moved out of the ERROR block into a never-flagged block; README prose, table row, CHANGELOG and class docblock all reframe seed bug #1 as motivation-but-out-of-scope — no literal sentinel, the null is swallowed by the downstream sink, catching it needs sink-aware analysis.
Both non-blocking suggestions also handled in e1c4ccd:
- mixed-param gate now counts only REQUIRED mixed params —
get(string $key, mixed $default = null): ?string+?? ''silent (fixture OptionalMixedDefaultHelper, control-run verified); residual FP (required mixed key + default param) documented with inline-ignore escape hatch. - Nullsafe dead-branch: refuted by instrumented probe on phpstan 2.2.7 — the coalesce left operand is analysed null-narrowed before processNode(), so every nullable-receiver shape (promoted property, param, chained ?->, array offset, under both ?? and ?:) fires with the unmodified rule. Adding removeNull() would create an unreachable branch the mutation gate (--min-msi=75) punishes. Shipped the missing coverage instead: NullsafeCallCoalesce fixture fires at :21.
Fixture-dir fire count 9/9 intended, all negatives silent, level-max clean on src. PHPUnit/coverage still gated on the commonmark audit fix reaching main.
…ullsafe coverage - README/CHANGELOG/docblock: Carbon::parse(text() ?? null) moved to never-flagged — no literal sentinel, the null is swallowed by a downstream sink; seed bug #1 reframed motivation-but-out-of-scope. - hasMixedParameter() counts only REQUIRED mixed params: a lookup's optional mixed $default is the caller's own default, not external data. Fixture OptionalMixedDefaultHelper silent; residual FP documented with inline-ignore escape hatch. - Nullsafe 'dead branch' finding refuted by instrumented probe: PHPStan null-narrows the coalesce left operand before processNode, all nullable- receiver shapes fire unmodified. No null-stripping added (unreachable branch would trip the mutation gate); NullsafeCallCoalesce fixture pins coverage.
Goosterhof
left a comment
Goosterhof
left a comment
There was a problem hiding this comment.
🔴 Majors 🔴 Majors
1 major · 0 blockers · 0 minors · 2 nits · 0 praise · 2 inline
New rule ForbidSentinelFallbackOnNarrowingHelperRule (psr-0002) — shape-keyed detection of a literal sentinel fallback on a first-party mixed-in/nullable-scalar-out helper. The rule logic itself reads carefully-reasoned (namespace-boundary matching, required-vs-optional mixed param split, NeverType bottom-type guard on the return check) and the ally's two prior findings on this thread were both addressed and resolved at this HEAD — nothing to re-litigate there.
Cross-file findings
ci-passedis failing, and the failure blocks every gate that would actually verify this PR's own new code —Audit dependenciesfails beforeFormat check/Static analysis (self)/Tests with coverage/Coverage threshold gate/Mutation testingever run.- Trace:
gh pr checks 61→check (8.4)fail,check (8.5)fail,ci-passedfail — all three are required (branch-protection matrix legs + rollup).- Job log (
Audit dependenciesstep, run 31254839986) →league/commonmarkhigh/medium-severity advisories reported 2026-08-06 (PKSA-mc58-w91n-f5gv,CVE-2026-71488,CVE-2026-71478) — every subsequent step in the job (Format check,Static analysis (self),Tests with coverage,Coverage threshold gate,Mutation testing) shows-(skipped). - This is NOT this PR's regression: sibling PR #62
chore(deps): league/commonmark 2.9.0 — clear composer audit, opened the same day, targets exactly this advisory and is green — confirms the break is a fleet-wide dependency-audit event onmain, not somethingpsr-0002introduced.
- Consequence: the 229-line test suite + 15 fixtures this PR ships — including the two exact scenarios the ally's review already probed (the
NullsafeMethodCallnarrowing claim atsrc/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php:203-209, and the required-vs-optionalmixedsplit) — have never been machine-verified. The PR body itself says "local PHPUnit blocked… CI is authoritative for the suite + 83% coverage gate" — but CI never reaches that stage on this head. The--min-msi=75mutation gate and the 83% coverage gate are both unexercised too. - Fix: rebase onto
mainonce #62 merges (or cherry-pick the commonmark bump onto this branch) soci-passedactually reaches — and clears — the rule's own test/mutation/coverage gates before this merges. - Per the CI verdict cap this caps the review at Major / COMMENT, not approve-worthy, regardless of the code read.
- Trace:
Findings (detail inline)
- 🔴 (see Cross-file findings above — un-anchorable, spans the Actions run + sibling PR, not a single diff line)
- ⚪ src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php:207 · ⚪ src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php:325
Automated war-room agent review — posted because this PR carries the Agent Review Requested label.
| } | ||
|
|
||
| if ($expr instanceof ConstFetch) { | ||
| return $expr->name->toLowerString() === 'null' ? null : $expr->name->toString(); |
There was a problem hiding this comment.
⚪ describeSentinel()'s ConstFetch branch treats ANY non-null constant fetch as a literal sentinel ($expr->name->toString()), not just true/false as the docblock (ForbidSentinelFallbackOnNarrowingHelperRule.php:186 — "true / false, a class constant") describes. A bare global constant fallback (?? SOME_APP_DEFAULT) also fires under this branch, which is arguably in-spirit (still a fixed plausible-looking value) but is undocumented scope. Not blocking — worth a one-line doc note or a deliberate ConstFetch-name allowlist (true/false only) if the wider match wasn't intended.
| private function isFirstParty(string $owner): bool | ||
| { | ||
| foreach ($this->narrowingHelperNamespacePrefixes as $prefix) { | ||
| if (str_starts_with($owner, mb_rtrim($prefix, '\\') . '\\')) { |
There was a problem hiding this comment.
⚪ mb_rtrim($prefix, '\\') — the trimmed character is a single-byte ASCII backslash; plain rtrim() is equivalent here and cheaper (no multibyte-safe scanning needed). mb_rtrim earns its keep when the haystack/needle can be multibyte, which a namespace-prefix trim never is.
2 participants
New rule: flag a literal sentinel fallback (
?? '',?? 0,?: 'unknown',?? false,?? [],?? Foo::BAR) on the result of a narrowing helper — a first-party boundary helper with amixedparam and a nullable-scalar return, where null means "unreadable input". The sentinel converts the failure signal into a plausible value that gets persisted or unlocks branches.Seed: tc-api #360 (TC-0390 typing campaign) — three confirmed production bugs from this one shape:
Carbon::parse(text($leaf) ?? null)wrote today as a date of birth;?? ''seeded['']which read non-empty and unlocked aforceDelete()wipe; gender persisted as empty SET string.Detection is shape-keyed, not a name list — resolved reflection must satisfy: declarer under
narrowingHelperNamespacePrefixes(new param, default['App'], boundary-aware matching), ≥1MixedTypeparam (explicit or untyped), nullable-scalar(-union) return via Type API (containsNull+ per-member scalar probe,NeverTyperejected).Never fires:
?? null(preserves the signal — the remediation), property/array-offset coalesces, non-literal fallbacks, vendor/builtin calls, non-nullable or nullable-object returns. Deliberate misses (ADR-0021: FN acceptable, FP not): long ternary with explicit null test, variable-laundered results.Verification: local PHPUnit blocked (no vendor on the authoring host); rule exercised via live PHPStan 2.2.7 runs over the fixture dir through the shipped
extension.neon— 8/8 intended sites fire, all 8 negative fixtures silent,%param%path proven with a second prefix,level: maxclean on the rule source. CI is authoritative for the suite + 83% coverage gate.Candidate MAJOR per CHANGELOG (surfaces new errors in already-clean consumers); not tagged, release ally-gated.
Refs: psr-0002