-
Notifications
You must be signed in to change notification settings - Fork 589
Do not report ?? null / ??= null as unnecessary when evaluating the right side has side effects
#6418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
phpstan-bot
wants to merge
3
commits into
phpstan:2.2.x
Choose a base branch
from
phpstan-bot:create-pull-request/patch-himiway
base: 2.2.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Do not report ?? null / ??= null as unnecessary when evaluating the right side has side effects
#6418
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
9fce02f
Do not report `?? null` / `??= null` as unnecessary when evaluating t…
phpstan-bot 4d16ac0
Make the null-coalesce side-effect test data PHP version independent
phpstan-bot 440f81a
Cover every node kind checked by NullCoalesceRule::containsAssign()
phpstan-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <?php // lint >= 8.2 | ||
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug15134; | ||
|
|
||
| use LogicException; | ||
|
|
||
| interface Node {} | ||
|
|
||
| class A implements Node {} | ||
|
|
||
| abstract class Parser { | ||
|
|
||
| protected function parseExpressionChild(bool $value): ?Node { | ||
| return $this->parseNumber($value) | ||
| ?? $this->parseSpace($value); | ||
| } | ||
|
|
||
| abstract protected function parseNumber(bool $value): ?A; | ||
|
|
||
| protected function parseSpace(bool $value): null { | ||
| if ($value === false) { | ||
| throw new LogicException('The string is not a mathematical expression.'); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| } |
178 changes: 178 additions & 0 deletions
178
tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace UnnecessaryNullCoalesceSideEffects; | ||
|
|
||
| use LogicException; | ||
|
|
||
| const NULL_CONSTANT = null; | ||
|
|
||
| /** @return null */ | ||
| function returnsNull(bool $value) | ||
| { | ||
| if ($value === false) { | ||
| throw new LogicException('nope'); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * @phpstan-pure | ||
| * @return null | ||
| */ | ||
| function pureReturnsNull() | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| class Foo | ||
| { | ||
|
|
||
| public const NULL_CONSTANT = null; | ||
|
|
||
| /** @var string|null */ | ||
| public $stringOrNull = null; | ||
|
|
||
| /** @var null */ | ||
| public $alwaysNull = null; | ||
|
|
||
| /** @return null */ | ||
| public function returnsNull() | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| /** @return null */ | ||
| public static function staticReturnsNull() | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| /** @return null */ | ||
| public function __invoke() | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| function funcCallOnRightSide(Foo $foo, ?string $name): ?string | ||
| { | ||
| return $foo->stringOrNull ?? returnsNull($name !== null); | ||
| } | ||
|
|
||
| function methodCallOnRightSide(Foo $foo): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $foo->returnsNull(); | ||
| } | ||
|
|
||
| function staticCallOnRightSide(Foo $foo): ?string | ||
| { | ||
| return $foo->stringOrNull ?? Foo::staticReturnsNull(); | ||
| } | ||
|
|
||
| function invokeOnRightSide(Foo $foo): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $foo(); | ||
| } | ||
|
|
||
| function closureCallOnRightSide(Foo $foo): ?string | ||
| { | ||
| $closure = static function () { | ||
| echo 'side effect'; | ||
|
|
||
| return null; | ||
| }; | ||
|
|
||
| return $foo->stringOrNull ?? $closure(); | ||
| } | ||
|
|
||
| function assignOnRightSide(Foo $foo): ?string | ||
| { | ||
| $result = $foo->stringOrNull ?? $x = null; | ||
| echo $x; | ||
|
|
||
| return $result; | ||
| } | ||
|
|
||
| function assignOpOnRightSide(Foo $foo, ?string $name): ?string | ||
| { | ||
| $x = $name; | ||
| $x ??= $foo->returnsNull(); | ||
|
|
||
| return $x; | ||
| } | ||
|
|
||
| function assignOpPureOnRightSide(?string $name): ?string | ||
| { | ||
| $x = $name; | ||
| $x ??= null; | ||
|
|
||
| return $x; | ||
| } | ||
|
|
||
| function pureFuncCallOnRightSide(Foo $foo): ?string | ||
| { | ||
| return $foo->stringOrNull ?? pureReturnsNull(); | ||
| } | ||
|
|
||
| function constantOnRightSide(Foo $foo): ?string | ||
| { | ||
| return $foo->stringOrNull ?? NULL_CONSTANT; | ||
| } | ||
|
|
||
| function classConstantOnRightSide(Foo $foo): ?string | ||
| { | ||
| return $foo->stringOrNull ?? Foo::NULL_CONSTANT; | ||
| } | ||
|
|
||
| function nullVariableOnRightSide(Foo $foo): ?string | ||
| { | ||
| $null = null; | ||
|
|
||
| return $foo->stringOrNull ?? $null; | ||
| } | ||
|
|
||
| function nullPropertyOnRightSide(Foo $foo, Foo $bar): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $bar->alwaysNull; | ||
| } | ||
|
|
||
| function assignRefOnRightSide(Foo $foo): ?string | ||
| { | ||
| $null = null; | ||
|
|
||
| return $foo->stringOrNull ?? $ref = &$null; | ||
| } | ||
|
|
||
| /** @param array<int, null> $nulls */ | ||
| function assignOpInOffsetOnRightSide(Foo $foo, array $nulls, int $i): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $nulls[$i += 1]; | ||
| } | ||
|
|
||
| /** @param array<int, null> $nulls */ | ||
| function postIncOnRightSide(Foo $foo, array $nulls, int $i): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $nulls[$i++]; | ||
| } | ||
|
|
||
| /** @param array<int, null> $nulls */ | ||
| function preIncOnRightSide(Foo $foo, array $nulls, int $i): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $nulls[++$i]; | ||
| } | ||
|
|
||
| /** @param array<int, null> $nulls */ | ||
| function postDecOnRightSide(Foo $foo, array $nulls, int $i): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $nulls[$i--]; | ||
| } | ||
|
|
||
| /** @param array<int, null> $nulls */ | ||
| function preDecOnRightSide(Foo $foo, array $nulls, int $i): ?string | ||
| { | ||
| return $foo->stringOrNull ?? $nulls[--$i]; | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these look untested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed and pushed as
e56caaa5c.staabm was right — only the
Assignbranch ofcontainsAssign()was exercised (by$foo->stringOrNull ?? $x = nullon 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:AssignRef$ref = &$nullAssignOp$nulls[$i += 1]PostInc$nulls[$i++]PreInc$nulls[++$i]PostDec$nulls[$i--]PreDec$nulls[--$i]The four inc/dec cases put the operator inside an array offset on an
array<int, null>rather than writing$null++directly:++/--applied to anullvariable is deprecated since PHP 8.3 and would be testing a construct nobody writes, while the offset form keeps the right side's typenull(verified with\PHPStan\dumpType()— all six infer asnull, so each actually reaches thecontainsAssign()check rather than bailing earlier on the type test).AssignOpneeded the same treatment because the onlyAssignOpthat evaluates tonullon 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:
|| $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.instanceofclause 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.NullCoalesceRuleTest44/44,make tests21353 OK (97 skipped, unchanged),make phpstanno errors,make lintclean. The rule source is untouched by this commit; the expected-error list in the test is unchanged since every new case expects no error.