Do not narrow the left side of ?? to non-null when the coalesce context still permits a falsey value#6000
Conversation
…text still permits a falsey value
- `CoalesceHandler::specifyTypes()` narrows the left operand of `??` to
non-null (i.e. memorizes the offset/variable as existing) when the
coalesce is truthy and the right operand is falsey. That branch was
also reached for the "not false" context produced by `=== false` /
`!== false` (`createFalse()->negate()`), which still permits the
coalesce to be null (a missing offset yields null, and null !== false).
- Guard that narrowing with `!$context->falsey()` so it only fires when
the context guarantees a non-null (truthy) result, matching the
existing isset-guarded falsey branch.
- Fixes the false-positive `isset.offset` and `nullCoalesce.offset`
"always exists and is not nullable" errors after
`if (($arr[$key] ?? null) === false) { return; }`.
- Also fixes the analogous `!== false` (true branch) case and coalesces
with a non-null but falsey right operand (`?? 0`); the loose `==`/`!=`
variants were verified to remain correct, because `null == false` is
true and therefore does legitimately imply the offset exists.
|
|
||
| if ((new ConstantBooleanType(false))->isSuperTypeOf($scope->getType($expr->right)->toBoolean())->yes()) { | ||
| if ( | ||
| !$context->falsey() |
There was a problem hiding this comment.
I always mix up ->false and ->falsey.
would be great to also have tests for ==false, ==true
There was a problem hiding this comment.
Done. Here's what I did to address staabm's review:
Review point 1: "I always mix up ->false and ->falsey"
No code change needed — the guard correctly uses ->falsey(). I verified this empirically: reverting the guard makes the strict testBug13488 fail (it reports the false-positive isset.offset errors), and restoring it passes. The !$context->falsey() check is what blocks the createFalse()->negate() ("anything but false") context, which is exactly the buggy narrowing path. ->false() would be the wrong method here.
Review point 2: "would be great to also have tests for ==false, ==true"
Added a new fixture tests/PHPStan/Rules/Variables/data/bug-13488-loose.php covering all four loose variants, plus testBug13488Loose() in both IssetRuleTest and NullCoalesceRuleTest. I determined the correct expectations empirically rather than guessing:
| Condition (true branch) | Matches | Offset must exist? | Errors expected |
|---|---|---|---|
($foo[$k] ?? null) == false |
false or null (missing) |
no | none |
($foo[$k] ?? null) == true |
true only |
yes | isset.offset + nullCoalesce.offset |
($foo[$k] ?? null) != false |
true only |
yes | isset.offset + nullCoalesce.offset |
($foo[$k] ?? null) != true |
false or null (missing) |
no | none |
This documents that loose comparison narrowing is correct (because null == false), which is why the fix intentionally left the ==/!= path untouched — the contrast the reviewer wanted to see spelled out.
Verification
testBug13488(strict) andtestBug13488Loose(loose) — 4/4 pass.- Full
IssetRuleTest+NullCoalesceRuleTest— 66/66 pass. make phpstan— no errors.
Committed as a separate logical change and pushed to the PR branch.
Loose == false / != true keep a coalesced offset possibly-missing (null == false), while == true / != false imply it exists. Cover all four variants for both the isset() and ?? left-side offset rules. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
After a check like
if (($arr[$key] ?? null) === false) { return; }, PHPStan wrongly memorized$arr[$key]as always existing. This produced false-positiveisset.offsetandnullCoalesce.offseterrors ("Offset … always exists and is not nullable") on subsequentisset($arr[$key])/$arr[$key] ?? …uses, even though$keymay not be present in the array. This is a follow-up to #11708, which only fixed thepreg_matchvariant.The fix teaches
CoalesceHandlernot to narrow the left side of??to non-null when the surrounding condition only proves the coalesce is not exactlyfalse— because a missing offset producesnull, andnull !== false.Changes
src/Analyser/ExprHandler/CoalesceHandler.php: guarded the branch that narrows$expr->leftto non-null with!$context->falsey(). That branch previously fired for any context carrying theTRUEbit, including thecreateFalse()->negate()("anything butfalse") context, which still permits anullresult.tests/PHPStan/Rules/Variables/data/bug-13488.php: regression fixture based on the issue's playground sample, extended with the analogous!== falseand falsey-right-operand (?? 0) cases.tests/PHPStan/Rules/Variables/IssetRuleTest.phpandNullCoalesceRuleTest.php: addedtestBug13488()expecting no errors (both rules were affected by the same wrong narrowing).Root cause
EqualityTypeSpecifyingHelper::specifyTypesForConstantBinaryExpression()translates$x === falsebeing false into a truthy-style narrowing by callingspecifyTypesInCondition($x, createFalse()->negate()). That context (0b1011) means "anything butfalse" and correctly yields no narrowing for ordinary expressions via the default handler. However,CoalesceHandler::specifyTypes()treated any context with theTRUEbit as permission to stripnullfrom the??left operand (and thus mark the offset/variable as set). For$arr[$key] ?? null, a missing offset makes the whole expressionnull, which satisfies!== false, so the offset cannot be assumed to exist.The pattern — "not-
falsedoes not imply not-null/truthy" — affects every strict comparison that produces the not-falsecontext:($arr[$key] ?? null) === false(reported case, false branch)($arr[$key] ?? null) !== false(true branch)($arr[$key] ?? 0) === falseand any other falsey right operand of??All three are fixed by the single guard. The loose
==/!=variants were probed and are intentionally left untouched:null == falseistrue, so($arr[$key] ?? null) != falsegenuinely does imply the offset exists.Test
IssetRuleTest::testBug13488andNullCoalesceRuleTest::testBug13488run the reproducer at level withtreatPhpDocTypesAsCertainand assert no errors. Both fail on the unfixed tree (they report theisset.offset/nullCoalesce.offsetfalse positives) and pass with the fix.!== falseand?? 0cases, which are false positives before the fix and clean afterwards.Fixes phpstan/phpstan#13488