From 3ee5b725863d6e5a6e6ab51f13d1c9c7bbffc939 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Wed, 13 Nov 2024 16:40:46 +0100 Subject: [PATCH] [TypeDeclaration] Skip test methods with exception in ReturnNeverTypeRector, [php74] Skip conditinal assign in RestoreDefaultNullToNullableTypePropertyRector as most likely desired to assign or fail (#6430) * [TypeDeclaration] Skip test methods with exception in ReturnNeverTypeRector * [php74] Skip conditinal assign in RestoreDefaultNullToNullableTypePropertyRector as most likely desired to assign or fail --- .../skip_assigned_in_construct.php.inc | 2 +- .../Fixture/skip_checked.php.inc | 19 +++++++++ .../Fixture/skip_test_method.php.inc | 16 +++++++ ...efaultNullToNullableTypePropertyRector.php | 9 ++-- .../Rector/FunctionLike/MixedTypeRector.php | 5 +-- .../ConstructorAssignDetector.php | 42 ++++++++----------- .../ClassMethod/ReturnNeverTypeRector.php | 26 +++++++++++- .../StatementDepthAttributeDecorator.php | 28 +++++++++++++ src/NodeTypeResolver/Node/AttributeKey.php | 5 +++ 9 files changed, 116 insertions(+), 36 deletions(-) create mode 100644 rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_checked.php.inc create mode 100644 rules-tests/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector/Fixture/skip_test_method.php.inc create mode 100644 src/NodeDecorator/StatementDepthAttributeDecorator.php diff --git a/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_assigned_in_construct.php.inc b/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_assigned_in_construct.php.inc index a319ee3d995..c5feb1994ce 100644 --- a/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_assigned_in_construct.php.inc +++ b/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_assigned_in_construct.php.inc @@ -2,7 +2,7 @@ namespace Rector\Tests\Php74\Rector\Property\RestoreDefaultNullToNullableTypePropertyRector\Fixture; -class SkipAssignedInConstruct +final class SkipAssignedInConstruct { public ?string $name; diff --git a/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_checked.php.inc b/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_checked.php.inc new file mode 100644 index 00000000000..388959e976e --- /dev/null +++ b/rules-tests/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector/Fixture/skip_checked.php.inc @@ -0,0 +1,19 @@ +name = $item; + } + + if (! isset($this->item)) { + throw new \InvalidArgumentException(); + } + } +} diff --git a/rules-tests/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector/Fixture/skip_test_method.php.inc b/rules-tests/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector/Fixture/skip_test_method.php.inc new file mode 100644 index 00000000000..3ef0281fc02 --- /dev/null +++ b/rules-tests/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector/Fixture/skip_test_method.php.inc @@ -0,0 +1,16 @@ +expectException(Exception::class); + + throw new \InvalidArgumentException(); + } +} diff --git a/rules/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector.php b/rules/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector.php index aca18cfa533..1a91f990919 100644 --- a/rules/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector.php +++ b/rules/Php74/Rector/Property/RestoreDefaultNullToNullableTypePropertyRector.php @@ -71,7 +71,7 @@ public function refactor(Node $node): ?Node $hasChanged = false; foreach ($node->getProperties() as $property) { - if ($this->shouldSkip($property, $node)) { + if ($this->shouldSkipProperty($property, $node)) { continue; } @@ -93,7 +93,7 @@ public function provideMinPhpVersion(): int return PhpVersionFeature::TYPED_PROPERTIES; } - private function shouldSkip(Property $property, Class_ $class): bool + private function shouldSkipProperty(Property $property, Class_ $class): bool { if ($property->type === null) { return true; @@ -103,8 +103,7 @@ private function shouldSkip(Property $property, Class_ $class): bool return true; } - $onlyProperty = $property->props[0]; - if ($onlyProperty->default instanceof Expr) { + if ($property->props[0]->default instanceof Expr) { return true; } @@ -118,7 +117,7 @@ private function shouldSkip(Property $property, Class_ $class): bool // is variable assigned in constructor $propertyName = $this->getName($property); - return $this->constructorAssignDetector->isPropertyAssigned($class, $propertyName); + return $this->constructorAssignDetector->isPropertyAssignedConditionally($class, $propertyName); } private function isReadonlyProperty(Property $property): bool diff --git a/rules/Php80/Rector/FunctionLike/MixedTypeRector.php b/rules/Php80/Rector/FunctionLike/MixedTypeRector.php index 95cb308bd9d..bbe5c887dad 100644 --- a/rules/Php80/Rector/FunctionLike/MixedTypeRector.php +++ b/rules/Php80/Rector/FunctionLike/MixedTypeRector.php @@ -94,11 +94,8 @@ public function refactor(Node $node): ?Node $this->refactorParamTypes($node, $phpDocInfo); $hasChanged = $this->paramTagRemover->removeParamTagsIfUseless($phpDocInfo, $node, new MixedType()); - if ($this->hasChanged) { - return $node; - } - if ($hasChanged) { + if ($this->hasChanged || $hasChanged) { return $node; } diff --git a/rules/TypeDeclaration/AlreadyAssignDetector/ConstructorAssignDetector.php b/rules/TypeDeclaration/AlreadyAssignDetector/ConstructorAssignDetector.php index 13862a4d9d6..f518d4bcca3 100644 --- a/rules/TypeDeclaration/AlreadyAssignDetector/ConstructorAssignDetector.php +++ b/rules/TypeDeclaration/AlreadyAssignDetector/ConstructorAssignDetector.php @@ -10,6 +10,7 @@ use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Identifier; use PhpParser\Node\Stmt; +use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Else_; @@ -19,6 +20,8 @@ use PhpParser\NodeTraverser; use PHPStan\Type\ObjectType; use Rector\NodeAnalyzer\PropertyFetchAnalyzer; +use Rector\NodeDecorator\StatementDepthAttributeDecorator; +use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\NodeTypeResolver\NodeTypeResolver; use Rector\PhpDocParser\NodeTraverser\SimpleCallableNodeTraverser; use Rector\PhpParser\Comparing\NodeComparator; @@ -28,11 +31,6 @@ final readonly class ConstructorAssignDetector { - /** - * @var string - */ - private const IS_FIRST_LEVEL_STATEMENT = 'first_level_stmt'; - public function __construct( private NodeTypeResolver $nodeTypeResolver, private PropertyAssignMatcher $propertyAssignMatcher, @@ -43,7 +41,12 @@ public function __construct( ) { } - public function isPropertyAssigned(ClassLike $classLike, string $propertyName): bool + public function isPropertyAssignedConditionally(Class_ $class, string $propertyName): bool + { + return $this->isPropertyAssigned($class, $propertyName, true); + } + + public function isPropertyAssigned(ClassLike $classLike, string $propertyName, bool $allowConditional = false): bool { $initializeClassMethods = $this->matchInitializeClassMethod($classLike); if ($initializeClassMethods === []) { @@ -52,12 +55,12 @@ public function isPropertyAssigned(ClassLike $classLike, string $propertyName): $isAssignedInConstructor = false; - $this->decorateFirstLevelStatementAttribute($initializeClassMethods); + StatementDepthAttributeDecorator::decorateClassMethods($initializeClassMethods); foreach ($initializeClassMethods as $initializeClassMethod) { $this->simpleCallableNodeTraverser->traverseNodesWithCallable((array) $initializeClassMethod->stmts, function ( Node $node - ) use ($propertyName, &$isAssignedInConstructor): ?int { + ) use ($propertyName, &$isAssignedInConstructor, $allowConditional): ?int { if ($this->isIfElseAssign($node, $propertyName)) { $isAssignedInConstructor = true; return NodeTraverser::STOP_TRAVERSAL; @@ -77,10 +80,15 @@ public function isPropertyAssigned(ClassLike $classLike, string $propertyName): return NodeTraverser::STOP_TRAVERSAL; } - $isFirstLevelStatement = $assign->getAttribute(self::IS_FIRST_LEVEL_STATEMENT); + $isFirstLevelStatement = $assign->getAttribute(AttributeKey::IS_FIRST_LEVEL_STATEMENT); // cannot be nested if ($isFirstLevelStatement !== true) { + if ($allowConditional) { + $isAssignedInConstructor = true; + return NodeTraverser::STOP_TRAVERSAL; + } + return null; } @@ -139,22 +147,6 @@ private function matchAssignExprToPropertyName(Node $node, string $propertyName) return $this->propertyAssignMatcher->matchPropertyAssignExpr($node, $propertyName); } - /** - * @param ClassMethod[] $classMethods - */ - private function decorateFirstLevelStatementAttribute(array $classMethods): void - { - foreach ($classMethods as $classMethod) { - foreach ((array) $classMethod->stmts as $methodStmt) { - $methodStmt->setAttribute(self::IS_FIRST_LEVEL_STATEMENT, true); - - if ($methodStmt instanceof Expression) { - $methodStmt->expr->setAttribute(self::IS_FIRST_LEVEL_STATEMENT, true); - } - } - } - } - /** * @return ClassMethod[] */ diff --git a/rules/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector.php b/rules/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector.php index 5208c7e5fd3..676cfa4ef88 100644 --- a/rules/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector.php +++ b/rules/TypeDeclaration/Rector/ClassMethod/ReturnNeverTypeRector.php @@ -8,6 +8,7 @@ use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Function_; use Rector\PHPStan\ScopeFetcher; +use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; use Rector\Rector\AbstractRector; use Rector\TypeDeclaration\NodeManipulator\AddNeverReturnType; use Rector\ValueObject\PhpVersionFeature; @@ -21,7 +22,8 @@ final class ReturnNeverTypeRector extends AbstractRector implements MinPhpVersionInterface { public function __construct( - private readonly AddNeverReturnType $addNeverReturnType + private readonly AddNeverReturnType $addNeverReturnType, + private readonly TestsNodeAnalyzer $testsNodeAnalyzer ) { } @@ -67,6 +69,11 @@ public function getNodeTypes(): array public function refactor(Node $node): ?Node { $scope = ScopeFetcher::fetch($node); + + if ($this->isTestClassMethodWithFilledReturnType($node)) { + return null; + } + return $this->addNeverReturnType->add($node, $scope); } @@ -74,4 +81,21 @@ public function provideMinPhpVersion(): int { return PhpVersionFeature::NEVER_TYPE; } + + private function isTestClassMethodWithFilledReturnType(ClassMethod|Function_ $callLike): bool + { + if (! $callLike instanceof ClassMethod) { + return false; + } + + if (! $callLike->isPublic()) { + return false; + } + + if (! $this->testsNodeAnalyzer->isInTestClass($callLike)) { + return false; + } + + return $callLike->returnType instanceof Node; + } } diff --git a/src/NodeDecorator/StatementDepthAttributeDecorator.php b/src/NodeDecorator/StatementDepthAttributeDecorator.php new file mode 100644 index 00000000000..d26a8e28a25 --- /dev/null +++ b/src/NodeDecorator/StatementDepthAttributeDecorator.php @@ -0,0 +1,28 @@ +stmts as $methodStmt) { + $methodStmt->setAttribute(AttributeKey::IS_FIRST_LEVEL_STATEMENT, true); + + if ($methodStmt instanceof Expression) { + $methodStmt->expr->setAttribute(AttributeKey::IS_FIRST_LEVEL_STATEMENT, true); + } + } + } + } +} diff --git a/src/NodeTypeResolver/Node/AttributeKey.php b/src/NodeTypeResolver/Node/AttributeKey.php index 3305711dcd7..099684f6b96 100644 --- a/src/NodeTypeResolver/Node/AttributeKey.php +++ b/src/NodeTypeResolver/Node/AttributeKey.php @@ -285,4 +285,9 @@ final class AttributeKey * @var string */ public const ATTRIBUTE_COMMENT = 'attribute_comment'; + + /** + * @var string + */ + public const IS_FIRST_LEVEL_STATEMENT = 'first_level_stmt'; }