Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decorate ImpossibleCheckTypeHelper with reason #3579

Draft
wants to merge 3 commits into
base: 1.12.x
Choose a base branch
from
Draft

Decorate ImpossibleCheckTypeHelper with reason #3579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
902e6c3
Decorate with reason
VincentLanglet Oct 20, 2024
12271ac
POC
VincentLanglet Oct 20, 2024
3088f94
Fix
VincentLanglet Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Rules/Comparison/ConstantConditionRuleHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,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;
}
Expand Down
11 changes: 8 additions & 3 deletions src/Rules/Comparison/ImpossibleCheckTypeFunctionCallRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use PHPStan\Parser\LastConditionVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use function count;
use function sprintf;
use function strtolower;

Expand Down Expand Up @@ -41,17 +42,21 @@ public function processNode(Node $node, Scope $scope): array
if (strtolower($functionName) === 'is_a') {
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) {
return $ruleErrorBuilder->acceptsReasonsTip($reasons);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied 6ac62d3...dc5d8f4#diff-6f5d047ab5b60f37c6dca68882f80e1f7ba2d946e0ffb91290d39589cf55acc2R58-R60

but looking at the failing test, I feel like we want to keep the treatPhpDocTypesAsCertainTip @ondrejmirtes , no ?
(So maybe the StrictComparisonOfDifferentTypesRule too ?)

Currently I dunno if I need to

  • Update the tests.
  • Or update this line of code.

Your call.

}

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;
}
Expand Down
83 changes: 48 additions & 35 deletions src/Rules/Comparison/ImpossibleCheckTypeHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,17 @@ public function __construct(
{
}

/**
* @return array{bool|null, list<string>}
*/
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) {
Expand All @@ -69,34 +72,34 @@ 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',
'interface_exists',
'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;
Expand All @@ -113,36 +116,36 @@ public function findSpecifiedType(
|| $haystackType->getIterableValueType()->isEnum()->yes();

if (!$isStrictComparison) {
return null;
return [null, []];
}

$valueType = $haystackType->getIterableValueType();
$constantNeedleTypesCount = count($needleType->getFiniteTypes());
$constantHaystackTypesCount = count($valueType->getFiniteTypes());
$isNeedleSupertype = $needleType->isSuperTypeOf($valueType);
$isNeedleSupertype = $needleType->isSuperTypeOfWithReason($valueType);
if ($haystackType->isConstantArray()->no()) {
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) {
Expand All @@ -164,18 +167,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, []];
}
}
}
Expand All @@ -186,7 +189,7 @@ public function findSpecifiedType(
if ($objectType instanceof ConstantStringType
&& !$this->reflectionProvider->hasClass($objectType->getValue())
) {
return false;
return [false, []];
}

$methodArg = $node->getArgs()[1]->value;
Expand All @@ -199,11 +202,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, []];
}
}

Expand All @@ -219,15 +222,15 @@ public function findSpecifiedType(

if ($genericType instanceof TypeWithClassName) {
if ($genericType->hasMethod($methodType->getValue())->yes()) {
return true;
return [true, []];
}

$classReflection = $genericType->getClassReflection();
if (
$classReflection !== null
&& $classReflection->isFinal()
&& $genericType->hasMethod($methodType->getValue())->no()) {
return false;
return [false, []];
}
}
}
Expand All @@ -240,7 +243,7 @@ public function findSpecifiedType(

// don't validate types on overwrite
if ($specifiedTypes->shouldOverwrite()) {
return null;
return [null, []];
}

$sureTypes = $specifiedTypes->getSureTypes();
Expand All @@ -249,15 +252,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 = [];
Expand All @@ -277,7 +280,12 @@ public function findSpecifiedType(
/** @var Type $resultType */
$resultType = $sureType[1];

$results[] = $resultType->isSuperTypeOf($argumentType);
$isSuperType = $resultType->isSuperTypeOfWithReason($argumentType);
if ($isSuperType->result->no()) {
return [false, $isSuperType->reasons];
}

$results[] = $isSuperType->result;
}

foreach ($sureNotTypes as $sureNotType) {
Expand All @@ -295,15 +303,20 @@ public function findSpecifiedType(
/** @var Type $resultType */
$resultType = $sureNotType[1];

$results[] = $resultType->isSuperTypeOf($argumentType)->negate();
$isSuperType = $resultType->isSuperTypeOfWithReason($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
Expand Down
11 changes: 8 additions & 3 deletions src/Rules/Comparison/ImpossibleCheckTypeMethodCallRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;
use function count;
use function sprintf;

/**
Expand Down Expand Up @@ -39,17 +40,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) {
return $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;
}
Expand Down
11 changes: 8 additions & 3 deletions src/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\ShouldNotHappenException;
use function count;
use function sprintf;

/**
Expand Down Expand Up @@ -39,17 +40,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) {
return $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;
}
Expand Down
10 changes: 9 additions & 1 deletion src/Type/Accessory/AccessoryLowercaseStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use PHPStan\Type\Type;
use PHPStan\Type\UnionType;
use PHPStan\Type\VerbosityLevel;
use function sprintf;

class AccessoryLowercaseStringType implements CompoundType, AccessoryType
{
Expand Down Expand Up @@ -110,7 +111,14 @@ public function isSubTypeOfWithReason(Type $otherType): IsSuperTypeOfResult
return $otherType->isSuperTypeOfWithReason($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());
}

Expand Down
2 changes: 1 addition & 1 deletion src/Type/Php/TypeSpecifyingFunctionsDynamicReturnTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public function getTypeFromFunctionCall(
$isAlways = $this->getHelper()->findSpecifiedType(
$scope,
$functionCall,
);
)[0];
if ($isAlways === null) {
return null;
}
Expand Down
15 changes: 15 additions & 0 deletions tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1102,4 +1102,19 @@ public function testAlwaysTruePregMatch(): void
$this->analyse([__DIR__ . '/data/always-true-preg-match.php'], []);
}

public function testBug11799(): void
{
$this->checkAlwaysTrueCheckTypeFunctionCall = true;
$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.",
Comment on lines +1113 to +1115
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the comparison is between 'publishDate'|'approvedAt'|'allowedValues' and lowercase string, and because of how UnionType::isSuperTypeOfWithReason works

$result = IsSuperTypeOfResult::createNo()->or(...array_map(static fn (Type $innerType) => $innerType->isSuperTypeOfWithReason($otherType), $this->types));
		if ($result->yes()) {
			return $result;
		}

we have a list with three reasons.

Is it ok for you @ondrejmirtes ?

],
]);
}

}
Loading
Loading