Skip to content

Commit b20c608

Browse files
phpstan-botVincentLangletclaudestaabm
authored
Report ?? null / ??= null on an always-set left side as an unnecessary null coalesce
Co-authored-by: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Co-authored-by: Vincent Langlet <vincentlanglet@hotmail.fr> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Markus Staab <maggus.staab@googlemail.com>
1 parent 371b67d commit b20c608

11 files changed

Lines changed: 282 additions & 23 deletions

File tree

‎conf/bleedingEdge.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,5 @@ parameters:
2222
checkDynamicConstantNameValues: true
2323
unusedLabel: true
2424
newOnNonObject: true
25+
unnecessaryNullCoalesce: true
2526
finiteTypesInHaystack: true

‎conf/config.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ parameters:
4848
checkDynamicConstantNameValues: false
4949
unusedLabel: false
5050
newOnNonObject: false
51+
unnecessaryNullCoalesce: false
5152
finiteTypesInHaystack: false
5253
fileExtensions:
5354
- php

‎conf/parametersSchema.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ parametersSchema:
5151
checkDynamicConstantNameValues: bool()
5252
unusedLabel: bool()
5353
newOnNonObject: bool()
54+
unnecessaryNullCoalesce: bool()
5455
finiteTypesInHaystack: bool()
5556
])
5657
fileExtensions: listOf(string())

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1903,7 +1903,7 @@ public function processStmtNode(
19031903

19041904
$bodyScope = $initScope;
19051905
$isIterableAtLeastOnce = TrinaryLogic::createYes();
1906-
$lastCondExpr = array_last($stmt->cond) ?? null;
1906+
$lastCondExpr = array_last($stmt->cond);
19071907
if (count($stmt->cond) > 0) {
19081908
$storage = $originalStorage->duplicate();
19091909

@@ -3669,7 +3669,7 @@ public function processArgs(
36693669
}
36703670

36713671
$this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $storage, $context);
3672-
$closureResult = $this->processClosureNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $context, $parameterType ?? null, $parameterNativeType);
3672+
$closureResult = $this->processClosureNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $context, $parameterType, $parameterNativeType);
36733673
if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) {
36743674
$throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $closureResult->getThrowPoints()));
36753675
$impurePoints = array_merge($impurePoints, $closureResult->getImpurePoints());
@@ -3735,7 +3735,7 @@ public function processArgs(
37353735
}
37363736

37373737
$this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $storage, $context);
3738-
$arrowFunctionResult = $this->processArrowFunctionNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $parameterType ?? null, $parameterNativeType);
3738+
$arrowFunctionResult = $this->processArrowFunctionNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $parameterType, $parameterNativeType);
37393739
if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) {
37403740
$throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $arrowFunctionResult->getThrowPoints()));
37413741
$impurePoints = array_merge($impurePoints, $arrowFunctionResult->getImpurePoints());

