Replace error-prone instanceof in Rules classes #3858

zonuexe · 2025-03-06T16:31:03Z

Reduce usage of $type instanceof *Type pattern in Rules classes.

zonuexe · 2025-03-06T16:33:24Z

src/Rules/TooWideTypehints/TooWideMethodReturnTypehintRule.php

-			&& !$isFirstDeclaration
+			!$isFirstDeclaration
+			&& !$method->isPrivate()
+			&& ($returnType->isNull()->yes() || $returnType->isTrue()->yes() || $returnType->isFalse()->yes())


Logical operators are short-circuit evaluated, so rearranging expressions can lead to micro-optimizations.

ondrejmirtes

I don't want to replace these calls by just accessing the first element. Union types should be properly handled. Multiple values should be iterated over.

src/Rules/Api/NodeConnectingVisitorAttributesRule.php

zonuexe · 2025-03-06T16:53:31Z

Since ConstantArrayType and ConstantStringType each return return [$this]; from the getConstant*() methods, I thought it would be effective to simply replace them with count() === 0 or count() === 1 to minimize the change difference, but I also prefer to replace it with foreach instead of $types[0], so I will change my approach and fix it that way.

zonuexe · 2025-03-07T18:47:31Z

src/Rules/PhpDoc/RequireExtendsCheck.php

-			$class = $type->getClassName();
-			$referencedClassReflection = $type->getClassReflection();
+			$classNames = $type->getObjectClassNames();
+			sort($classNames);


I am sorting this array because tests seem to fail randomly.

https://github.com/phpstan/phpstan-src/actions/runs/13726816592/job/38395109333?pr=3858

zonuexe · 2025-03-07T18:50:41Z

MissingTypehintCheck was reverted in this branch because the literal substitution seemed to change its behavior. a408a58

phpstan-bot · 2025-03-07T18:50:57Z

This pull request has been marked as ready for review.

staabm · 2025-03-08T07:44:15Z

src/Rules/PhpDoc/RequireExtendsCheck.php

 				$errors[] = RuleErrorBuilder::message(sprintf('PHPDoc tag @phpstan-require-extends contains non-object type %s.', $type->describe(VerbosityLevel::typeOnly())))
 					->identifier('requireExtends.nonObject')
 					->build();
 				continue;
 			}

-			$class = $type->getClassName();
-			$referencedClassReflection = $type->getClassReflection();
+			$classNames = $type->getObjectClassNames();


Introduce this var earlier so you don't need to call getObjectClassNames twice (methods called on Type can get expensive)

staabm · 2025-03-08T07:45:01Z

src/Rules/PhpDoc/RequireImplementsDefinitionTraitRule.php

