From 34bacd74410573cf79754348231849b474b7312e Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Thu, 17 Oct 2024 10:36:12 +0200 Subject: [PATCH] Show TypeResult reasons in StrictComparisonOfDifferentTypesRule --- conf/config.neon | 3 + src/Analyser/DirectInternalScopeFactory.php | 2 + src/Analyser/LazyInternalScopeFactory.php | 1 + src/Analyser/MutatingScope.php | 42 +--------- src/Analyser/RicherScopeGetTypeHelper.php | 78 +++++++++++++++++++ .../StrictComparisonOfDifferentTypesRule.php | 18 ++++- src/Testing/PHPStanTestCase.php | 20 +++-- src/Type/MixedType.php | 13 +++- ...rictComparisonOfDifferentTypesRuleTest.php | 20 +++++ .../Comparison/data/strict-comparison.php | 20 +++++ .../Rules/Methods/CallMethodsRuleTest.php | 6 ++ 11 files changed, 173 insertions(+), 50 deletions(-) create mode 100644 src/Analyser/RicherScopeGetTypeHelper.php diff --git a/conf/config.neon b/conf/config.neon index 3146a8fd3f..5ad99397a9 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -582,6 +582,9 @@ services: arguments: cacheFilePath: %resultCachePath% + - + class: PHPStan\Analyser\RicherScopeGetTypeHelper + - class: PHPStan\Cache\Cache arguments: diff --git a/src/Analyser/DirectInternalScopeFactory.php b/src/Analyser/DirectInternalScopeFactory.php index 49a3395d2e..cbec53d0d1 100644 --- a/src/Analyser/DirectInternalScopeFactory.php +++ b/src/Analyser/DirectInternalScopeFactory.php @@ -34,6 +34,7 @@ public function __construct( private PropertyReflectionFinder $propertyReflectionFinder, private Parser $parser, private NodeScopeResolver $nodeScopeResolver, + private RicherScopeGetTypeHelper $richerScopeGetTypeHelper, private PhpVersion $phpVersion, private bool $explicitMixedInUnknownGenericNew, private bool $explicitMixedForGlobalVariables, @@ -85,6 +86,7 @@ public function create( $this->propertyReflectionFinder, $this->parser, $this->nodeScopeResolver, + $this->richerScopeGetTypeHelper, $this->constantResolver, $context, $this->phpVersion, diff --git a/src/Analyser/LazyInternalScopeFactory.php b/src/Analyser/LazyInternalScopeFactory.php index fec1521768..1e2e938678 100644 --- a/src/Analyser/LazyInternalScopeFactory.php +++ b/src/Analyser/LazyInternalScopeFactory.php @@ -79,6 +79,7 @@ public function create( $this->container->getByType(PropertyReflectionFinder::class), $this->container->getService('currentPhpVersionSimpleParser'), $this->container->getByType(NodeScopeResolver::class), + $this->container->getByType(RicherScopeGetTypeHelper::class), $this->container->getByType(ConstantResolver::class), $context, $this->container->getByType(PhpVersion::class), diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 401f011dc5..90b3fdd465 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -204,6 +204,7 @@ public function __construct( private PropertyReflectionFinder $propertyReflectionFinder, private Parser $parser, private NodeScopeResolver $nodeScopeResolver, + private RicherScopeGetTypeHelper $richerScopeGetTypeHelper, private ConstantResolver $constantResolver, private ScopeContext $context, private PhpVersion $phpVersion, @@ -924,46 +925,11 @@ private function resolveType(string $exprString, Expr $node): Type } if ($node instanceof Expr\BinaryOp\Identical) { - if ( - $node->left instanceof Variable - && is_string($node->left->name) - && $node->right instanceof Variable - && is_string($node->right->name) - && $node->left->name === $node->right->name - ) { - return new ConstantBooleanType(true); - } - - $leftType = $this->getType($node->left); - $rightType = $this->getType($node->right); - - if ( - ( - $node->left instanceof Node\Expr\PropertyFetch - || $node->left instanceof Node\Expr\StaticPropertyFetch - ) - && $rightType->isNull()->yes() - && !$this->hasPropertyNativeType($node->left) - ) { - return new BooleanType(); - } - - if ( - ( - $node->right instanceof Node\Expr\PropertyFetch - || $node->right instanceof Node\Expr\StaticPropertyFetch - ) - && $leftType->isNull()->yes() - && !$this->hasPropertyNativeType($node->right) - ) { - return new BooleanType(); - } - - return $this->initializerExprTypeResolver->resolveIdenticalType($leftType, $rightType)->type; + return $this->richerScopeGetTypeHelper->getIdenticalResult($this, $node)->type; } if ($node instanceof Expr\BinaryOp\NotIdentical) { - return $this->getType(new Expr\BooleanNot(new BinaryOp\Identical($node->left, $node->right))); + return $this->richerScopeGetTypeHelper->getNotIdenticalResult($this, $node)->type; } if ($node instanceof Expr\Instanceof_) { @@ -2654,7 +2620,7 @@ private function promoteNativeTypes(): self /** * @param Node\Expr\PropertyFetch|Node\Expr\StaticPropertyFetch $propertyFetch */ - private function hasPropertyNativeType($propertyFetch): bool + public function hasPropertyNativeType($propertyFetch): bool { $propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($propertyFetch, $this); if ($propertyReflection === null) { diff --git a/src/Analyser/RicherScopeGetTypeHelper.php b/src/Analyser/RicherScopeGetTypeHelper.php new file mode 100644 index 0000000000..e60612f820 --- /dev/null +++ b/src/Analyser/RicherScopeGetTypeHelper.php @@ -0,0 +1,78 @@ + + */ + public function getIdenticalResult(Scope $scope, Identical $expr): TypeResult + { + if ( + $expr->left instanceof Variable + && is_string($expr->left->name) + && $expr->right instanceof Variable + && is_string($expr->right->name) + && $expr->left->name === $expr->right->name + ) { + return new TypeResult(new ConstantBooleanType(true), []); + } + + $leftType = $scope->getType($expr->left); + $rightType = $scope->getType($expr->right); + + if ( + ( + $expr->left instanceof Node\Expr\PropertyFetch + || $expr->left instanceof Node\Expr\StaticPropertyFetch + ) + && $rightType->isNull()->yes() + && !$scope->hasPropertyNativeType($expr->left) + ) { + return new TypeResult(new BooleanType(), []); + } + + if ( + ( + $expr->right instanceof Node\Expr\PropertyFetch + || $expr->right instanceof Node\Expr\StaticPropertyFetch + ) + && $leftType->isNull()->yes() + && !$scope->hasPropertyNativeType($expr->right) + ) { + return new TypeResult(new BooleanType(), []); + } + + return $this->initializerExprTypeResolver->resolveIdenticalType($leftType, $rightType); + } + + /** + * @return TypeResult + */ + public function getNotIdenticalResult(Scope $scope, Node\Expr\BinaryOp\NotIdentical $expr): TypeResult + { + $identicalResult = $this->getIdenticalResult($scope, new Identical($expr->left, $expr->right)); + $identicalType = $identicalResult->type; + if ($identicalType instanceof ConstantBooleanType) { + return new TypeResult(new ConstantBooleanType(!$identicalType->getValue()), $identicalResult->reasons); + } + + return new TypeResult(new BooleanType(), []); + } + +} diff --git a/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php b/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php index 0db119501f..7d9c764132 100644 --- a/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php +++ b/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php @@ -3,6 +3,7 @@ namespace PHPStan\Rules\Comparison; use PhpParser\Node; +use PHPStan\Analyser\RicherScopeGetTypeHelper; use PHPStan\Analyser\Scope; use PHPStan\Parser\LastConditionVisitor; use PHPStan\Rules\Rule; @@ -10,6 +11,7 @@ use PHPStan\TrinaryLogic; use PHPStan\Type\Constant\ConstantBooleanType; use PHPStan\Type\VerbosityLevel; +use function count; use function sprintf; /** @@ -19,6 +21,7 @@ final class StrictComparisonOfDifferentTypesRule implements Rule { public function __construct( + private RicherScopeGetTypeHelper $richerScopeGetTypeHelper, private bool $checkAlwaysTrueStrictComparison, private bool $treatPhpDocTypesAsCertain, private bool $reportAlwaysTrueInLastCondition, @@ -34,11 +37,15 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { - if (!$node instanceof Node\Expr\BinaryOp\Identical && !$node instanceof Node\Expr\BinaryOp\NotIdentical) { + if ($node instanceof Node\Expr\BinaryOp\Identical) { + $nodeTypeResult = $this->richerScopeGetTypeHelper->getIdenticalResult($this->treatPhpDocTypesAsCertain ? $scope : $scope->doNotTreatPhpDocTypesAsCertain(), $node); + } elseif ($node instanceof Node\Expr\BinaryOp\NotIdentical) { + $nodeTypeResult = $this->richerScopeGetTypeHelper->getNotIdenticalResult($this->treatPhpDocTypesAsCertain ? $scope : $scope->doNotTreatPhpDocTypesAsCertain(), $node); + } else { return []; } - $nodeType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node) : $scope->getNativeType($node); + $nodeType = $nodeTypeResult->type; if (!$nodeType instanceof ConstantBooleanType) { return []; } @@ -46,7 +53,12 @@ public function processNode(Node $node, Scope $scope): array $leftType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node->left) : $scope->getNativeType($node->left); $rightType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node->right) : $scope->getNativeType($node->right); - $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node): RuleErrorBuilder { + $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $nodeTypeResult): RuleErrorBuilder { + $reasons = $nodeTypeResult->reasons; + if (count($reasons) > 0) { + return $ruleErrorBuilder->acceptsReasonsTip($reasons); + } + if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } diff --git a/src/Testing/PHPStanTestCase.php b/src/Testing/PHPStanTestCase.php index 14efcc7480..8f9fdfd44f 100644 --- a/src/Testing/PHPStanTestCase.php +++ b/src/Testing/PHPStanTestCase.php @@ -7,6 +7,7 @@ use PHPStan\Analyser\Error; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; +use PHPStan\Analyser\RicherScopeGetTypeHelper; use PHPStan\Analyser\ScopeFactory; use PHPStan\Analyser\TypeSpecifier; use PHPStan\BetterReflection\Reflector\ClassReflector; @@ -168,18 +169,20 @@ public static function createScopeFactory(ReflectionProvider $reflectionProvider $reflectionProviderProvider = new DirectReflectionProviderProvider($reflectionProvider); $constantResolver = new ConstantResolver($reflectionProviderProvider, $dynamicConstantNames); + $initializerExprTypeResolver = new InitializerExprTypeResolver( + $constantResolver, + $reflectionProviderProvider, + $container->getByType(PhpVersion::class), + $container->getByType(OperatorTypeSpecifyingExtensionRegistryProvider::class), + new OversizedArrayBuilder(), + $container->getParameter('usePathConstantsAsConstantString'), + ); + return new ScopeFactory( new DirectInternalScopeFactory( MutatingScope::class, $reflectionProvider, - new InitializerExprTypeResolver( - $constantResolver, - $reflectionProviderProvider, - $container->getByType(PhpVersion::class), - $container->getByType(OperatorTypeSpecifyingExtensionRegistryProvider::class), - new OversizedArrayBuilder(), - $container->getParameter('usePathConstantsAsConstantString'), - ), + $initializerExprTypeResolver, $container->getByType(DynamicReturnTypeExtensionRegistryProvider::class), $container->getByType(ExpressionTypeResolverExtensionRegistryProvider::class), $container->getByType(ExprPrinter::class), @@ -187,6 +190,7 @@ public static function createScopeFactory(ReflectionProvider $reflectionProvider new PropertyReflectionFinder(), self::getParser(), $container->getByType(NodeScopeResolver::class), + new RicherScopeGetTypeHelper($initializerExprTypeResolver), $container->getByType(PhpVersion::class), $container->getParameter('featureToggles')['explicitMixedInUnknownGenericNew'], $container->getParameter('featureToggles')['explicitMixedForGlobalVariables'], diff --git a/src/Type/MixedType.php b/src/Type/MixedType.php index 20117d68b3..0b9d87394a 100644 --- a/src/Type/MixedType.php +++ b/src/Type/MixedType.php @@ -159,7 +159,18 @@ public function isSuperTypeOfWithReason(Type $type): IsSuperTypeOfResult return IsSuperTypeOfResult::createMaybe(); } - return $this->subtractedType->isSuperTypeOfWithReason($type)->negate(); + $result = $this->subtractedType->isSuperTypeOfWithReason($type)->negate(); + if ($result->no()) { + return IsSuperTypeOfResult::createNo([ + sprintf( + 'Type %s has already been eliminated from %s.', + $this->subtractedType->describe(VerbosityLevel::precise()), + $this->describe(VerbosityLevel::typeOnly()), + ), + ]); + } + + return $result; } public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type diff --git a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php index 4721b4e78b..96712b66b9 100644 --- a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Comparison; +use PHPStan\Analyser\RicherScopeGetTypeHelper; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use const PHP_INT_SIZE; @@ -22,6 +23,7 @@ class StrictComparisonOfDifferentTypesRuleTest extends RuleTestCase protected function getRule(): Rule { return new StrictComparisonOfDifferentTypesRule( + self::getContainer()->getByType(RicherScopeGetTypeHelper::class), $this->checkAlwaysTrueStrictComparison, $this->treatPhpDocTypesAsCertain, $this->reportAlwaysTrueInLastCondition, @@ -216,10 +218,12 @@ public function testStrictComparison(): void [ 'Strict comparison using === between mixed and \'foo\' will always evaluate to false.', 808, + 'Type 1|string has already been eliminated from mixed.', ], [ 'Strict comparison using !== between mixed and 1 will always evaluate to true.', 812, + 'Type 1|string has already been eliminated from mixed.', ], [ 'Strict comparison using === between \'foo\' and \'foo\' will always evaluate to true.', @@ -275,6 +279,16 @@ public function testStrictComparison(): void 1014, $tipText, ], + [ + 'Strict comparison using === between mixed and null will always evaluate to false.', + 1030, + 'Type null has already been eliminated from mixed.', + ], + [ + 'Strict comparison using !== between mixed and null will always evaluate to true.', + 1034, + 'Type null has already been eliminated from mixed.', + ], ], ); } @@ -419,6 +433,7 @@ public function testStrictComparisonWithoutAlwaysTrue(): void [ 'Strict comparison using === between mixed and \'foo\' will always evaluate to false.', 808, + 'Type 1|string has already been eliminated from mixed.', ], [ 'Strict comparison using === between NAN and NAN will always evaluate to false.', @@ -433,6 +448,11 @@ public function testStrictComparisonWithoutAlwaysTrue(): void 1014, $tipText, ], + [ + 'Strict comparison using === between mixed and null will always evaluate to false.', + 1030, + 'Type null has already been eliminated from mixed.', + ], ], ); } diff --git a/tests/PHPStan/Rules/Comparison/data/strict-comparison.php b/tests/PHPStan/Rules/Comparison/data/strict-comparison.php index 3e05dd8b83..b6389bd5ee 100644 --- a/tests/PHPStan/Rules/Comparison/data/strict-comparison.php +++ b/tests/PHPStan/Rules/Comparison/data/strict-comparison.php @@ -1017,3 +1017,23 @@ public function doFoo($a): void } } + +class SubtractedMixedAgainstNull +{ + + public function doFoo($m): void + { + if ($m === null) { + return; + } + + if ($m === null) { + + } + + if ($m !== null) { + + } + } + +} diff --git a/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php b/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php index 7a46d849d3..b9c196420d 100644 --- a/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php +++ b/tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php @@ -457,14 +457,17 @@ public function testCallMethods(): void [ 'Parameter #1 $i of method Test\SubtractedMixed::requireInt() expects int, mixed given.', 1277, + 'Type int has already been eliminated from mixed.', ], [ 'Parameter #1 $i of method Test\SubtractedMixed::requireInt() expects int, mixed given.', 1284, + 'Type int|string has already been eliminated from mixed.', ], [ 'Parameter #1 $parameter of method Test\SubtractedMixed::requireIntOrString() expects int|string, mixed given.', 1285, + 'Type int|string has already been eliminated from mixed.', ], [ 'Parameter #2 $b of method Test\ExpectsExceptionGenerics::expectsExceptionUpperBound() expects Exception, Throwable given.', @@ -793,14 +796,17 @@ public function testCallMethodsOnThisOnly(): void [ 'Parameter #1 $i of method Test\SubtractedMixed::requireInt() expects int, mixed given.', 1277, + 'Type int has already been eliminated from mixed.', ], [ 'Parameter #1 $i of method Test\SubtractedMixed::requireInt() expects int, mixed given.', 1284, + 'Type int|string has already been eliminated from mixed.', ], [ 'Parameter #1 $parameter of method Test\SubtractedMixed::requireIntOrString() expects int|string, mixed given.', 1285, + 'Type int|string has already been eliminated from mixed.', ], [ 'Parameter #2 $b of method Test\ExpectsExceptionGenerics::expectsExceptionUpperBound() expects Exception, Throwable given.',