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.

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

Replace error-prone instanceof in Rules classes #3858

Open
wants to merge 22 commits into
base: 2.1.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7fd86ee
Replace error-prone instanceof in Rules classes
zonuexe Mar 6, 2025
c7d029f
wip
zonuexe Mar 6, 2025
9394d68
fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 6, 2025
b829cec
Update src/Rules/Api/NodeConnectingVisitorAttributesRule.php
zonuexe Mar 6, 2025
a6b2214
fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 6, 2025
0e3032a
fixup! fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 6, 2025
6d74a44
fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 6, 2025
4013a02
fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 6, 2025
ef06222
Revert src/Rules/MissingTypehintCheck.php
zonuexe Mar 6, 2025
635f751
foreach $type->getConstantArrays()
zonuexe Mar 7, 2025
ef358e4
Support multiple tags and improve interface handling in @phpstan-requ…
zonuexe Mar 7, 2025
a6c09fb
Added support for ConstantString unions in NodeConnectingVisitorAttri…
zonuexe Mar 7, 2025
2ea3057
fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 7, 2025
5139160
Added support for ConstantString unions in DuplicateKeysInLiteralArra…
zonuexe Mar 7, 2025
3de5d01
Added support for ConstantString unions in RequireExtendsRule
zonuexe Mar 7, 2025
0bb0d3c
fixup! Replace error-prone instanceof in Rules classes
zonuexe Mar 7, 2025
36d6f05
update baseline
zonuexe Mar 7, 2025
eade6d3
fix tests
zonuexe Mar 7, 2025
92bf49f
sort
zonuexe Mar 7, 2025
a7a4e32
fixup! fix tests
zonuexe Mar 7, 2025
0dd9a85
Fix
zonuexe Mar 11, 2025
5d96286
Avoid calling methods twice with variables
zonuexe Mar 11, 2025
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
68 changes: 1 addition & 67 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -411,30 +411,12 @@ parameters:
count: 1
path: src/Reflection/SignatureMap/Php8SignatureMapProvider.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantStringType is error\-prone and deprecated\. Use Type\:\:getConstantStrings\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/Api/NodeConnectingVisitorAttributesRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\ConstantScalarType is error\-prone and deprecated\. Use Type\:\:isConstantScalarValue\(\) or Type\:\:getConstantScalarTypes\(\) or Type\:\:getConstantScalarValues\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantBooleanType is error\-prone and deprecated\. Use Type\:\:isTrue\(\) or Type\:\:isFalse\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 2
path: src/Rules/Classes/ImpossibleInstanceOfRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\ObjectType is error\-prone and deprecated\. Use Type\:\:isObject\(\) or Type\:\:getObjectClassNames\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 2
path: src/Rules/Classes/RequireExtendsRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\ObjectType is error\-prone and deprecated\. Use Type\:\:isObject\(\) or Type\:\:getObjectClassNames\(\) instead\.$#'
identifier: phpstanApi.instanceofType
Expand All @@ -459,12 +441,6 @@ parameters:
count: 6
path: src/Rules/Comparison/BooleanOrConstantConditionRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantBooleanType is error\-prone and deprecated\. Use Type\:\:isTrue\(\) or Type\:\:isFalse\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 2
path: src/Rules/Comparison/ConstantLooseComparisonRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantBooleanType is error\-prone and deprecated\. Use Type\:\:isTrue\(\) or Type\:\:isFalse\(\) instead\.$#'
identifier: phpstanApi.instanceofType
Expand Down Expand Up @@ -492,7 +468,7 @@ parameters:
-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantBooleanType is error\-prone and deprecated\. Use Type\:\:isTrue\(\) or Type\:\:isFalse\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 2
count: 1
path: src/Rules/Comparison/ImpossibleCheckTypeHelper.php

-
Expand Down Expand Up @@ -645,18 +621,6 @@ parameters:
count: 1
path: src/Rules/Methods/StaticMethodCallCheck.php

-
message: '#^Doing instanceof PHPStan\\Type\\ObjectType is error\-prone and deprecated\. Use Type\:\:isObject\(\) or Type\:\:getObjectClassNames\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/PhpDoc/RequireExtendsCheck.php

-
message: '#^Doing instanceof PHPStan\\Type\\ObjectType is error\-prone and deprecated\. Use Type\:\:isObject\(\) or Type\:\:getObjectClassNames\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/PhpDoc/RequireImplementsDefinitionTraitRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\Generic\\GenericObjectType is error\-prone and deprecated\.$#'
identifier: phpstanApi.instanceofType
Expand All @@ -675,36 +639,6 @@ parameters:
count: 1
path: src/Rules/RuleLevelHelper.php