@@ -49,34 +51,38 @@ public function processNode(Node $node, Scope $scope): array
 		$errors = [];
 		foreach ($implementsTags as $implementsTag) {
 			$type = $implementsTag->getType();
-			if (!$type instanceof ObjectType) {
+			if (count($type->getObjectClassNames()) === 0) {


Introduce a var so you don't need to call getObjectClassNames twice

staabm · 2025-03-08T07:45:44Z

src/Rules/Variables/CompactVariablesRule.php

 	 */
 	private function findConstantStrings(Type $type): array
 	{
-		if ($type instanceof ConstantStringType) {
-			return [$type];
+		if (count($type->getConstantStrings()) > 0) {


Introduce a var so you don't need to call getConstantStrings twice

VincentLanglet · 2025-03-09T22:05:01Z

src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php

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


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

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.

VincentLanglet · 2025-03-09T22:08:53Z

src/Rules/Comparison/ConstantLooseComparisonRule.php

@@ -57,7 +57,7 @@ public function processNode(Node $node, Scope $scope): array
 			return $ruleErrorBuilder->treatPhpDocTypesAsCertainTip();
 		};

-		if (!$nodeType->getValue()) {
+		if (in_array(false, $nodeType->getConstantScalarValues(), true)) {


I'm not sure but isn't $nodeType->isFalse()->yes() better ?

That's correct, thanks!

VincentLanglet · 2025-03-09T22:13:27Z

src/Rules/Comparison/ImpossibleCheckTypeHelper.php

-					$assertValue = ($this->treatPhpDocTypesAsCertain ? $scope->getType($arg) : $scope->getNativeType($arg))->toBoolean();
-					if (!$assertValue instanceof ConstantBooleanType) {
+					$assertValues = ($this->treatPhpDocTypesAsCertain ? $scope->getType($arg) : $scope->getNativeType($arg))->getConstantScalarValues();
+					if (count($assertValues) !== 1 || !is_bool($assertValues[0])) {


You could maybe have kept the toBoolean and checked

$assertValue->isTrue()->yes() || $assertValue->isFalse()->yes()

Co-authored-by: Markus Staab <[email protected]>

…ire-extends

…butesRule

…ysRule

Co-authored-by: Vincent Langlet <[email protected]>

Co-authored-by: Markus Staab <[email protected]>

phpstan-bot · 2025-03-11T22:58:24Z

This pull request has been marked as ready for review.

VincentLanglet · 2025-03-12T08:29:37Z

src/Rules/Comparison/ImpossibleCheckTypeHelper.php

+					/** @var bool */
+					return $assertValue->getConstantScalarValues()[0];


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

zonuexe commented Mar 6, 2025

View reviewed changes

zonuexe marked this pull request as draft March 6, 2025 16:34

ondrejmirtes requested changes Mar 6, 2025

View reviewed changes

zonuexe force-pushed the refactor/error-prone-instanceof-type branch from bbbe15f to 776e845 Compare March 6, 2025 16:46

staabm reviewed Mar 6, 2025

View reviewed changes

src/Rules/Api/NodeConnectingVisitorAttributesRule.php Outdated Show resolved Hide resolved

zonuexe force-pushed the refactor/error-prone-instanceof-type branch 7 times, most recently from 9fc4e20 to c57fe20 Compare March 7, 2025 18:00

zonuexe commented Mar 7, 2025

View reviewed changes

zonuexe marked this pull request as ready for review March 7, 2025 18:50

staabm reviewed Mar 8, 2025

View reviewed changes

VincentLanglet reviewed Mar 9, 2025

View reviewed changes

zonuexe and others added 11 commits March 12, 2025 01:26

Replace error-prone instanceof in Rules classes

7fd86ee

wip

c7d029f

fixup! Replace error-prone instanceof in Rules classes

9394d68

Update src/Rules/Api/NodeConnectingVisitorAttributesRule.php

b829cec

Co-authored-by: Markus Staab <[email protected]>

fixup! Replace error-prone instanceof in Rules classes

a6b2214

fixup! fixup! Replace error-prone instanceof in Rules classes

0e3032a

fixup! Replace error-prone instanceof in Rules classes

6d74a44

fixup! Replace error-prone instanceof in Rules classes

4013a02

Revert src/Rules/MissingTypehintCheck.php

ef06222

foreach $type->getConstantArrays()

635f751

Support multiple tags and improve interface handling in @phpstan-requ…

ef358e4

…ire-extends

zonuexe added 9 commits March 12, 2025 01:26

Added support for ConstantString unions in NodeConnectingVisitorAttri…

a6c09fb

…butesRule

fixup! Replace error-prone instanceof in Rules classes

2ea3057

Added support for ConstantString unions in DuplicateKeysInLiteralArra…

5139160

…ysRule

Added support for ConstantString unions in RequireExtendsRule

3de5d01

fixup! Replace error-prone instanceof in Rules classes

0bb0d3c

update baseline

36d6f05

fix tests

eade6d3

sort

92bf49f

fixup! fix tests

a7a4e32

zonuexe marked this pull request as draft March 11, 2025 16:26

zonuexe force-pushed the refactor/error-prone-instanceof-type branch from 83b0029 to b031d1d Compare March 11, 2025 21:03

zonuexe and others added 2 commits March 12, 2025 06:12

Fix

0dd9a85

Co-authored-by: Vincent Langlet <[email protected]>

Avoid calling methods twice with variables

5d96286

Co-authored-by: Markus Staab <[email protected]>

zonuexe force-pushed the refactor/error-prone-instanceof-type branch from b031d1d to 5d96286 Compare March 11, 2025 21:12

zonuexe marked this pull request as ready for review March 11, 2025 22:58

VincentLanglet reviewed Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace error-prone instanceof in Rules classes #3858

Replace error-prone instanceof in Rules classes #3858

zonuexe commented Mar 6, 2025

zonuexe Mar 6, 2025

ondrejmirtes left a comment

zonuexe commented Mar 6, 2025

zonuexe Mar 7, 2025

zonuexe commented Mar 7, 2025

phpstan-bot commented Mar 7, 2025

staabm Mar 8, 2025

staabm Mar 8, 2025

staabm Mar 8, 2025

zonuexe Mar 11, 2025

VincentLanglet Mar 9, 2025

zonuexe Mar 11, 2025

VincentLanglet Mar 9, 2025

zonuexe Mar 11, 2025

VincentLanglet Mar 9, 2025

phpstan-bot commented Mar 11, 2025

VincentLanglet Mar 12, 2025

		/** @var bool */
		return $assertValue->getConstantScalarValues()[0];

Replace error-prone instanceof in Rules classes #3858

Are you sure you want to change the base?

Replace error-prone instanceof in Rules classes #3858

Conversation

zonuexe commented Mar 6, 2025

Choose a reason for hiding this comment

ondrejmirtes left a comment

Choose a reason for hiding this comment

zonuexe commented Mar 6, 2025

Choose a reason for hiding this comment

zonuexe commented Mar 7, 2025

phpstan-bot commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phpstan-bot commented Mar 11, 2025

Choose a reason for hiding this comment