Report ?? null / ??= null on an always-set left side as an unnecessary null coalesce#5865
Report ?? null / ??= null on an always-set left side as an unnecessary null coalesce#5865phpstan-bot wants to merge 5 commits into
?? null / ??= null on an always-set left side as an unnecessary null coalesce#5865Conversation
c6159f0 to
2636415
Compare
staabm
left a comment
There was a problem hiding this comment.
It fixes more open issues which might need tests
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression test for
|
Done. I addressed the reviewer's request to add non-regression tests for issues #12179 and #9966. What I added
Verification
Committed as One note for the reviewer: I built the reproducers from the issue bodies (which include the exact array shape for #9966 and the "always-defined nullable variable" description for #12179), since the playground links are JS-rendered and weren't fetchable here. |
…ssary null coalesce - Add a `unnecessaryNullCoalesce` feature toggle (off by default, enabled in bleedingEdge.neon). - In `NullCoalesceRule`, after the existing isset-based check, report a `nullCoalesce.unnecessary` error when the right side of `??`/`??=` is always `null` and the left side is always set (so the coalesce can never change the result). "Always set" is determined via `MutatingScope::issetCheck()` with a trivial callback, which correctly handles function/method/static calls, defined nullable variables, always-set nullable properties and array offsets, etc. - Clean up the redundant `?? null` instances this surfaced in PHPStan's own source (`NodeScopeResolver`, `FileTypeMapper`, `Php8SignatureMapProvider`), mostly `array_last(...) ?? null` and a few always-initialized locals.
…alues Closes phpstan/phpstan#12179 Closes phpstan/phpstan#9966 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b7a4dac to
badef10
Compare
Summary
hello1($name) ?? nullis equivalent tohello1($name): a??(or??=) with anullfallback only does something when the left side can be undefined. When the left side is always set, the coalesce never changes the result and is redundant. PHPStan did not report this. This change adds detection for it.Changes
conf/config.neon,conf/bleedingEdge.neon,conf/parametersSchema.neon: newfeatureToggles.unnecessaryNullCoalesce(false by default, true under bleeding edge).src/Rules/Variables/NullCoalesceRule.php: after the existingIssetCheckpass, runcheckUnnecessaryNullCoalesce(). It reportsnullCoalesce.unnecessarywhen:null(isNull()->yes()), andMutatingScope::issetCheck($left, static fn (): bool => true) === true.?? nullinstances surfaced by the new rule in PHPStan's own code:src/Analyser/NodeScopeResolver.php(array_last($stmt->cond) ?? null,$parameterType ?? null×2)src/Type/FileTypeMapper.php(severalarray_last(...) ?? null)src/Reflection/SignatureMap/Php8SignatureMapProvider.php($phpDocReturnType ?? null)tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php: pass the toggle, addtestBug4337, and account for the extra (correct) report intestBug14213.Root cause
This is not a regression but a missing check. The existing
NullCoalesceRule/IssetCheckonly reported the left side being "not nullable" or "always null". The third redundant shape — left always set but nullable, right null — produced no diagnostic becauseIssetCheck::check()returnsnullfor that case. The fix reuses the already-existing "always set" traversal (MutatingScope::issetCheck) and only adds the new diagnostic when the right side is itself alwaysnull.Analogous cases
All of the following go through the single
issetCheck(..., fn () => true)"always set" probe and are covered by one code path; each is exercised intests/.../data/bug-4337.php:hello1($name) ?? null(the reported case)$foo->method() ?? nullFoo::staticMethod() ?? null$x ?? null$foo->stringOrNull ?? nullFoo::$staticStringOrNull ?? null$arr['a'] ?? null$x ??= nullNegative cases verified to not report: maybe-undefined variable, maybe-undefined array offset, and any
??/??=whose right side is not null. Nullsafe access ($obj?->prop ?? null) is already handled by the existingIssetChecknullsafe branch, so it short-circuits before the new check and does not double-report.Test
NullCoalesceRuleTest::testBug4337analysesdata/bug-4337.php(copied from the issue's example plus the analogous constructs) and asserts the eight expectednullCoalesce.unnecessaryreports plus the three negative cases. Verified failing before the fix (no errors) and passing after. The full suite,make phpstan,make cs-fixall pass.Fixes phpstan/phpstan#4337
Fixes phpstan/phpstan#12179
Fixes phpstan/phpstan#9966