‎src/Reflection/SignatureMap/Php8SignatureMapProvider.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ private function getSignature(
477477

478478
return new FunctionSignature(
479479
$parameters,
480-
TypehintHelper::decideType($returnType, $phpDocReturnType ?? null),
480+
TypehintHelper::decideType($returnType, $phpDocReturnType),
481481
$returnType,
482482
$variadic,
483483
);

‎src/Rules/Variables/NullCoalesceRule.php‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,14 @@
44

55
use PhpParser\Node;
66
use PHPStan\Analyser\Scope;
7+
use PHPStan\DependencyInjection\AutowiredParameter;
78
use PHPStan\DependencyInjection\RegisteredRule;
9+
use PHPStan\Rules\IdentifierRuleError;
810
use PHPStan\Rules\IssetCheck;
911
use PHPStan\Rules\Rule;
12+
use PHPStan\Rules\RuleErrorBuilder;
1013
use PHPStan\Type\Type;
14+
use function sprintf;
1115

1216
/**
1317
* @implements Rule<Node\Expr>
@@ -16,7 +20,11 @@
1620
final class NullCoalesceRule implements Rule
1721
{
1822

19-
public function __construct(private IssetCheck $issetCheck)
23+
public function __construct(
24+
private IssetCheck $issetCheck,
25+
#[AutowiredParameter(ref: '%featureToggles.unnecessaryNullCoalesce%')]
26+
private bool $unnecessaryNullCoalesce,
27+
)
2028
{
2129
}
2230

@@ -41,18 +49,50 @@ public function processNode(Node $node, Scope $scope): array
4149
};
4250

4351
if ($node instanceof Node\Expr\BinaryOp\Coalesce) {
44-
$error = $this->issetCheck->check($node->left, $scope, 'on left side of ??', 'nullCoalesce', $typeMessageCallback);
52+
$left = $node->left;
53+
$right = $node->right;
54+
$operator = '??';
4555
} elseif ($node instanceof Node\Expr\AssignOp\Coalesce) {
46-
$error = $this->issetCheck->check($node->var, $scope, 'on left side of ??=', 'nullCoalesce', $typeMessageCallback);
56+
$left = $node->var;
57+
$right = $node->expr;
58+
$operator = '??=';
4759
} else {
4860
return [];
4961
}
5062

51-
if ($error === null) {
52-
return [];
63+
$error = $this->issetCheck->check($left, $scope, sprintf('on left side of %s', $operator), 'nullCoalesce', $typeMessageCallback);
64+
if ($error !== null) {
65+
return [$error];
66+
}
67+
68+
$unnecessaryError = $this->checkUnnecessaryNullCoalesce($left, $right, $operator, $scope);
69+
if ($unnecessaryError !== null) {
70+
return [$unnecessaryError];
71+
}
72+
73+
return [];
74+
}
75+
76+
private function checkUnnecessaryNullCoalesce(Node\Expr $left, Node\Expr $right, string $operator, Scope $scope): ?IdentifierRuleError
77+
{
78+
if (!$this->unnecessaryNullCoalesce) {
79+
return null;
80+
}
81+
82+
if (!$scope->getType($right)->isNull()->yes()) {
83+
return null;
84+
}
85+
86+
// The coalesce only changes the result when the left side is undefined.
87+
// If the left side is always set, `?? null` (or `??= null`) never changes
88+
// anything, so the whole coalesce is redundant.
89+
if ($scope->toMutatingScope()->issetCheck($left, static fn (): bool => true) !== true) {
90+
return null;
5391
}
5492

55-
return [$error];
93+
return RuleErrorBuilder::message(
94+
sprintf('Coalesce operator %s is unnecessary because the left side is always set and the right side is null.', $operator),
95+
)->identifier('nullCoalesce.unnecessary')->build();
5696
}
5797

5898
}

‎src/Type/FileTypeMapper.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -497,8 +497,8 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
497497
}
498498
}
499499

500-
$className = array_last($classStack) ?? null;
501-
$functionName = array_last($functionStack) ?? null;
500+
$className = array_last($classStack);
501+
$functionName = array_last($functionStack);
502502
$nameScopeKey = $this->getNameScopeKey($originalClassFileName, $className, $lookForTrait, $functionName);
503503

504504
$phpDocNode = null;
@@ -519,7 +519,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
519519
$typeAliasStack[] = $this->getTypeAliasesMap($phpDocNode);
520520
}
521521

522-
$parentNameScope = array_last($typeMapStack) ?? null;
522+
$parentNameScope = array_last($typeMapStack);
523523

524524
$typeMapStack[] = new IntermediaryNameScope(
525525
$namespace,
@@ -535,7 +535,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
535535
} elseif ($node instanceof Node\Stmt\ClassLike) {
536536
$typeAliasStack[] = [];
537537
} else {
538-
$parentNameScope = array_last($typeMapStack) ?? null;
538+
$parentNameScope = array_last($typeMapStack);
539539
$typeMapStack[] = new IntermediaryNameScope(
540540
$namespace,
541541
$uses,
@@ -566,7 +566,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
566566
)
567567
) && !array_key_exists($nameScopeKey, $nameScopeMap)
568568
) {
569-
$parentNameScope = array_last($typeMapStack) ?? null;
569+
$parentNameScope = array_last($typeMapStack);
570570
$typeAliasesMap = array_last($typeAliasStack) ?? [];
571571
$nameScopeMap[$nameScopeKey] = new IntermediaryNameScope(
572572
$namespace,
@@ -653,7 +653,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
653653
continue;
654654
}
655655

656-
$className = array_last($classStack) ?? null;
656+
$className = array_last($classStack);
657657
if ($className === null) {
658658
throw new ShouldNotHappenException();
659659
}

‎tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php‎

Lines changed: 81 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ protected function getRule(): Rule
2323
new PropertyReflectionFinder(),
2424
true,
2525
$this->shouldTreatPhpDocTypesAsCertain(),
26-
));
26+
), true);
2727
}
2828

2929
public function testCoalesceRule(): void
@@ -386,12 +386,20 @@ public function testPr4372(): void
386386

387387
public function testBug14213(): void
388388
{
389-
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], [
390-
[
391-
'Variable $x1 on left side of ?? always exists and is always null.',
392-
22,
393-
],
394-
]);
389+
$errors = [];
390+
if (PHP_VERSION_ID >= 80100) {
391+
// This is only detected with FiberScope.
392+
$errors[] = [
393+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
394+
21,
395+
];
396+
}
397+
$errors[] = [
398+
'Variable $x1 on left side of ?? always exists and is always null.',
399+
22,
400+
];
401+
402+
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], $errors);
395403
}
396404

397405
public function testBug11488(): void
@@ -452,6 +460,72 @@ public function testBug14459Hooked(): void
452460
]);
453461
}
454462

463+
public function testBug4337(): void
464+
{
465+
$this->analyse([__DIR__ . '/data/bug-4337.php'], [
466+
[
467+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
468+
37,
469+
],
470+
[
471+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
472+
42,
473+
],
474+
[
475+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
476+
47,
477+
],
478+
[
479+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
480+
53,
481+
],
482+
[
483+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
484+
58,
485+
],
486+
[
487+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
488+
63,
489+
],
490+
[
491+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
492+
69,
493+
],
494+
[
495+
'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.',
496+
75,
497+
],
498+
]);
499+
}
500+
501+
public function testBug12179(): void
502+
{
503+
$this->analyse([__DIR__ . '/data/bug-12179.php'], [
504+
[
505+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
506+
8,
507+
],
508+
[
509+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
510+
24,
511+
],
512+
]);
513+
}
514+
515+
public function testBug9966(): void
516+
{
517+
$this->analyse([__DIR__ . '/data/bug-9966.php'], [
518+
[
519+
'Offset \'key1\' on array{key1: string, key2: string|null, key3?: string, key4?: string|null} on left side of ?? always exists and is not nullable.',
520+
9,
521+
],
522+
[
523+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
524+
12,
525+
],
526+
]);
527+
}
528+
455529
public function testBug14393(): void
456530
{
457531
$this->analyse([__DIR__ . '/data/bug-14393.php'], [

‎tests/PHPStan/Rules/Variables/data/bug-12179.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<?php
2+
3+
namespace Bug12179;
4+
5+
function alwaysDefinedNullableParam(?string $name): ?string
6+
{
7+
// $name is always defined but nullable - the `?? null` is unnecessary
8+
return $name ?? null;
9+
}
10+
11+
function maybeUndefinedVariable(): ?string
12+
{
13+
if (rand() > 0.5) {
14+
$x = 'foo';
15+
}
16+
17+
// $x might be undefined - not reported
18+
return $x ?? null;
19+
}
20+
21+
/** @param array{foo?: ?int, bar: ?int} $x */
22+
function foo(array $x): void {
23+
var_dump($x['foo'] ?? null); // Fine
24+
var_dump($x['bar'] ?? null); // ?? null is redundant here
25+
}

0 commit comments

Comments
 (0)