Narrow the right side of ??= with falsey isset() instead of !== null - #6145
Narrow the right side of ??= with falsey isset() instead of !== null#6145phpstan-bot wants to merge 1 commit into
??= with falsey isset() instead of !== null#6145Conversation
…null`
- `AssignOpHandler::processExpr()` built the scope for the right-hand side of `??=` with `filterByFalseyValue(new NotIdentical($var, null))`, i.e. it asserted `$var === null`. That is only one half of the `??=` condition: the right side also runs when `$var` is not set at all.
- Asserting `$var === null` on a target that can never hold `null` collapsed the whole containing type to `never`, so anything else read on the right side was analysed on a `*NEVER*` value (e.g. `Offset 'bar' on *NEVER* ...`) or reported as unnecessary.
- Replaced the condition with `filterByFalseyValue(new Isset_([$var]))`, which is exactly the `??=` short-circuit condition and matches what `CoalesceHandler::resolveType()` already uses for `??`.
- The single change fixes the whole family of left-hand-side forms, each covered by a new test: optional array offsets (`array{foo?: string}`), non-constant arrays with a constant key (`array<string, string>`, previously `non-empty-array&hasOffsetValue('foo', *NEVER*)`), nested offsets, non-nullable instance properties, non-nullable static properties, array offsets on a property, and undefined variables (`$undefined ??= $undefined ?? 1` reported "always exists and is always null" instead of "is never defined").
- Nullable targets keep the old, more precise narrowing to `null`, except for `array<string, T|null>` where the value is now `T|null` on the right side — consistent with what plain `??` already infers there.
- Probed the sibling constructs and left them unchanged: `CoalesceHandler` (`??`), `IssetHandler`, `EmptyHandler` and the nullsafe handlers already use the correct condition; the nullsafe handlers' `!== null` truthy filter is the right semantics for `?->`.
staabm
left a comment
There was a problem hiding this comment.
Is offset access with ?? and ArrayAccess or ArrayObject covered in tests?
|
While that finishes, here's a summary of what I've done so far. The reviewer's question was well-founded —
Same root cause as the reported bug: for At rule level there's also a bogus error, when the same offset appears on both sides: That message is exactly backwards — the inner Tests added (each verified failing before the fix, passing after):
Two notes on things I hit along the way:
|
staabm
left a comment
There was a problem hiding this comment.
add those tests for ArrayAccess and ArrayObject
Summary
$data['foo'] ??= $data['bar'] ?? null;onarray{foo?: string, bar?: string}reported a bogus error —Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.withtreatPhpDocTypesAsCertain: false, andOffset 'bar' on *NEVER* on left side of ?? always exists and is not nullable.with it enabled. Both messages come from the same cause: while analysing the right-hand side of??=, PHPStan narrowed$datadown to*NEVER*.The fix builds the right-hand-side scope from the actual
??=short-circuit condition (!isset($var)) instead of from$var === null.Changes
src/Analyser/ExprHandler/AssignOpHandler.php— inprocessExpr(), the scope for$expr->expris now$scope->filterByFalseyValue(new Expr\Isset_([$expr->var]))instead of$scope->filterByFalseyValue(new BinaryOp\NotIdentical($expr->var, new ConstFetch(new Name('null')))). Removed the now-unusedConstFetch/Nameimports.tests/PHPStan/Rules/Variables/data/bug-15021.php+NullCoalesceRuleTest::testBug15021()— the reproducer from the issue, expecting no errors.tests/PHPStan/Rules/Variables/data/null-coalesce-assign-right-side-scope.php+NullCoalesceRuleTest::testNullCoalesceAssignRightSideScope()— the analogous left-hand-side forms.tests/PHPStan/Analyser/nsrt/bug-15021.php— type-level assertions for every form.Analogous cases fixed by the same change (each has a failing-before test):
array{foo?: string, bar?: string}, the reported casearray<string, string>, previouslynon-empty-array<string, string>&hasOffsetValue('foo', *NEVER*)array<string, string|null>, right side sawnull, nowstring|null(matching plain??)$data['a']['b'] ??= …onarray{a?: array{b?: string, c?: string}}$foo->nonNullable ??= …Foo::$staticNonNullable ??= …$foo->data['foo'] ??= …$undefined ??= $undefined ?? 1reportedVariable $undefined on left side of ?? always exists and is always null., now correctly… is never defined.Sibling constructs probed and found already correct, so left untouched:
src/Analyser/ExprHandler/CoalesceHandler.php— bothresolveType()andprocessExpr()derive the right-side scope from theisset/coalesce condition, not from!== nullsrc/Analyser/ExprHandler/IssetHandler.php,src/Analyser/ExprHandler/EmptyHandler.php— already model "not set or null"src/Analyser/ExprHandler/NullsafePropertyFetchHandler.php,NullsafeMethodCallHandler.php— these use a truthy!== nullfilter, which is the correct semantics for?->(the object must be non-null), not a coalesce-style short circuitRoot cause
??=evaluates its right-hand side when the target is either unset or null.AssignOpHandlermodelled only the second half by filtering the scope with the falsey value of$var !== null, i.e. asserting$var === null.For every target whose type cannot hold
null— an optional array offset typedstring, a non-nullable property, anarray<string, string>value — that assertion is unsatisfiable, soTypeSpecifierpropagated the contradiction outwards and turned the containing expression intonever: the whole$dataarray, the whole object, etc. Every rule and type query on the right-hand side then operated on*NEVER*, producing either cascading nonsense messages (Offset 'bar' on *NEVER' …) or, when PHPDoc types were not treated as certain, the "coalesce is unnecessary" verdict.Using
filterByFalseyValue(new Isset_([$var]))states the real condition.IssetHandler::specifyTypes()already knows how to narrow!isset(...)conservatively — it only removes a constant array when the offset is definitely set and definitely non-null (which is genuinely unreachable code), and it leaves possibly-missing offsets alone. This is also the exact conditionCoalesceHandler::resolveType()uses for the right side of??, so??=and??now agree.Test
NullCoalesceRuleTest::testBug15021()— verbatim reproducer from the issue's playground link; expects no errors. Fails before the fix withOffset 'bar' on *NEVER* on left side of ?? always exists and is not nullable.NullCoalesceRuleTest::testNullCoalesceAssignRightSideScope()— covers property, static property, property offset, nested offset, non-constant array and undefined-variable targets. Before the fix it produced two extraOffset … on *NEVER*errors and the wrong message for the undefined variable.tests/PHPStan/Analyser/nsrt/bug-15021.php—assertType()on the right-hand-side scope for all of the above; 7 assertions fail before the fix (5 of them*NEVER*).Full
make tests,make phpstanandmake cs-fixare green.Fixes phpstan/phpstan#15021