Do not report ?? null / ??= null as unnecessary when evaluating the right side has side effects

[phpstan-bot]

Summary

Coalesce operator ?? is unnecessary because the left side is always set and the right side is null. was reported for $this->parseNumber($value) ?? $this->parseSpace($value) where parseSpace() is declared to return null. Deleting the coalesce there is not equivalent: the right side is still evaluated whenever the left side is null, and that call can do work and throw.

The rule now only reports the coalesce as unnecessary when evaluating the right side cannot do anything besides producing its null value.

Changes

• src/Node/CoalesceExpressionNode.php — the node now also carries the ExpressionResult of the right operand (getRightResult()).
• src/Analyser/ExprHandler/CoalesceHandler.php — passes the already-computed $rightResult into the node.
• src/Analyser/ExprHandler/AssignOpHandler.php — passes the already-computed $valueResult into the node for ??=.
• src/Rules/Variables/NullCoalesceRule.php — bails out when the right side has impure points, explicit throw points, a yield, or contains an assignment/increment/decrement.

Analogous cases probed and fixed with the same change (all in tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php):

• ?? and ??= — both operators handled by this rule.
• Right side being a function call, a method call, a static call, an __invoke() call and a closure call returning null.
• Right side containing a plain assignment ($a ?? $x = null). Assignments to local variables are not impure points, so this was a second, independent instance of the same false positive; it is covered by the containsAssign() check, which also covers ??=, =&, ++ and -- on the right side.

Probed and deliberately left reported, because deleting them really is a no-op: literal null, a global constant and a class constant whose value is null, a null variable, a read of a null-typed property, and a call to a @phpstan-pure function returning null.

Root cause

checkUnnecessaryNullCoalesce() reasoned purely about values: if the left side is always set and the right side's type is null, the result of the expression is the same with or without the coalesce. That ignores that ?? is short-circuiting — the right side is evaluated exactly when the left side is null, so deleting the operator also deletes that evaluation. Any right side that is not side-effect free therefore made the report a false positive.

The fix reuses the criterion the codebase already applies when deciding whether an expression statement does nothing (src/Analyser/StmtHandler/ExpressionHandler.php: no impure points, no explicit throw points, and no assignment). To get at it the right side's ExpressionResult, which the two handlers already compute, is now handed to the rule through CoalesceExpressionNode instead of being thrown away.

Test

• tests/PHPStan/Rules/Variables/data/bug-15134.php + NullCoalesceRuleTest::testBug15134() — the reproducer from the issue's playground link, expecting no errors. It fails before the fix with the reported false positive on the ?? line.
• tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php + NullCoalesceRuleTest::testUnnecessaryNullCoalesceSideEffects() — covers the analogous right-side constructs listed above. Before the fix it produces 7 additional false positives (function/method/static/__invoke/closure calls, the assignment right side, and the ??= call right side) on top of the 6 reports that are correct and are kept.
• Existing expectations in NullCoalesceRuleTest (bug-4337, bug-12179, bug-9966, bug-14213) are unchanged — those all use side-effect-free right sides.

Fixes phpstan/phpstan#15134

[VincentLanglet]

1) PHPStan\Rules\Variables\NullCoalesceRuleTest::testUnnecessaryNullCoalesceSideEffects
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 '104: Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.
-111: Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.
 116: Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.
 121: Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.
 128: Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.
 133: Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.
 '

on PHP 7.4

[phpstan-bot]

Fixed and pushed as d7a542881.

Cause of the PHP 7.4 failure: the data file declared the null-returning functions with a native : null return type (PHP 8.2+ syntax). When PHPStan's phpVersion is lower, : null resolves to an object type UnnecessaryNullCoalesceSideEffects\null (a class named null), so pureReturnsNull()'s right side was no longer null and the rule bailed before reaching the "unnecessary" report — hence the missing error on line 111. I reproduced this locally by running the test with phpVersion: 70400 and dumping the right-side type.

Fix: tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php now declares the null return types via /** @return null */ PHPDoc instead of native syntax (the closure just infers null from its body), so all 13 cases exercise what they claim on every analysed PHP version. The // lint >= 8.2 marker is gone since the file is now valid PHP 7.4 syntax, and the expected line numbers in NullCoalesceRuleTest::testUnnecessaryNullCoalesceSideEffects() were shifted accordingly.

bug-15134.php keeps the verbatim reproducer from the issue's playground link (native : null, // lint >= 8.2); it expects no errors, so it passes on lower versions too.

Verification:

• NullCoalesceRuleTest — 44/44 pass on PHP 8.4, and with phpVersion: 70400 forced both new tests pass (the 3 other failures there are pre-existing property-hook syntax cases that my synthetic 7.4 harness can't represent — real 7.4 CI runs downgraded code).
• Reverting only NullCoalesceRule.php to its pre-fix state still makes the rewritten data file fail for the right reason: 7 extra false positives (lines 63, 68, 73, 78, 89, 94, 103) plus the bug-15134 false positive.
• make lint clean, make phpstan no errors, make tests 21353 tests OK (97 skipped, as before).

[staabm]

these look untested

[phpstan-bot]

Addressed and pushed as e56caaa5c.

staabm was right — only the Assign branch of containsAssign() was exercised (by $foo->stringOrNull ?? $x = null on line 94). The other six node kinds had no coverage.

What I added to tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php — one right side per remaining node kind, each expecting no error:

Node kind	Right side	Line
AssignRef	$ref = &$null	147
AssignOp	$nulls[$i += 1]	153
PostInc	$nulls[$i++]	159
PreInc	$nulls[++$i]	165
PostDec	$nulls[$i--]	171
PreDec	$nulls[--$i]	177

The four inc/dec cases put the operator inside an array offset on an array<int, null> rather than writing $null++ directly: ++/-- applied to a null variable is deprecated since PHP 8.3 and would be testing a construct nobody writes, while the offset form keeps the right side's type null (verified with \PHPStan\dumpType() — all six infer as null, so each actually reaches the containsAssign() check rather than bailing earlier on the type test). AssignOp needed the same treatment because the only AssignOp that evaluates to null on its own is ??=, which would have nested a second coalesce into the case.

Verification that each branch is load-bearing, not just that the tests pass:

• With || $this->containsAssign($right) removed entirely, all 7 assignment cases report the false positive (94, 147, 153, 159, 165, 171, 177); the 6 legitimate reports are unaffected.
• Removing each instanceof clause one at a time makes exactly one case regress — Assign→94, AssignRef→147, AssignOp→153, PostInc→159, PreInc→165, PostDec→171, PreDec→177. No clause is redundant, and no case covers two clauses.

NullCoalesceRuleTest 44/44, make tests 21353 OK (97 skipped, unchanged), make phpstan no errors, make lint clean. The rule source is untouched by this commit; the expected-error list in the test is unchanged since every new case expects no error.

[staabm]

the 7.4 error is pre-existent

1) PHPStan\Rules\Arrays\NonexistentOffsetInArrayDimFetchRuleTest::testBug7905
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'
+'15: Cannot access offset mixed on array<string, string>|Bug7905Rule\null.
 '

/home/runner/work/phpstan-src/phpstan-src/src/Testing/RuleTestCase.php:224
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php:359
/home/runner/work/phpstan-src/phpstan-src/tests/vendor/phpunit/phpunit/src/TextUI/Command.php:146