diff --git a/src/Rules/Comparison/ConstantConditionInTraitHelper.php b/src/Rules/Comparison/ConstantConditionInTraitHelper.php index 31f70c70cc6..4d101f59a6a 100644 --- a/src/Rules/Comparison/ConstantConditionInTraitHelper.php +++ b/src/Rules/Comparison/ConstantConditionInTraitHelper.php @@ -50,6 +50,9 @@ public function emitNoError( } /** + * $value tells apart the contexts in which the same expression resolves to a different + * constant result. Rules whose message already encodes the result can pass a fixed value. + * * @param class-string> $ruleName */ public function emitError( diff --git a/src/Rules/Variables/EmptyRule.php b/src/Rules/Variables/EmptyRule.php index d1656a3be27..287490ec469 100644 --- a/src/Rules/Variables/EmptyRule.php +++ b/src/Rules/Variables/EmptyRule.php @@ -3,8 +3,11 @@ namespace PHPStan\Rules\Variables; use PhpParser\Node; +use PHPStan\Analyser\CollectedDataEmitter; +use PHPStan\Analyser\NodeCallbackInvoker; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; use PHPStan\Type\Type; @@ -16,7 +19,10 @@ final class EmptyRule implements Rule { - public function __construct(private IssetCheck $issetCheck) + public function __construct( + private IssetCheck $issetCheck, + private ConstantConditionInTraitHelper $constantConditionInTraitHelper, + ) { } @@ -25,7 +31,7 @@ public function getNodeType(): string return Node\Expr\Empty_::class; } - public function processNode(Node $node, Scope $scope): array + public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array { $error = $this->issetCheck->check($node->expr, $scope, 'in empty()', 'empty', static function (Type $type): ?string { $isNull = $type->isNull(); @@ -60,6 +66,14 @@ public function processNode(Node $node, Scope $scope): array }); if ($error === null) { + $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $node->expr); + return []; + } + + if ($scope->isInTrait()) { + // IssetCheck's message already distinguishes the possible outcomes, + // so the contexts only need to be told apart by error/no error. + $this->constantConditionInTraitHelper->emitError(self::class, $scope, $node->expr, true, $error); return []; } diff --git a/src/Rules/Variables/IssetRule.php b/src/Rules/Variables/IssetRule.php index 839241a169b..ffa4433511d 100644 --- a/src/Rules/Variables/IssetRule.php +++ b/src/Rules/Variables/IssetRule.php @@ -3,8 +3,11 @@ namespace PHPStan\Rules\Variables; use PhpParser\Node; +use PHPStan\Analyser\CollectedDataEmitter; +use PHPStan\Analyser\NodeCallbackInvoker; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; use PHPStan\Type\Type; @@ -16,7 +19,10 @@ final class IssetRule implements Rule { - public function __construct(private IssetCheck $issetCheck) + public function __construct( + private IssetCheck $issetCheck, + private ConstantConditionInTraitHelper $constantConditionInTraitHelper, + ) { } @@ -25,7 +31,7 @@ public function getNodeType(): string return Node\Expr\Isset_::class; } - public function processNode(Node $node, Scope $scope): array + public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array { $messages = []; foreach ($node->vars as $var) { @@ -42,8 +48,17 @@ public function processNode(Node $node, Scope $scope): array return 'is not nullable'; }); if ($error === null) { + $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $var); continue; } + + if ($scope->isInTrait()) { + // IssetCheck's message already distinguishes the possible outcomes, + // so the contexts only need to be told apart by error/no error. + $this->constantConditionInTraitHelper->emitError(self::class, $scope, $var, true, $error); + continue; + } + $messages[] = $error; } diff --git a/src/Rules/Variables/NullCoalesceRule.php b/src/Rules/Variables/NullCoalesceRule.php index 6250b59b906..1f1bd634ab1 100644 --- a/src/Rules/Variables/NullCoalesceRule.php +++ b/src/Rules/Variables/NullCoalesceRule.php @@ -3,9 +3,12 @@ namespace PHPStan\Rules\Variables; use PhpParser\Node; +use PHPStan\Analyser\CollectedDataEmitter; +use PHPStan\Analyser\NodeCallbackInvoker; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\RegisteredRule; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Rule; @@ -22,6 +25,7 @@ final class NullCoalesceRule implements Rule public function __construct( private IssetCheck $issetCheck, + private ConstantConditionInTraitHelper $constantConditionInTraitHelper, #[AutowiredParameter(ref: '%featureToggles.unnecessaryNullCoalesce%')] private bool $unnecessaryNullCoalesce, ) @@ -33,7 +37,7 @@ public function getNodeType(): string return Node\Expr::class; } - public function processNode(Node $node, Scope $scope): array + public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array { $typeMessageCallback = static function (Type $type): ?string { $isNull = $type->isNull(); @@ -60,17 +64,22 @@ public function processNode(Node $node, Scope $scope): array return []; } - $error = $this->issetCheck->check($left, $scope, sprintf('on left side of %s', $operator), 'nullCoalesce', $typeMessageCallback); - if ($error !== null) { - return [$error]; + $error = $this->issetCheck->check($left, $scope, sprintf('on left side of %s', $operator), 'nullCoalesce', $typeMessageCallback) + ?? $this->checkUnnecessaryNullCoalesce($left, $right, $operator, $scope); + + if ($error === null) { + $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $left); + return []; } - $unnecessaryError = $this->checkUnnecessaryNullCoalesce($left, $right, $operator, $scope); - if ($unnecessaryError !== null) { - return [$unnecessaryError]; + if ($scope->isInTrait()) { + // The error messages already distinguish the possible outcomes, + // so the contexts only need to be told apart by error/no error. + $this->constantConditionInTraitHelper->emitError(self::class, $scope, $left, true, $error); + return []; } - return []; + return [$error]; } private function checkUnnecessaryNullCoalesce(Node\Expr $left, Node\Expr $right, string $operator, Scope $scope): ?IdentifierRuleError diff --git a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php index 582fdeb1076..738d10675c2 100644 --- a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php +++ b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php @@ -2,16 +2,19 @@ namespace PHPStan\Rules\Variables; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; +use PHPStan\Rules\Comparison\ConstantConditionInTraitRule; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Properties\PropertyDescriptor; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; +use PHPStan\Testing\CompositeRule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\RequiresPhp; /** - * @extends RuleTestCase + * @extends RuleTestCase */ class EmptyRuleTest extends RuleTestCase { @@ -20,12 +23,19 @@ class EmptyRuleTest extends RuleTestCase protected function getRule(): Rule { - return new EmptyRule(new IssetCheck( - new PropertyDescriptor(), - new PropertyReflectionFinder(), - true, - $this->treatPhpDocTypesAsCertain, - )); + // @phpstan-ignore argument.type + return new CompositeRule([ + new EmptyRule( + new IssetCheck( + new PropertyDescriptor(), + new PropertyReflectionFinder(), + true, + $this->treatPhpDocTypesAsCertain, + ), + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + ), + new ConstantConditionInTraitRule(), + ]); } protected function shouldTreatPhpDocTypesAsCertain(): bool @@ -252,4 +262,24 @@ public function testIssetAfterRememberedConstructor(): void ]); } + public function testInTrait(): void + { + $this->treatPhpDocTypesAsCertain = true; + + $this->analyse([__DIR__ . '/data/isset-in-trait.php'], [ + [ + 'Property IssetInTrait\FirstNonNullableProperty::$k (int<1, max>) in empty() is not falsy.', + 84, + ], + [ + 'Property IssetInTrait\SecondNonNullableProperty::$k (int<1, max>) in empty() is not falsy.', + 84, + ], + [ + 'Variable $s in empty() always exists and is not falsy.', + 121, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/IssetRuleTest.php b/tests/PHPStan/Rules/Variables/IssetRuleTest.php index 7e83c53117b..4bf11fb3f53 100644 --- a/tests/PHPStan/Rules/Variables/IssetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/IssetRuleTest.php @@ -2,15 +2,18 @@ namespace PHPStan\Rules\Variables; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; +use PHPStan\Rules\Comparison\ConstantConditionInTraitRule; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Properties\PropertyDescriptor; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; +use PHPStan\Testing\CompositeRule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\RequiresPhp; /** - * @extends RuleTestCase + * @extends RuleTestCase */ class IssetRuleTest extends RuleTestCase { @@ -19,12 +22,19 @@ class IssetRuleTest extends RuleTestCase protected function getRule(): Rule { - return new IssetRule(new IssetCheck( - new PropertyDescriptor(), - new PropertyReflectionFinder(), - true, - $this->treatPhpDocTypesAsCertain, - )); + // @phpstan-ignore argument.type + return new CompositeRule([ + new IssetRule( + new IssetCheck( + new PropertyDescriptor(), + new PropertyReflectionFinder(), + true, + $this->treatPhpDocTypesAsCertain, + ), + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + ), + new ConstantConditionInTraitRule(), + ]); } protected function shouldTreatPhpDocTypesAsCertain(): bool @@ -612,4 +622,31 @@ public function testBug14610(): void $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14610.php'], []); } + public function testInTrait(): void + { + $this->treatPhpDocTypesAsCertain = true; + + $this->analyse([__DIR__ . '/data/isset-in-trait.php'], [ + [ + 'Property IssetInTrait\FirstNonNullableProperty::$k (int<1, max>) in isset() is not nullable.', + 83, + ], + [ + 'Property IssetInTrait\SecondNonNullableProperty::$k (int<1, max>) in isset() is not nullable.', + 83, + ], + [ + 'Variable $s in isset() always exists and is not nullable.', + 120, + ], + ]); + } + + public function testBug14416(): void + { + $this->treatPhpDocTypesAsCertain = true; + + $this->analyse([__DIR__ . '/data/bug-14416.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index fc6c577822d..3d24237dff0 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -2,28 +2,39 @@ namespace PHPStan\Rules\Variables; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; +use PHPStan\Rules\Comparison\ConstantConditionInTraitRule; use PHPStan\Rules\IssetCheck; use PHPStan\Rules\Properties\PropertyDescriptor; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; +use PHPStan\Testing\CompositeRule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\RequiresPhp; use const PHP_VERSION_ID; /** - * @extends RuleTestCase + * @extends RuleTestCase */ class NullCoalesceRuleTest extends RuleTestCase { protected function getRule(): Rule { - return new NullCoalesceRule(new IssetCheck( - new PropertyDescriptor(), - new PropertyReflectionFinder(), - true, - $this->shouldTreatPhpDocTypesAsCertain(), - ), true); + // @phpstan-ignore argument.type + return new CompositeRule([ + new NullCoalesceRule( + new IssetCheck( + new PropertyDescriptor(), + new PropertyReflectionFinder(), + true, + $this->shouldTreatPhpDocTypesAsCertain(), + ), + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + true, + ), + new ConstantConditionInTraitRule(), + ]); } public function testCoalesceRule(): void @@ -560,4 +571,22 @@ public function testBug14393(): void ]); } + public function testInTrait(): void + { + $this->analyse([__DIR__ . '/data/isset-in-trait.php'], [ + [ + 'Property IssetInTrait\FirstNonNullableProperty::$k (int<1, max>) on left side of ?? is not nullable.', + 82, + ], + [ + 'Property IssetInTrait\SecondNonNullableProperty::$k (int<1, max>) on left side of ?? is not nullable.', + 82, + ], + [ + 'Variable $s on left side of ?? always exists and is not nullable.', + 119, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-14416.php b/tests/PHPStan/Rules/Variables/data/bug-14416.php new file mode 100644 index 00000000000..0a7ebe572dc --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-14416.php @@ -0,0 +1,23 @@ +i); + } +} + +class MyClass +{ + use MyTrait; + + public int $i = 10; +} + +class MyClass2 +{ + use MyTrait; +} diff --git a/tests/PHPStan/Rules/Variables/data/isset-in-trait.php b/tests/PHPStan/Rules/Variables/data/isset-in-trait.php new file mode 100644 index 00000000000..19d9ba2a5e3 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/isset-in-trait.php @@ -0,0 +1,138 @@ +i ?? -1); + var_dump(isset($this->i)); + var_dump(empty($this->i)); + } + +} + +class DeclaredProperty +{ + + use MaybeDeclaredPropertyTrait; + + /** @var positive-int */ + public int $i = 10; + +} + +class UndeclaredProperty +{ + + use MaybeDeclaredPropertyTrait; + +} + +/** + * The property is non-nullable in one class using the trait and nullable in the other, + * so nothing should be reported. + */ +trait DifferentPropertyTypeTrait +{ + + public function doFoo(): void + { + var_dump($this->j ?? -1); + var_dump(isset($this->j)); + var_dump(empty($this->j)); + } + +} + +class NonNullableProperty +{ + + use DifferentPropertyTypeTrait; + + /** @var positive-int */ + public int $j = 10; + +} + +class NullableProperty +{ + + use DifferentPropertyTypeTrait; + + public ?int $j = null; + +} + +/** + * The property is non-nullable in every class using the trait, so the errors are reported + * in the context of each of them. + */ +trait SamePropertyTypeTrait +{ + + public function doFoo(): void + { + var_dump($this->k ?? -1); + var_dump(isset($this->k)); + var_dump(empty($this->k)); + } + +} + +class FirstNonNullableProperty +{ + + use SamePropertyTypeTrait; + + /** @var positive-int */ + public int $k = 10; + +} + +class SecondNonNullableProperty +{ + + use SamePropertyTypeTrait; + + /** @var positive-int */ + public int $k = 20; + +} + +/** + * The checked expression does not depend on the class using the trait, so the errors are + * reported once, directly in the trait. + */ +trait ClassIndependentTrait +{ + + public function doFoo(): void + { + $s = 'foo'; + var_dump($s ?? -1); + var_dump(isset($s)); + var_dump(empty($s)); + } + +} + +class FirstClassIndependent +{ + + use ClassIndependentTrait; + +} + +class SecondClassIndependent +{ + + use ClassIndependentTrait; + +}