From 3597438d23b6be0f70067d8ae7d5a78797adcb64 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 10 Mar 2025 19:08:41 +0100 Subject: [PATCH 1/5] [CodeQuality] Add AddInstanceofAssertForNullableInstanceRector --- composer.json | 5 +- phpstan.neon | 13 +- ...eofAssertForNullableInstanceRectorTest.php | 28 ++ .../Fixture/add_instance_only_once.php.inc | 68 +++++ .../Fixture/call_on_nullable.php.inc | 62 ++++ .../Fixture/skip_int_nullabe.php.inc | 27 ++ .../Fixture/skip_non_nullable_type.php.inc | 24 ++ .../Source/SomeClassUsedInTests.php | 13 + .../config/configured_rule.php | 10 + .../Fixture/skip_sole_type.php.inc | 2 +- .../Fixture/union_type_mock.php.inc | 4 +- ...tanceofAssertForNullableInstanceRector.php | 270 ++++++++++++++++++ .../ValueObject/VariableNameToType.php | 24 ++ 13 files changed, 538 insertions(+), 12 deletions(-) create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/AddInstanceofAssertForNullableInstanceRectorTest.php create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/add_instance_only_once.php.inc create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/call_on_nullable.php.inc create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_int_nullabe.php.inc create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_non_nullable_type.php.inc create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Source/SomeClassUsedInTests.php create mode 100644 rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/config/configured_rule.php create mode 100644 rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php create mode 100644 rules/CodeQuality/ValueObject/VariableNameToType.php diff --git a/composer.json b/composer.json index ea05b0d6..3ef2b741 100644 --- a/composer.json +++ b/composer.json @@ -12,11 +12,12 @@ "phpstan/phpstan": "^2.1.8", "phpecs/phpecs": "^2.0", "phpstan/extension-installer": "^1.4", - "symplify/vendor-patches": "^11.3", + "symplify/vendor-patches": "^11.4", "tracy/tracy": "^2.10", "tomasvotruba/class-leak": "^1.2", "rector/swiss-knife": "^1.0", - "phpstan/phpstan-webmozart-assert": "^2.0" + "phpstan/phpstan-webmozart-assert": "^2.0", + "rector/type-perfect": "^2.0" }, "autoload": { "psr-4": { diff --git a/phpstan.neon b/phpstan.neon index 70ebfee0..655b005b 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -19,13 +19,12 @@ parameters: - */Fixture/* - */Expected/* -# to be enabled later once rector upgraded to use phpstan v2 -# # see https://github.com/rectorphp/type-perfect/ -# type_perfect: -# no_mixed: true -# null_over_false: true -# narrow_param: true -# narrow_return: true + # see https://github.com/rectorphp/type-perfect/ + type_perfect: + no_mixed: true + null_over_false: true + narrow_param: true + narrow_return: true ignoreErrors: # phpstan false positive diff --git a/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/AddInstanceofAssertForNullableInstanceRectorTest.php b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/AddInstanceofAssertForNullableInstanceRectorTest.php new file mode 100644 index 00000000..b6f0e61d --- /dev/null +++ b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/AddInstanceofAssertForNullableInstanceRectorTest.php @@ -0,0 +1,28 @@ +doTestFile($filePath); + } + + public static function provideData(): Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/add_instance_only_once.php.inc b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/add_instance_only_once.php.inc new file mode 100644 index 00000000..7922a205 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/add_instance_only_once.php.inc @@ -0,0 +1,68 @@ +getSomeObject(); + $value = $someObject->getSomeMethod(); + + $this->assertSame(123, $value); + + // we know the value here, no need to add instanceof + $value = $someObject->getSomeMethod(); + } + + private function getSomeObject(): ?SomeClassUsedInTests + { + if (mt_rand(0, 1)) { + return new SomeClassUsedInTests(); + } + + return null; + } +} + +?> +----- +getSomeObject(); + $this->assertInstanceof(\Rector\PHPUnit\Tests\CodeQuality\Rector\ClassMethod\AddInstanceofAssertForNullableInstanceRector\Source\SomeClassUsedInTests::class, $someObject); + $value = $someObject->getSomeMethod(); + + $this->assertSame(123, $value); + + // we know the value here, no need to add instanceof + $value = $someObject->getSomeMethod(); + } + + private function getSomeObject(): ?SomeClassUsedInTests + { + if (mt_rand(0, 1)) { + return new SomeClassUsedInTests(); + } + + return null; + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/call_on_nullable.php.inc b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/call_on_nullable.php.inc new file mode 100644 index 00000000..487bb889 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/call_on_nullable.php.inc @@ -0,0 +1,62 @@ +getSomeObject(); + $value = $someObject->getSomeMethod(); + + $this->assertSame(123, $value); + } + + private function getSomeObject(): ?SomeClassUsedInTests + { + if (mt_rand(0, 1)) { + return new SomeClassUsedInTests(); + } + + return null; + } +} + +?> +----- +getSomeObject(); + $this->assertInstanceof(\Rector\PHPUnit\Tests\CodeQuality\Rector\ClassMethod\AddInstanceofAssertForNullableInstanceRector\Source\SomeClassUsedInTests::class, $someObject); + $value = $someObject->getSomeMethod(); + + $this->assertSame(123, $value); + } + + private function getSomeObject(): ?SomeClassUsedInTests + { + if (mt_rand(0, 1)) { + return new SomeClassUsedInTests(); + } + + return null; + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_int_nullabe.php.inc b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_int_nullabe.php.inc new file mode 100644 index 00000000..40939b09 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_int_nullabe.php.inc @@ -0,0 +1,27 @@ +getValue(); + $value = $someObject->getSomeMethod(); + + $this->assertSame(123, $value); + } + + private function getValue(): ?int + { + if (mt_rand(0, 1)) { + return 1000; + } + + return null; + } +} diff --git a/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_non_nullable_type.php.inc b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_non_nullable_type.php.inc new file mode 100644 index 00000000..7f0ad0f1 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Fixture/skip_non_nullable_type.php.inc @@ -0,0 +1,24 @@ +getSomeObject(); + $value = $someObject->getSomeMethod(); + + $this->assertSame(123, $value); + } + + private function getSomeObject(): SomeClassUsedInTests + { + return new SomeClassUsedInTests(); + } +} diff --git a/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Source/SomeClassUsedInTests.php b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Source/SomeClassUsedInTests.php new file mode 100644 index 00000000..0c1bd934 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector/Source/SomeClassUsedInTests.php @@ -0,0 +1,13 @@ +rule(AddInstanceofAssertForNullableInstanceRector::class); +}; diff --git a/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/skip_sole_type.php.inc b/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/skip_sole_type.php.inc index a67e6d31..2f76011e 100644 --- a/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/skip_sole_type.php.inc +++ b/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/skip_sole_type.php.inc @@ -6,7 +6,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\SingleMockPropertyTypeRector\Source\AnyBook; -class SkipSoleType extends TestCase +final class SkipSoleType extends TestCase { private MockObject $anyBook; } diff --git a/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/union_type_mock.php.inc b/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/union_type_mock.php.inc index 31c61ad5..60287bf5 100644 --- a/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/union_type_mock.php.inc +++ b/rules-tests/CodeQuality/Rector/Class_/SingleMockPropertyTypeRector/Fixture/union_type_mock.php.inc @@ -6,7 +6,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\SingleMockPropertyTypeRector\Source\AnyBook; -class UnionTypeMock extends TestCase +final class UnionTypeMock extends TestCase { private MockObject|AnyBook $anyBook; } @@ -21,7 +21,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Rector\PHPUnit\Tests\CodeQuality\Rector\Class_\SingleMockPropertyTypeRector\Source\AnyBook; -class UnionTypeMock extends TestCase +final class UnionTypeMock extends TestCase { private MockObject $anyBook; } diff --git a/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php b/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php new file mode 100644 index 00000000..befbd9ac --- /dev/null +++ b/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php @@ -0,0 +1,270 @@ +getSomeObject(); + + $value = $someObject->getSomeMethod(); + } + + private function getSomeObject(): ?SomeClass + { + if (mt_rand(0, 1)) { + return new SomeClass(); + } + + return null; + } +} +CODE_SAMPLE + + , + <<<'CODE_SAMPLE' +use PHPUnit\Framework\TestCase; + +final class SomeTest extends TestCase +{ + public function test() + { + $someObject = $this->getSomeObject(); + $this->assertInstanceof(SomeClass::class, $someObject); + + $value = $someObject->getSomeMethod(); + } + + private function getSomeObject(): ?SomeClass + { + if (mt_rand(0, 1)) { + return new SomeClass(); + } + + return null; + } +} +CODE_SAMPLE + ), + ] + ); + } + + /** + * @return array> + */ + public function getNodeTypes(): array + { + return [ClassMethod::class, Foreach_::class]; + } + + /** + * @param ClassMethod|Foreach_ $node + */ + public function refactor(Node $node): ?Node + { + if (! $this->testsNodeAnalyzer->isInTestClass($node)) { + return null; + } + + if ($node->stmts === [] || $node->stmts === null) { + return null; + } + + $hasChanged = false; + + /** @var VariableNameToType[] $nullableVariableNamesToTypes */ + $nullableVariableNamesToTypes = []; + + foreach ($node->stmts as $key => $stmt) { + if ($stmt instanceof Expression && $stmt->expr instanceof Assign) { + $variableNameToType = $this->collectVariableFromAssign($stmt->expr); + if ($variableNameToType instanceof VariableNameToType) { + $nullableVariableNamesToTypes[] = $variableNameToType; + continue; + } + } + + // has callable on nullable variable of already collected name? + $matchedNullableVariableNameToType = $this->matchedNullableVariableNameToType( + $stmt, + $nullableVariableNamesToTypes + ); + if (! $matchedNullableVariableNameToType instanceof VariableNameToType) { + continue; + } + + // adding type here + popping the variable name out + $assertInstanceofExpression = $this->createAssertInstanceof($matchedNullableVariableNameToType); + array_splice($node->stmts, $key, 0, [$assertInstanceofExpression]); + + // remove variable name from nullable ones + $hasChanged = true; + + // from now on, the variable is not nullable, remove to avoid double asserts + foreach ($nullableVariableNamesToTypes as $key => $nullableVariableNamesToType) { + if ($matchedNullableVariableNameToType === $nullableVariableNamesToType) { + unset($nullableVariableNamesToTypes[$key]); + } + } + } + + if (! $hasChanged) { + return null; + } + + return $node; + } + + private function isNullableType(Type $type): bool + { + if (! $type instanceof UnionType) { + return false; + } + + if (! TypeCombinator::containsNull($type)) { + return false; + } + + return count($type->getTypes()) === 2; + } + + private function collectVariableFromAssign(Assign $assign): ?VariableNameToType + { + if (! $assign->expr instanceof MethodCall) { + return null; + } + + if (! $assign->var instanceof Variable) { + return null; + } + + $variableType = $this->getType($assign); + if (! $this->isNullableType($variableType)) { + return null; + } + + // $fullType = TypeCombinator::removeNull($variableType); + $variableName = $this->getName($assign->var); + + $bareVariableType = TypeCombinator::removeNull($variableType); + if (! $bareVariableType instanceof ObjectType) { + return null; + } + + return new VariableNameToType($variableName, $bareVariableType->getClassName()); + } + + private function createAssertInstanceof(VariableNameToType $variableNameToType): Expression + { + $args = [ + new Arg(new ClassConstFetch(new FullyQualified($variableNameToType->getObjectType()), 'class')), + new Arg(new Variable($variableNameToType->getVariableName())), + ]; + + $methodCall = new MethodCall(new Variable('this'), 'assertInstanceof', $args); + + return new Expression($methodCall); + } + + /** + * @param VariableNameToType[] $nullableVariableNamesToTypes + */ + private function matchNullableVariableNameToType( + array $nullableVariableNamesToTypes, + Variable $variable + ): ?VariableNameToType { + $variableName = $this->getName($variable); + + foreach ($nullableVariableNamesToTypes as $nullableVariableNameToType) { + if ($nullableVariableNameToType->getVariableName() !== $variableName) { + continue; + } + + return $nullableVariableNameToType; + } + + return null; + } + + /** + * @param VariableNameToType[] $variableNamesToTypes + */ + private function matchedNullableVariableNameToType( + Stmt $stmt, + array $variableNamesToTypes + ): ?VariableNameToType { + $matchedNullableVariableNameToType = null; + + $this->traverseNodesWithCallable($stmt, function (Node $node) use ( + $variableNamesToTypes, + &$matchedNullableVariableNameToType + ): null { + if (! $node instanceof MethodCall) { + return null; + } + + if (! $node->var instanceof Variable) { + return null; + } + + $variableType = $this->getType($node->var); + if (! $this->isNullableType($variableType)) { + return null; + } + + // is the variable we're interested in? + $matchedNullableVariableNameToType = $this->matchNullableVariableNameToType( + $variableNamesToTypes, + $node->var + ); + + return null; + }); + + return $matchedNullableVariableNameToType; + } +} diff --git a/rules/CodeQuality/ValueObject/VariableNameToType.php b/rules/CodeQuality/ValueObject/VariableNameToType.php new file mode 100644 index 00000000..33582056 --- /dev/null +++ b/rules/CodeQuality/ValueObject/VariableNameToType.php @@ -0,0 +1,24 @@ +variableName; + } + + public function getObjectType(): string + { + return $this->objectType; + } +} From 013a7253742f46261c4bfdc11619c0f4e31f3933 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 11 Mar 2025 12:32:07 +0100 Subject: [PATCH 2/5] update config sort packages --- composer.json | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/composer.json b/composer.json index 3ef2b741..6587a799 100644 --- a/composer.json +++ b/composer.json @@ -48,17 +48,15 @@ "rector": "vendor/bin/rector process --ansi" }, "extra": { - "enable-patching": true, - "branch-alias": { - "dev-main": "0.11-dev" - } + "enable-patching": true }, "conflict": { - "rector/rector": "<0.11" + "rector/rector": "<2.0" }, "minimum-stability": "dev", "prefer-stable": true, "config": { + "sort-packages": true, "allow-plugins": { "cweagans/composer-patches": true, "rector/extension-installer": true, From bd169df3173706529be5b1039a0a9c82956cdc88 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 11 Mar 2025 12:32:37 +0100 Subject: [PATCH 3/5] register rule to code quality set --- config/sets/phpunit-code-quality.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/sets/phpunit-code-quality.php b/config/sets/phpunit-code-quality.php index 3eea0839..b1c9f15b 100644 --- a/config/sets/phpunit-code-quality.php +++ b/config/sets/phpunit-code-quality.php @@ -9,6 +9,7 @@ use Rector\PHPUnit\CodeQuality\Rector\Class_\SingleMockPropertyTypeRector; use Rector\PHPUnit\CodeQuality\Rector\Class_\TestWithToDataProviderRector; use Rector\PHPUnit\CodeQuality\Rector\Class_\YieldDataProviderRector; +use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\AddInstanceofAssertForNullableInstanceRector; use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\DataProviderArrayItemsNewLinedRector; use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\RemoveEmptyTestMethodRector; use Rector\PHPUnit\CodeQuality\Rector\ClassMethod\ReplaceTestAnnotationWithPrefixedFunctionRector; @@ -75,6 +76,9 @@ AssertEmptyNullableObjectToAssertInstanceofRector::class, AssertCountWithZeroToAssertEmptyRector::class, + // avoid call on nullable object + AddInstanceofAssertForNullableInstanceRector::class, + /** * Improve direct testing of your code, without mock creep. Make it simple, clear and easy to maintain: * From bea1d39f47f6fcf2cccc989901812ca072665cf6 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 11 Mar 2025 12:36:39 +0100 Subject: [PATCH 4/5] cleanup --- .../NullableObjectAssignCollector.php | 44 +++++++++++++++++++ ...tanceofAssertForNullableInstanceRector.php | 31 ++----------- .../VariableNameToTypeCollection.php | 16 +++++++ 3 files changed, 63 insertions(+), 28 deletions(-) create mode 100644 rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php create mode 100644 rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php diff --git a/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php b/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php new file mode 100644 index 00000000..6bec307b --- /dev/null +++ b/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php @@ -0,0 +1,44 @@ +expr instanceof MethodCall) { + return null; + } + + if (! $assign->var instanceof Variable) { + return null; + } + + $variableName = $this->nodeNameResolver->getName($assign->var); + $variableType = $this->nodeTypeResolver->getType($assign); + + $bareVariableType = TypeCombinator::removeNull($variableType); + if (! $bareVariableType instanceof ObjectType) { + return null; + } + + return new VariableNameToType($variableName, $bareVariableType->getClassName()); + } +} diff --git a/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php b/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php index befbd9ac..0ea02719 100644 --- a/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php +++ b/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php @@ -15,10 +15,10 @@ use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\Foreach_; -use PHPStan\Type\ObjectType; use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; use PHPStan\Type\UnionType; +use Rector\PHPUnit\CodeQuality\NodeAnalyser\NullableObjectAssignCollector; use Rector\PHPUnit\CodeQuality\ValueObject\VariableNameToType; use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; use Rector\Rector\AbstractRector; @@ -32,6 +32,7 @@ final class AddInstanceofAssertForNullableInstanceRector extends AbstractRector { public function __construct( private readonly TestsNodeAnalyzer $testsNodeAnalyzer, + private readonly NullableObjectAssignCollector $nullableObjectAssignCollector, ) { } @@ -121,7 +122,7 @@ public function refactor(Node $node): ?Node foreach ($node->stmts as $key => $stmt) { if ($stmt instanceof Expression && $stmt->expr instanceof Assign) { - $variableNameToType = $this->collectVariableFromAssign($stmt->expr); + $variableNameToType = $this->nullableObjectAssignCollector->collect($stmt->expr); if ($variableNameToType instanceof VariableNameToType) { $nullableVariableNamesToTypes[] = $variableNameToType; continue; @@ -172,32 +173,6 @@ private function isNullableType(Type $type): bool return count($type->getTypes()) === 2; } - private function collectVariableFromAssign(Assign $assign): ?VariableNameToType - { - if (! $assign->expr instanceof MethodCall) { - return null; - } - - if (! $assign->var instanceof Variable) { - return null; - } - - $variableType = $this->getType($assign); - if (! $this->isNullableType($variableType)) { - return null; - } - - // $fullType = TypeCombinator::removeNull($variableType); - $variableName = $this->getName($assign->var); - - $bareVariableType = TypeCombinator::removeNull($variableType); - if (! $bareVariableType instanceof ObjectType) { - return null; - } - - return new VariableNameToType($variableName, $bareVariableType->getClassName()); - } - private function createAssertInstanceof(VariableNameToType $variableNameToType): Expression { $args = [ diff --git a/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php b/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php new file mode 100644 index 00000000..155712cb --- /dev/null +++ b/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php @@ -0,0 +1,16 @@ + Date: Tue, 11 Mar 2025 12:41:27 +0100 Subject: [PATCH 5/5] add collection --- .../NullableObjectAssignCollector.php | 44 ++++++++++++-- ...tanceofAssertForNullableInstanceRector.php | 58 ++++--------------- .../VariableNameToTypeCollection.php | 25 ++++++++ 3 files changed, 74 insertions(+), 53 deletions(-) diff --git a/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php b/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php index 6bec307b..681220e6 100644 --- a/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php +++ b/rules/CodeQuality/NodeAnalyser/NullableObjectAssignCollector.php @@ -7,21 +7,55 @@ use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\Variable; +use PhpParser\Node\Stmt\ClassMethod; +use PhpParser\Node\Stmt\Expression; +use PhpParser\Node\Stmt\Foreach_; use PHPStan\Type\ObjectType; use PHPStan\Type\TypeCombinator; use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\NodeTypeResolver; use Rector\PHPUnit\CodeQuality\ValueObject\VariableNameToType; +use Rector\PHPUnit\CodeQuality\ValueObject\VariableNameToTypeCollection; -final class NullableObjectAssignCollector +/** + * We look for object|null type on the left: + * + * $value = $this->getSomething(); + */ +final readonly class NullableObjectAssignCollector { public function __construct( - private readonly NodeNameResolver $nodeNameResolver, - private readonly NodeTypeResolver $nodeTypeResolver, + private NodeNameResolver $nodeNameResolver, + private NodeTypeResolver $nodeTypeResolver, ) { } - public function collect(Assign $assign): ?VariableNameToType + public function collect(ClassMethod|Foreach_ $stmtsAware): VariableNameToTypeCollection + { + $variableNamesToType = []; + + // first round to collect assigns + foreach ((array) $stmtsAware->stmts as $stmt) { + if (! $stmt instanceof Expression) { + continue; + } + + if (! $stmt->expr instanceof Assign) { + continue; + } + + $variableNameToType = $this->collectFromAssign($stmt->expr); + if (! $variableNameToType instanceof VariableNameToType) { + continue; + } + + $variableNamesToType[] = $variableNameToType; + } + + return new VariableNameToTypeCollection($variableNamesToType); + } + + private function collectFromAssign(Assign $assign): ?VariableNameToType { if (! $assign->expr instanceof MethodCall) { return null; @@ -31,7 +65,6 @@ public function collect(Assign $assign): ?VariableNameToType return null; } - $variableName = $this->nodeNameResolver->getName($assign->var); $variableType = $this->nodeTypeResolver->getType($assign); $bareVariableType = TypeCombinator::removeNull($variableType); @@ -39,6 +72,7 @@ public function collect(Assign $assign): ?VariableNameToType return null; } + $variableName = $this->nodeNameResolver->getName($assign->var); return new VariableNameToType($variableName, $bareVariableType->getClassName()); } } diff --git a/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php b/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php index 0ea02719..f4c0bb13 100644 --- a/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php +++ b/rules/CodeQuality/Rector/ClassMethod/AddInstanceofAssertForNullableInstanceRector.php @@ -6,7 +6,6 @@ use PhpParser\Node; use PhpParser\Node\Arg; -use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\ClassConstFetch; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\Variable; @@ -20,6 +19,7 @@ use PHPStan\Type\UnionType; use Rector\PHPUnit\CodeQuality\NodeAnalyser\NullableObjectAssignCollector; use Rector\PHPUnit\CodeQuality\ValueObject\VariableNameToType; +use Rector\PHPUnit\CodeQuality\ValueObject\VariableNameToTypeCollection; use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; use Rector\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; @@ -111,28 +111,18 @@ public function refactor(Node $node): ?Node return null; } - if ($node->stmts === [] || $node->stmts === null) { + if ($node->stmts === [] || $node->stmts === null || count($node->stmts) < 2) { return null; } $hasChanged = false; - - /** @var VariableNameToType[] $nullableVariableNamesToTypes */ - $nullableVariableNamesToTypes = []; + $variableNameToTypeCollection = $this->nullableObjectAssignCollector->collect($node); foreach ($node->stmts as $key => $stmt) { - if ($stmt instanceof Expression && $stmt->expr instanceof Assign) { - $variableNameToType = $this->nullableObjectAssignCollector->collect($stmt->expr); - if ($variableNameToType instanceof VariableNameToType) { - $nullableVariableNamesToTypes[] = $variableNameToType; - continue; - } - } - // has callable on nullable variable of already collected name? $matchedNullableVariableNameToType = $this->matchedNullableVariableNameToType( $stmt, - $nullableVariableNamesToTypes + $variableNameToTypeCollection ); if (! $matchedNullableVariableNameToType instanceof VariableNameToType) { continue; @@ -146,11 +136,7 @@ public function refactor(Node $node): ?Node $hasChanged = true; // from now on, the variable is not nullable, remove to avoid double asserts - foreach ($nullableVariableNamesToTypes as $key => $nullableVariableNamesToType) { - if ($matchedNullableVariableNameToType === $nullableVariableNamesToType) { - unset($nullableVariableNamesToTypes[$key]); - } - } + $variableNameToTypeCollection->remove($matchedNullableVariableNameToType); } if (! $hasChanged) { @@ -185,37 +171,14 @@ private function createAssertInstanceof(VariableNameToType $variableNameToType): return new Expression($methodCall); } - /** - * @param VariableNameToType[] $nullableVariableNamesToTypes - */ - private function matchNullableVariableNameToType( - array $nullableVariableNamesToTypes, - Variable $variable - ): ?VariableNameToType { - $variableName = $this->getName($variable); - - foreach ($nullableVariableNamesToTypes as $nullableVariableNameToType) { - if ($nullableVariableNameToType->getVariableName() !== $variableName) { - continue; - } - - return $nullableVariableNameToType; - } - - return null; - } - - /** - * @param VariableNameToType[] $variableNamesToTypes - */ private function matchedNullableVariableNameToType( Stmt $stmt, - array $variableNamesToTypes + VariableNameToTypeCollection $variableNameToTypeCollection ): ?VariableNameToType { $matchedNullableVariableNameToType = null; $this->traverseNodesWithCallable($stmt, function (Node $node) use ( - $variableNamesToTypes, + $variableNameToTypeCollection, &$matchedNullableVariableNameToType ): null { if (! $node instanceof MethodCall) { @@ -231,12 +194,11 @@ private function matchedNullableVariableNameToType( return null; } - // is the variable we're interested in? - $matchedNullableVariableNameToType = $this->matchNullableVariableNameToType( - $variableNamesToTypes, - $node->var + $matchedNullableVariableNameToType = $variableNameToTypeCollection->matchByVariableName( + $this->getName($node->var) ); + // is the variable we're interested in? return null; }); diff --git a/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php b/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php index 155712cb..baf6da60 100644 --- a/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php +++ b/rules/CodeQuality/ValueObject/VariableNameToTypeCollection.php @@ -13,4 +13,29 @@ public function __construct( private array $variableNameToType ) { } + + public function matchByVariableName(string $variableName): ?VariableNameToType + { + foreach ($this->variableNameToType as $variableNameToType) { + if ($variableNameToType->getVariableName() !== $variableName) { + continue; + } + + return $variableNameToType; + } + + return null; + } + + public function remove(VariableNameToType $matchedNullableVariableNameToType): void + { + foreach ($this->variableNameToType as $key => $variableNamesToType) { + if ($matchedNullableVariableNameToType !== $variableNamesToType) { + continue; + } + + unset($this->variableNameToType[$key]); + break; + } + } }