diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 528084e3532..55a8d2f5d51 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -20,3 +20,4 @@ parameters: reportMethodPurityOverride: true checkDynamicConstantNameValues: true unusedLabel: true + unnecessaryNullCoalesce: true diff --git a/conf/config.neon b/conf/config.neon index df8f46567a1..4ab67acdf16 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -47,6 +47,7 @@ parameters: reportMethodPurityOverride: false checkDynamicConstantNameValues: false unusedLabel: false + unnecessaryNullCoalesce: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index dc07b020e09..e3b06a5a2d5 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -49,6 +49,7 @@ parametersSchema: reportMethodPurityOverride: bool() checkDynamicConstantNameValues: bool() unusedLabel: bool() + unnecessaryNullCoalesce: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 9faedfce2f9..6206208f3c7 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1865,7 +1865,7 @@ public function processStmtNode( $bodyScope = $initScope; $isIterableAtLeastOnce = TrinaryLogic::createYes(); - $lastCondExpr = array_last($stmt->cond) ?? null; + $lastCondExpr = array_last($stmt->cond); if (count($stmt->cond) > 0) { $storage = $originalStorage->duplicate(); @@ -3635,7 +3635,7 @@ public function processArgs( } $this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $storage, $context); - $closureResult = $this->processClosureNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $context, $parameterType ?? null, $parameterNativeType); + $closureResult = $this->processClosureNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $context, $parameterType, $parameterNativeType); if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) { $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())); $impurePoints = array_merge($impurePoints, $closureResult->getImpurePoints()); @@ -3694,7 +3694,7 @@ public function processArgs( } $this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $storage, $context); - $arrowFunctionResult = $this->processArrowFunctionNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $parameterType ?? null, $parameterNativeType); + $arrowFunctionResult = $this->processArrowFunctionNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $parameterType, $parameterNativeType); if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) { $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())); $impurePoints = array_merge($impurePoints, $arrowFunctionResult->getImpurePoints()); diff --git a/src/Reflection/SignatureMap/Php8SignatureMapProvider.php b/src/Reflection/SignatureMap/Php8SignatureMapProvider.php index 19884d70378..ad470ff3ecc 100644 --- a/src/Reflection/SignatureMap/Php8SignatureMapProvider.php +++ b/src/Reflection/SignatureMap/Php8SignatureMapProvider.php @@ -477,7 +477,7 @@ private function getSignature( return new FunctionSignature( $parameters, - TypehintHelper::decideType($returnType, $phpDocReturnType ?? null), + TypehintHelper::decideType($returnType, $phpDocReturnType), $returnType, $variadic, ); diff --git a/src/Rules/Variables/NullCoalesceRule.php b/src/Rules/Variables/NullCoalesceRule.php index 79941d0293d..6250b59b906 100644 --- a/src/Rules/Variables/NullCoalesceRule.php +++ b/src/Rules/Variables/NullCoalesceRule.php @@ -4,10 +4,14 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; +use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; +use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Type; +use function sprintf; /** * @implements Rule @@ -16,7 +20,11 @@ final class NullCoalesceRule implements Rule { - public function __construct(private IssetCheck $issetCheck) + public function __construct( + private IssetCheck $issetCheck, + #[AutowiredParameter(ref: '%featureToggles.unnecessaryNullCoalesce%')] + private bool $unnecessaryNullCoalesce, + ) { } @@ -41,18 +49,50 @@ public function processNode(Node $node, Scope $scope): array }; if ($node instanceof Node\Expr\BinaryOp\Coalesce) { - $error = $this->issetCheck->check($node->left, $scope, 'on left side of ??', 'nullCoalesce', $typeMessageCallback); + $left = $node->left; + $right = $node->right; + $operator = '??'; } elseif ($node instanceof Node\Expr\AssignOp\Coalesce) { - $error = $this->issetCheck->check($node->var, $scope, 'on left side of ??=', 'nullCoalesce', $typeMessageCallback); + $left = $node->var; + $right = $node->expr; + $operator = '??='; } else { return []; } - if ($error === null) { - return []; + $error = $this->issetCheck->check($left, $scope, sprintf('on left side of %s', $operator), 'nullCoalesce', $typeMessageCallback); + if ($error !== null) { + return [$error]; + } + + $unnecessaryError = $this->checkUnnecessaryNullCoalesce($left, $right, $operator, $scope); + if ($unnecessaryError !== null) { + return [$unnecessaryError]; + } + + return []; + } + + private function checkUnnecessaryNullCoalesce(Node\Expr $left, Node\Expr $right, string $operator, Scope $scope): ?IdentifierRuleError + { + if (!$this->unnecessaryNullCoalesce) { + return null; + } + + if (!$scope->getType($right)->isNull()->yes()) { + return null; + } + + // The coalesce only changes the result when the left side is undefined. + // If the left side is always set, `?? null` (or `??= null`) never changes + // anything, so the whole coalesce is redundant. + if ($scope->toMutatingScope()->issetCheck($left, static fn (): bool => true) !== true) { + return null; } - return [$error]; + return RuleErrorBuilder::message( + sprintf('Coalesce operator %s is unnecessary because the left side is always set and the right side is null.', $operator), + )->identifier('nullCoalesce.unnecessary')->build(); } } diff --git a/src/Type/FileTypeMapper.php b/src/Type/FileTypeMapper.php index 431954eb380..ad098b412f2 100644 --- a/src/Type/FileTypeMapper.php +++ b/src/Type/FileTypeMapper.php @@ -484,8 +484,8 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA } } - $className = array_last($classStack) ?? null; - $functionName = array_last($functionStack) ?? null; + $className = array_last($classStack); + $functionName = array_last($functionStack); $nameScopeKey = $this->getNameScopeKey($originalClassFileName, $className, $lookForTrait, $functionName); $phpDocNode = null; @@ -506,7 +506,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA $typeAliasStack[] = $this->getTypeAliasesMap($phpDocNode); } - $parentNameScope = array_last($typeMapStack) ?? null; + $parentNameScope = array_last($typeMapStack); $typeMapStack[] = new IntermediaryNameScope( $namespace, @@ -522,7 +522,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA } elseif ($node instanceof Node\Stmt\ClassLike) { $typeAliasStack[] = []; } else { - $parentNameScope = array_last($typeMapStack) ?? null; + $parentNameScope = array_last($typeMapStack); $typeMapStack[] = new IntermediaryNameScope( $namespace, $uses, @@ -553,7 +553,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA ) ) && !array_key_exists($nameScopeKey, $nameScopeMap) ) { - $parentNameScope = array_last($typeMapStack) ?? null; + $parentNameScope = array_last($typeMapStack); $typeAliasesMap = array_last($typeAliasStack) ?? []; $nameScopeMap[$nameScopeKey] = new IntermediaryNameScope( $namespace, @@ -640,7 +640,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA continue; } - $className = array_last($classStack) ?? null; + $className = array_last($classStack); if ($className === null) { throw new ShouldNotHappenException(); } diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index e22565393fe..ad49895d130 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -23,7 +23,7 @@ protected function getRule(): Rule new PropertyReflectionFinder(), true, $this->shouldTreatPhpDocTypesAsCertain(), - )); + ), true); } public function testCoalesceRule(): void @@ -364,12 +364,20 @@ public function testPr4372(): void public function testBug14213(): void { - $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], [ - [ - 'Variable $x1 on left side of ?? always exists and is always null.', - 22, - ], - ]); + $errors = []; + if (PHP_VERSION_ID >= 80100) { + // This is only detected with FiberScope. + $errors[] = [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 21, + ]; + } + $errors[] = [ + 'Variable $x1 on left side of ?? always exists and is always null.', + 22, + ]; + + $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], $errors); } public function testBug11488(): void @@ -430,6 +438,44 @@ public function testBug14459Hooked(): void ]); } + public function testBug4337(): void + { + $this->analyse([__DIR__ . '/data/bug-4337.php'], [ + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 37, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 42, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 47, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 53, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 58, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 63, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 69, + ], + [ + 'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.', + 75, + ], + ]); + } + public function testBug14393(): void { $this->analyse([__DIR__ . '/data/bug-14393.php'], [ diff --git a/tests/PHPStan/Rules/Variables/data/bug-4337.php b/tests/PHPStan/Rules/Variables/data/bug-4337.php new file mode 100644 index 00000000000..6beeafff95a --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-4337.php @@ -0,0 +1,98 @@ +method() ?? null; +} + +function staticCall(): ?string +{ + return Foo::staticMethod() ?? null; +} + +function definedNullableVariable(?string $name): ?string +{ + $x = $name; + return $x ?? null; +} + +function alwaysSetNullableProperty(Foo $foo): ?string +{ + return $foo->stringOrNull ?? null; +} + +function alwaysSetNullableStaticProperty(): ?string +{ + return Foo::$staticStringOrNull ?? null; +} + +/** @param array{a: string|null} $arr */ +function alwaysSetNullableOffset(array $arr): ?string +{ + return $arr['a'] ?? null; +} + +function assignCoalesceAlwaysSet(?string $name): void +{ + $x = $name; + $x ??= null; +} + +// these are NOT unnecessary and must not be reported + +function maybeUndefinedVariable(): ?string +{ + if (rand() > 0.5) { + $x = 'foo'; + } + + return $x ?? null; +} + +/** @param array $arr */ +function maybeUndefinedOffset(array $arr): ?string +{ + return $arr['a'] ?? null; +} + +function nonNullRightSide(?string $name): string +{ + return hello1($name) ?? 'default'; +}