-
message: '#^Doing instanceof PHPStan\\Type\\ObjectType is error\-prone and deprecated\. Use Type\:\:isObject\(\) or Type\:\:getObjectClassNames\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 2
path: src/Rules/RuleLevelHelper.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantBooleanType is error\-prone and deprecated\. Use Type\:\:isTrue\(\) or Type\:\:isFalse\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/TooWideTypehints/TooWideMethodReturnTypehintRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantStringType is error\-prone and deprecated\. Use Type\:\:getConstantStrings\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/UnusedFunctionParametersCheck.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantArrayType is error\-prone and deprecated\. Use Type\:\:getConstantArrays\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/Variables/CompactVariablesRule.php

-
message: '#^Doing instanceof PHPStan\\Type\\Constant\\ConstantStringType is error\-prone and deprecated\. Use Type\:\:getConstantStrings\(\) instead\.$#'
identifier: phpstanApi.instanceofType
count: 1
path: src/Rules/Variables/CompactVariablesRule.php

-
message: '''
#^Call to deprecated method assertFileNotExists\(\) of class PHPUnit\\Framework\\Assert\:
Expand Down
48 changes: 25 additions & 23 deletions src/Rules/Api/NodeConnectingVisitorAttributesRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use PHPStan\Analyser\Scope;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantStringType;
use PHPStan\Type\ObjectType;
use function array_keys;
use function in_array;
Expand Down Expand Up @@ -41,37 +40,40 @@ public function processNode(Node $node, Scope $scope): array
if (!isset($args[0])) {
return [];
}

$messages = [];
$argType = $scope->getType($args[0]->value);
if (!$argType instanceof ConstantStringType) {
return [];
}
if (!in_array($argType->getValue(), ['parent', 'previous', 'next'], true)) {
return [];
}
if (!$scope->isInClass()) {
return [];
}
foreach ($argType->getConstantStrings() as $constantString) {
$argValue = $constantString->getValue();
if (!in_array($argValue, ['parent', 'previous', 'next'], true)) {
continue;
}

$classReflection = $scope->getClassReflection();
$hasPhpStanInterface = false;
foreach (array_keys($classReflection->getInterfaces()) as $interfaceName) {
if (!str_starts_with($interfaceName, 'PHPStan\\')) {
if (!$scope->isInClass()) {
continue;
}

$hasPhpStanInterface = true;
}
$classReflection = $scope->getClassReflection();
$hasPhpStanInterface = false;
foreach (array_keys($classReflection->getInterfaces()) as $interfaceName) {
if (!str_starts_with($interfaceName, 'PHPStan\\')) {
continue;
}

if (!$hasPhpStanInterface) {
return [];
}
$hasPhpStanInterface = true;
}

return [
RuleErrorBuilder::message(sprintf('Node attribute \'%s\' is no longer available.', $argType->getValue()))
if (!$hasPhpStanInterface) {
continue;
}

$messages[] = RuleErrorBuilder::message(sprintf('Node attribute \'%s\' is no longer available.', $argValue))
->identifier('phpParser.nodeConnectingAttribute')
->tip('See: https://phpstan.org/blog/preprocessing-ast-for-custom-rules')
->build(),
];
->build();
}

return $messages;
}

}
41 changes: 20 additions & 21 deletions src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantIntegerType;
use PHPStan\Type\ConstantScalarType;
use function array_keys;
use function count;
use function implode;
use function is_int;
use function max;
use function sprintf;
use function var_export;
Expand Down Expand Up @@ -70,38 +70,37 @@ public function processNode(Node $node, Scope $scope): array
}
} else {
$keyType = $itemNode->getScope()->getType($key);

$arrayKeyValue = $keyType->toArrayKey();
if ($arrayKeyValue instanceof ConstantIntegerType) {
$arrayKeyValues = $keyType->toArrayKey()->getConstantScalarValues();
if (count($arrayKeyValues) === 1 && is_int($arrayKeyValues[0])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it better/possible to handle union of scalar values here (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the following?

modified   src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php
@@ -71,10 +71,12 @@ final class DuplicateKeysInLiteralArraysRule implements Rule
 			} else {
 				$keyType = $itemNode->getScope()->getType($key);
 				$arrayKeyValues = $keyType->toArrayKey()->getConstantScalarValues();
-				if (count($arrayKeyValues) === 1 && is_int($arrayKeyValues[0])) {
-					$autoGeneratedIndex = $autoGeneratedIndex === null
-						? $arrayKeyValues[0]
-						: max($autoGeneratedIndex, $arrayKeyValues[0]);
+				foreach ($arrayKeyValues as $arrayKeyValue) {
+					if (is_int($arrayKeyValue)) {
+						$autoGeneratedIndex = $autoGeneratedIndex === null
+							? $arrayKeyValue
+							: max($autoGeneratedIndex, $arrayKeyValue);
+					}
 				}
 			}
Or is it type-level arithmetic?
modified   src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php
@@ -42,7 +42,6 @@ final class DuplicateKeysInLiteralArraysRule implements Rule
 		$valueLines = [];
 
 		/**
-		 * @var int|false|null $autoGeneratedIndex
 		 * - An int value represent the biggest integer used as array key.
 		 *   When no key is provided this value + 1 will be used.
 		 * - Null is used as initializer instead of 0 to avoid issue with negative keys.
@@ -63,27 +62,37 @@ final class DuplicateKeysInLiteralArraysRule implements Rule
 				}
 
 				if ($autoGeneratedIndex === null) {
-					$autoGeneratedIndex = 0;
-					$keyType = new ConstantIntegerType(0);
+					$autoGeneratedIndex = new ConstantIntegerType(0);
 				} else {
-					$keyType = new ConstantIntegerType(++$autoGeneratedIndex);
+					$autoGeneratedIndex = $scope->getType(new Plus(
+						new Int_(1),
+						new TypeExpr($autoGeneratedIndex),
+					));
 				}
+				$keyType = $autoGeneratedIndex;
 			} else {
 				$keyType = $itemNode->getScope()->getType($key);
-				$arrayKeyValues = $keyType->toArrayKey()->getConstantScalarValues();
-				if (count($arrayKeyValues) === 1 && is_int($arrayKeyValues[0])) {
+				$arrayKeyType = $keyType->toArrayKey();
+				if ($autoGeneratedIndex !== false && $arrayKeyType->isInteger()->yes()) {
 					$autoGeneratedIndex = $autoGeneratedIndex === null
-						? $arrayKeyValues[0]
-						: max($autoGeneratedIndex, $arrayKeyValues[0]);
+						? $keyType
+						: $scope->getType(new FuncCall(
+							new Name('\max'),
+							[
+								new Arg(new TypeExpr($autoGeneratedIndex)),
+								new Arg(new TypeExpr($arrayKeyType)),
+							]
+						));
 				}
 			}

$autoGeneratedIndex is explained as follows:

		/**
		 * - An int value represent the biggest integer used as array key.
		 *   When no key is provided this value + 1 will be used.
		 * - Null is used as initializer instead of 0 to avoid issue with negative keys.
		 * - False means a non-scalar value was encountered and we cannot be sure of the next keys.
		 */
		$autoGeneratedIndex = null;

Since the key value actually changes depending on the input value, more appropriate typing is possible with type operations. However, since DuplicateKeysInLiteralArraysRule has some subtle behavior, I would like to keep the changes in this branch to a minimum.

$autoGeneratedIndex = $autoGeneratedIndex === null
? $arrayKeyValue->getValue()
: max($autoGeneratedIndex, $arrayKeyValue->getValue());
? $arrayKeyValues[0]
: max($autoGeneratedIndex, $arrayKeyValues[0]);
}
}

if (!$keyType instanceof ConstantScalarType) {
if (count($keyType->getConstantScalarValues()) === 0) {
$autoGeneratedIndex = false;
continue;
}

$value = $keyType->getValue();
$printedValue = $key !== null
? $this->exprPrinter->printExpr($key)
: $value;
foreach ($keyType->getConstantScalarValues() as $value) {
$printedValue = $key !== null
? $this->exprPrinter->printExpr($key)
: $value;
$printedValues[$value][] = $printedValue;

$printedValues[$value][] = $printedValue;
if (!isset($valueLines[$value])) {
$valueLines[$value] = $item->getStartLine();
}

if (!isset($valueLines[$value])) {
$valueLines[$value] = $item->getStartLine();
}
$previousCount = count($values);
$values[$value] = $printedValue;
if ($previousCount !== count($values)) {
continue;
}

$previousCount = count($values);
$values[$value] = $printedValue;
if ($previousCount !== count($values)) {
continue;
$duplicateKeys[$value] = true;
}

$duplicateKeys[$value] = true;
}

$messages = [];
Expand Down
65 changes: 32 additions & 33 deletions src/Rules/Classes/RequireExtendsRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use PHPStan\Node\InClassNode;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\ObjectType;
use PHPStan\Type\VerbosityLevel;
use function sprintf;

Expand Down Expand Up @@ -35,49 +34,49 @@ public function processNode(Node $node, Scope $scope): array
$extendsTags = $interface->getRequireExtendsTags();
foreach ($extendsTags as $extendsTag) {
$type = $extendsTag->getType();
if (!$type instanceof ObjectType) {
continue;
}
foreach ($type->getObjectClassNames() as $className) {
if ($classReflection->is($className)) {
continue;
}

if ($classReflection->is($type->getClassName())) {
continue;
}
$errors[] = RuleErrorBuilder::message(
sprintf(
'Interface %s requires implementing class to extend %s, but %s does not.',
$interface->getDisplayName(),
$type->describe(VerbosityLevel::typeOnly()),
$classReflection->getDisplayName(),
),
)
->identifier('class.missingExtends')
->build();

$errors[] = RuleErrorBuilder::message(
sprintf(
'Interface %s requires implementing class to extend %s, but %s does not.',
$interface->getDisplayName(),
$type->describe(VerbosityLevel::typeOnly()),
$classReflection->getDisplayName(),
),
)
->identifier('class.missingExtends')
->build();
break;
}
}
}

foreach ($classReflection->getTraits(true) as $trait) {
$extendsTags = $trait->getRequireExtendsTags();
foreach ($extendsTags as $extendsTag) {
$type = $extendsTag->getType();
if (!$type instanceof ObjectType) {
continue;
}
foreach ($type->getObjectClassNames() as $className) {
if ($classReflection->is($className)) {
continue;
}

if ($classReflection->is($type->getClassName())) {
continue;
}
$errors[] = RuleErrorBuilder::message(
sprintf(
'Trait %s requires using class to extend %s, but %s does not.',
$trait->getDisplayName(),
$type->describe(VerbosityLevel::typeOnly()),
$classReflection->getDisplayName(),
),
)
->identifier('class.missingExtends')
->build();

$errors[] = RuleErrorBuilder::message(
sprintf(
'Trait %s requires using class to extend %s, but %s does not.',
$trait->getDisplayName(),
$type->describe(VerbosityLevel::typeOnly()),
$classReflection->getDisplayName(),
),
)
->identifier('class.missingExtends')
->build();
break;
}
}
}

Expand Down
7 changes: 3 additions & 4 deletions src/Rules/Comparison/ConstantLooseComparisonRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use PHPStan\Parser\LastConditionVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\VerbosityLevel;
use function sprintf;

Expand Down Expand Up @@ -37,7 +36,7 @@ public function processNode(Node $node, Scope $scope): array
}

$nodeType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node) : $scope->getNativeType($node);
if (!$nodeType instanceof ConstantBooleanType) {
if (!$nodeType->isTrue()->yes() && !$nodeType->isFalse()->yes()) {
return [];
}

Expand All @@ -47,7 +46,7 @@ public function processNode(Node $node, Scope $scope): array
}

$instanceofTypeWithoutPhpDocs = $scope->getNativeType($node);
if ($instanceofTypeWithoutPhpDocs instanceof ConstantBooleanType) {
if ($instanceofTypeWithoutPhpDocs->isTrue()->yes() || $instanceofTypeWithoutPhpDocs->isFalse()->yes()) {
return $ruleErrorBuilder;
}
if (!$this->treatPhpDocTypesAsCertainTip) {
Expand All @@ -57,7 +56,7 @@ public function processNode(Node $node, Scope $scope): array
return $ruleErrorBuilder->treatPhpDocTypesAsCertainTip();
};

if (!$nodeType->getValue()) {
if ($nodeType->isFalse()->yes()) {
return [
$addTip(RuleErrorBuilder::message(sprintf(
'Loose comparison using %s between %s and %s will always evaluate to false.',
Expand Down
5 changes: 3 additions & 2 deletions src/Rules/Comparison/ImpossibleCheckTypeHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,12 @@ public function findSpecifiedType(
if ($functionName === 'assert' && $argsCount >= 1) {
$arg = $node->getArgs()[0]->value;
$assertValue = ($this->treatPhpDocTypesAsCertain ? $scope->getType($arg) : $scope->getNativeType($arg))->toBoolean();
if (!$assertValue instanceof ConstantBooleanType) {
if (! $assertValue->isTrue()->yes() && ! $assertValue->isFalse()->yes()) {
return null;
}

return $assertValue->getValue();
/** @var bool */
return $assertValue->getConstantScalarValues()[0];
Comment on lines +76 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

You can return $assertValue->isTrue()->yes() if you want to avoid the var phpdoc

}
if (in_array($functionName, [
'class_exists',
Expand Down
Loading