From e52f144f866240a94ac8ca48e4caaf6c6064f2a3 Mon Sep 17 00:00:00 2001 From: Vincent Langlet Date: Wed, 15 Jan 2025 10:44:14 +0100 Subject: [PATCH] Decorate with reason --- .../ConstantConditionRuleHelper.php | 2 +- .../ImpossibleCheckTypeFunctionCallRule.php | 11 ++- .../Comparison/ImpossibleCheckTypeHelper.php | 81 +++++++++++-------- .../ImpossibleCheckTypeMethodCallRule.php | 11 ++- ...mpossibleCheckTypeStaticMethodCallRule.php | 11 ++- .../StrictComparisonOfDifferentTypesRule.php | 2 +- src/Rules/RuleErrorBuilder.php | 2 +- .../AccessoryLowercaseStringType.php | 10 ++- ...ingFunctionsDynamicReturnTypeExtension.php | 2 +- ...mpossibleCheckTypeFunctionCallRuleTest.php | 14 ++++ ...rictComparisonOfDifferentTypesRuleTest.php | 29 ++++--- .../Rules/Comparison/data/bug-11799.php | 16 ++++ 12 files changed, 134 insertions(+), 57 deletions(-) create mode 100644 tests/PHPStan/Rules/Comparison/data/bug-11799.php diff --git a/src/Rules/Comparison/ConstantConditionRuleHelper.php b/src/Rules/Comparison/ConstantConditionRuleHelper.php index 36b2c569d8..3d1dd28f43 100644 --- a/src/Rules/Comparison/ConstantConditionRuleHelper.php +++ b/src/Rules/Comparison/ConstantConditionRuleHelper.php @@ -51,7 +51,7 @@ public function shouldSkip(Scope $scope, Expr $expr): bool || $expr instanceof MethodCall || $expr instanceof Expr\StaticCall ) { - $isAlways = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $expr); + $isAlways = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $expr)[0]; if ($isAlways !== null) { return true; } diff --git a/src/Rules/Comparison/ImpossibleCheckTypeFunctionCallRule.php b/src/Rules/Comparison/ImpossibleCheckTypeFunctionCallRule.php index a4b522d9f3..ed934621b5 100644 --- a/src/Rules/Comparison/ImpossibleCheckTypeFunctionCallRule.php +++ b/src/Rules/Comparison/ImpossibleCheckTypeFunctionCallRule.php @@ -7,6 +7,7 @@ use PHPStan\Parser\LastConditionVisitor; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; +use function count; use function sprintf; /** @@ -36,17 +37,21 @@ public function processNode(Node $node, Scope $scope): array } $functionName = (string) $node->name; - $isAlways = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $node); + [$isAlways, $reasons] = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $node); if ($isAlways === null) { return []; } - $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node): RuleErrorBuilder { + $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $reasons): RuleErrorBuilder { + if (count($reasons) > 0) { + $ruleErrorBuilder->acceptsReasonsTip($reasons); + } + if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } - $isAlways = $this->impossibleCheckTypeHelper->doNotTreatPhpDocTypesAsCertain()->findSpecifiedType($scope, $node); + $isAlways = $this->impossibleCheckTypeHelper->doNotTreatPhpDocTypesAsCertain()->findSpecifiedType($scope, $node)[0]; if ($isAlways !== null) { return $ruleErrorBuilder; } diff --git a/src/Rules/Comparison/ImpossibleCheckTypeHelper.php b/src/Rules/Comparison/ImpossibleCheckTypeHelper.php index 48eea97d6f..369133f87f 100644 --- a/src/Rules/Comparison/ImpossibleCheckTypeHelper.php +++ b/src/Rules/Comparison/ImpossibleCheckTypeHelper.php @@ -54,14 +54,17 @@ public function __construct( { } + /** + * @return array{bool|null, list} + */ public function findSpecifiedType( Scope $scope, Expr $node, - ): ?bool + ): array { if ($node instanceof FuncCall) { if ($node->isFirstClassCallable()) { - return null; + return [null, []]; } $argsCount = count($node->getArgs()); if ($node->name instanceof Node\Name) { @@ -70,10 +73,10 @@ public function findSpecifiedType( $arg = $node->getArgs()[0]->value; $assertValue = ($this->treatPhpDocTypesAsCertain ? $scope->getType($arg) : $scope->getNativeType($arg))->toBoolean(); if (!$assertValue instanceof ConstantBooleanType) { - return null; + return [null, []]; } - return $assertValue->getValue(); + return [$assertValue->getValue(), []]; } if (in_array($functionName, [ 'class_exists', @@ -81,23 +84,23 @@ public function findSpecifiedType( 'trait_exists', 'enum_exists', ], true)) { - return null; + return [null, []]; } if (in_array($functionName, ['count', 'sizeof'], true)) { - return null; + return [null, []]; } elseif ($functionName === 'defined') { - return null; + return [null, []]; } elseif ($functionName === 'array_search') { - return null; + return [null, []]; } elseif ($functionName === 'in_array' && $argsCount >= 2) { $haystackArg = $node->getArgs()[1]->value; $haystackType = ($this->treatPhpDocTypesAsCertain ? $scope->getType($haystackArg) : $scope->getNativeType($haystackArg)); if ($haystackType instanceof MixedType) { - return null; + return [null, []]; } if (!$haystackType->isArray()->yes()) { - return null; + return [null, []]; } $needleArg = $node->getArgs()[0]->value; @@ -114,7 +117,7 @@ public function findSpecifiedType( || $haystackType->getIterableValueType()->isEnum()->yes(); if (!$isStrictComparison) { - return null; + return [null, []]; } $valueType = $haystackType->getIterableValueType(); @@ -125,25 +128,25 @@ public function findSpecifiedType( if ($haystackType->isIterableAtLeastOnce()->yes()) { // In this case the generic implementation via typeSpecifier fails, because the argument types cannot be narrowed down. if ($constantNeedleTypesCount === 1 && $constantHaystackTypesCount === 1) { - if ($isNeedleSupertype->yes()) { - return true; + if ($isNeedleSupertype->result->yes()) { + return [true, $isNeedleSupertype->reasons]; } - if ($isNeedleSupertype->no()) { - return false; + if ($isNeedleSupertype->result->no()) { + return [false, $isNeedleSupertype->reasons]; } } - return null; + return [null, []]; } } if (!$haystackType instanceof ConstantArrayType || count($haystackType->getValueTypes()) > 0) { $haystackArrayTypes = $haystackType->getArrays(); if (count($haystackArrayTypes) === 1 && $haystackArrayTypes[0]->getIterableValueType() instanceof NeverType) { - return null; + return [null, []]; } - if ($isNeedleSupertype->maybe() || $isNeedleSupertype->yes()) { + if ($isNeedleSupertype->result->maybe() || $isNeedleSupertype->result->yes()) { foreach ($haystackArrayTypes as $haystackArrayType) { if ($haystackArrayType instanceof ConstantArrayType) { foreach ($haystackArrayType->getValueTypes() as $i => $haystackArrayValueType) { @@ -165,18 +168,18 @@ public function findSpecifiedType( } } - return null; + return [null, []]; } } - if ($isNeedleSupertype->yes()) { + if ($isNeedleSupertype->result->yes()) { $hasConstantNeedleTypes = $constantNeedleTypesCount > 0; $hasConstantHaystackTypes = $constantHaystackTypesCount > 0; if ( (!$hasConstantNeedleTypes && !$hasConstantHaystackTypes) || $hasConstantNeedleTypes !== $hasConstantHaystackTypes ) { - return null; + return [null, []]; } } } @@ -187,7 +190,7 @@ public function findSpecifiedType( if ($objectType instanceof ConstantStringType && !$this->reflectionProvider->hasClass($objectType->getValue()) ) { - return false; + return [false, []]; } $methodArg = $node->getArgs()[1]->value; @@ -200,11 +203,11 @@ public function findSpecifiedType( if ($objectType->getObjectClassNames() !== []) { if ($objectType->hasMethod($methodType->getValue())->yes()) { - return true; + return [true, []]; } if ($objectType->hasMethod($methodType->getValue())->no()) { - return false; + return [false, []]; } } @@ -220,7 +223,7 @@ public function findSpecifiedType( if ($genericType instanceof TypeWithClassName) { if ($genericType->hasMethod($methodType->getValue())->yes()) { - return true; + return [true, []]; } $classReflection = $genericType->getClassReflection(); @@ -228,7 +231,7 @@ public function findSpecifiedType( $classReflection !== null && $classReflection->isFinal() && $genericType->hasMethod($methodType->getValue())->no()) { - return false; + return [false, []]; } } } @@ -245,7 +248,7 @@ public function findSpecifiedType( // don't validate types on overwrite if ($specifiedTypes->shouldOverwrite()) { - return null; + return [null, []]; } $sureTypes = $specifiedTypes->getSureTypes(); @@ -254,15 +257,15 @@ public function findSpecifiedType( $rootExpr = $specifiedTypes->getRootExpr(); if ($rootExpr !== null) { if (self::isSpecified($typeSpecifierScope, $node, $rootExpr)) { - return null; + return [null, []]; } $rootExprType = ($this->treatPhpDocTypesAsCertain ? $scope->getType($rootExpr) : $scope->getNativeType($rootExpr)); if ($rootExprType instanceof ConstantBooleanType) { - return $rootExprType->getValue(); + return [$rootExprType->getValue(), []]; } - return null; + return [null, []]; } $results = []; @@ -282,7 +285,12 @@ public function findSpecifiedType( /** @var Type $resultType */ $resultType = $sureType[1]; - $results[] = $resultType->isSuperTypeOf($argumentType)->result; + $isSuperType = $resultType->isSuperTypeOf($argumentType); + if ($isSuperType->result->no()) { + return [false, $isSuperType->reasons]; + } + + $results[] = $isSuperType->result; } foreach ($sureNotTypes as $sureNotType) { @@ -300,15 +308,20 @@ public function findSpecifiedType( /** @var Type $resultType */ $resultType = $sureNotType[1]; - $results[] = $resultType->isSuperTypeOf($argumentType)->negate()->result; + $isSuperType = $resultType->isSuperTypeOf($argumentType); + if ($isSuperType->result->yes()) { + return [false, $isSuperType->reasons]; + } + + $results[] = $isSuperType->result->negate(); } if (count($results) === 0) { - return null; + return [null, []]; } $result = TrinaryLogic::createYes()->and(...$results); - return $result->maybe() ? null : $result->yes(); + return $result->maybe() ? [null, []] : [$result->yes(), []]; } private static function isSpecified(Scope $scope, Expr $node, Expr $expr): bool diff --git a/src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php b/src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php index 4b79d98acb..dcb43ff110 100644 --- a/src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php +++ b/src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php @@ -10,6 +10,7 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\ShouldNotHappenException; +use function count; use function sprintf; /** @@ -38,17 +39,21 @@ public function processNode(Node $node, Scope $scope): array return []; } - $isAlways = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $node); + [$isAlways, $reasons] = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $node); if ($isAlways === null) { return []; } - $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node): RuleErrorBuilder { + $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $reasons): RuleErrorBuilder { + if (count($reasons) > 0) { + $ruleErrorBuilder->acceptsReasonsTip($reasons); + } + if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } - $isAlways = $this->impossibleCheckTypeHelper->doNotTreatPhpDocTypesAsCertain()->findSpecifiedType($scope, $node); + $isAlways = $this->impossibleCheckTypeHelper->doNotTreatPhpDocTypesAsCertain()->findSpecifiedType($scope, $node)[0]; if ($isAlways !== null) { return $ruleErrorBuilder; } diff --git a/src/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRule.php b/src/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRule.php index e4b3721538..5430719a1a 100644 --- a/src/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRule.php +++ b/src/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRule.php @@ -10,6 +10,7 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\ShouldNotHappenException; +use function count; use function sprintf; /** @@ -38,17 +39,21 @@ public function processNode(Node $node, Scope $scope): array return []; } - $isAlways = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $node); + [$isAlways, $reasons] = $this->impossibleCheckTypeHelper->findSpecifiedType($scope, $node); if ($isAlways === null) { return []; } - $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node): RuleErrorBuilder { + $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $reasons): RuleErrorBuilder { + if (count($reasons) > 0) { + $ruleErrorBuilder->acceptsReasonsTip($reasons); + } + if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } - $isAlways = $this->impossibleCheckTypeHelper->doNotTreatPhpDocTypesAsCertain()->findSpecifiedType($scope, $node); + $isAlways = $this->impossibleCheckTypeHelper->doNotTreatPhpDocTypesAsCertain()->findSpecifiedType($scope, $node)[0]; if ($isAlways !== null) { return $ruleErrorBuilder; } diff --git a/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php b/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php index 9ba6ee4797..7d590f9fa6 100644 --- a/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php +++ b/src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php @@ -61,7 +61,7 @@ public function processNode(Node $node, Scope $scope): array $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $nodeTypeResult): RuleErrorBuilder { $reasons = $nodeTypeResult->reasons; if (count($reasons) > 0) { - return $ruleErrorBuilder->acceptsReasonsTip($reasons); + $ruleErrorBuilder->acceptsReasonsTip($reasons); } if (!$this->treatPhpDocTypesAsCertain) { diff --git a/src/Rules/RuleErrorBuilder.php b/src/Rules/RuleErrorBuilder.php index cb714dfd2c..bd7819733a 100644 --- a/src/Rules/RuleErrorBuilder.php +++ b/src/Rules/RuleErrorBuilder.php @@ -207,7 +207,7 @@ public function acceptsReasonsTip(array $reasons): self */ public function treatPhpDocTypesAsCertainTip(): self { - return $this->tip('Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.'); + return $this->addTip('Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.'); } /** diff --git a/src/Type/Accessory/AccessoryLowercaseStringType.php b/src/Type/Accessory/AccessoryLowercaseStringType.php index 97edeb39ba..daabef2a9a 100644 --- a/src/Type/Accessory/AccessoryLowercaseStringType.php +++ b/src/Type/Accessory/AccessoryLowercaseStringType.php @@ -31,6 +31,7 @@ use PHPStan\Type\Type; use PHPStan\Type\UnionType; use PHPStan\Type\VerbosityLevel; +use function sprintf; class AccessoryLowercaseStringType implements CompoundType, AccessoryType { @@ -96,7 +97,14 @@ public function isSubTypeOf(Type $otherType): IsSuperTypeOfResult return $otherType->isSuperTypeOf($this); } - return (new IsSuperTypeOfResult($otherType->isLowercaseString(), [])) + $isLowercase = $otherType->isLowercaseString(); + + return (new IsSuperTypeOfResult( + $isLowercase, + $otherType->isString()->yes() && $isLowercase->no() + ? [sprintf('%s is not lowercase.', $otherType->describe(VerbosityLevel::value()))] + : [], + )) ->and($otherType instanceof self ? IsSuperTypeOfResult::createYes() : IsSuperTypeOfResult::createMaybe()); } diff --git a/src/Type/Php/TypeSpecifyingFunctionsDynamicReturnTypeExtension.php b/src/Type/Php/TypeSpecifyingFunctionsDynamicReturnTypeExtension.php index a6ab212328..0edde68456 100644 --- a/src/Type/Php/TypeSpecifyingFunctionsDynamicReturnTypeExtension.php +++ b/src/Type/Php/TypeSpecifyingFunctionsDynamicReturnTypeExtension.php @@ -57,7 +57,7 @@ public function getTypeFromFunctionCall( $isAlways = $this->getHelper()->findSpecifiedType( $scope, $functionCall, - ); + )[0]; if ($isAlways === null) { return null; } diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php index 2551aea65b..7a10c678ab 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php @@ -977,4 +977,18 @@ public function testBug8954(): void $this->analyse([__DIR__ . '/data/bug-8954.php'], []); } + public function testBug11799(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-11799.php'], [ + [ + "Call to function in_array() with arguments string, array{'publishDate', 'approvedAt', 'allowedValues'} and true will always evaluate to false.", + 11, + "• 'publishDate' is not lowercase. +• 'approvedAt' is not lowercase. +• 'allowedValues' is not lowercase.", + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php index e8efadec21..3ca6d6c2e6 100644 --- a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php @@ -273,7 +273,8 @@ public function testStrictComparison(): void [ 'Strict comparison using === between lowercase-string|false and \'AB\' will always evaluate to false.', 1014, - $tipText, + "• 'AB' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], [ 'Strict comparison using === between mixed and null will always evaluate to false.', @@ -879,27 +880,32 @@ public function testLowercaseString(): void [ "Strict comparison using === between lowercase-string and 'AB' will always evaluate to false.", 10, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'AB' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], [ "Strict comparison using === between 'AB' and lowercase-string will always evaluate to false.", 11, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'AB' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], [ "Strict comparison using !== between 'AB' and lowercase-string will always evaluate to true.", 12, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'AB' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], [ "Strict comparison using === between lowercase-string and 'aBc' will always evaluate to false.", 15, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'aBc' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], [ "Strict comparison using !== between lowercase-string and 'aBc' will always evaluate to true.", 16, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'aBc' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], ]; @@ -907,13 +913,15 @@ public function testLowercaseString(): void $errors[] = [ "Strict comparison using === between lowercase-string|false and 'AB' will always evaluate to false.", 28, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'AB' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ]; } else { $errors[] = [ "Strict comparison using === between lowercase-string and 'AB' will always evaluate to false.", 28, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'AB' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ]; } @@ -983,15 +991,18 @@ public function testHashing(): void [ "Strict comparison using === between lowercase-string&non-falsy-string and 'ABC' will always evaluate to false.", 9, + "'ABC' is not lowercase.", ], [ "Strict comparison using === between (lowercase-string&non-falsy-string)|false and 'ABC' will always evaluate to false.", 12, + "'ABC' is not lowercase.", ], [ "Strict comparison using === between (lowercase-string&non-falsy-string)|(non-falsy-string&numeric-string) and 'A' will always evaluate to false.", 31, - 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + "• 'A' is not lowercase. +• Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.", ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/bug-11799.php b/tests/PHPStan/Rules/Comparison/data/bug-11799.php new file mode 100644 index 0000000000..9d2b2168b8 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-11799.php @@ -0,0 +1,16 @@ +