diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_after_throwing_branch.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_after_throwing_branch.php.inc new file mode 100644 index 00000000000..936a4cef48a --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_after_throwing_branch.php.inc @@ -0,0 +1,43 @@ +name = $name; + } +} + +?> +----- +name = $name; + } +} + +?> diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_in_if_elseif_else.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_in_if_elseif_else.php.inc new file mode 100644 index 00000000000..4fe52fd6a18 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_in_if_elseif_else.php.inc @@ -0,0 +1,43 @@ +name = 'one'; + } elseif ($type === 2) { + $this->name = 'two'; + } else { + $this->name = 'many'; + } + } +} + +?> +----- +name = 'one'; + } elseif ($type === 2) { + $this->name = 'two'; + } else { + $this->name = 'many'; + } + } +} + +?> diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_in_switch_with_default.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_in_switch_with_default.php.inc new file mode 100644 index 00000000000..53441210a9b --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/assign_in_switch_with_default.php.inc @@ -0,0 +1,43 @@ +name = 'one'; + break; + default: + $this->name = 'many'; + } + } +} + +?> +----- +name = 'one'; + break; + default: + $this->name = 'many'; + } + } +} + +?> diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/return_after_assign.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/return_after_assign.php.inc new file mode 100644 index 00000000000..31df9613088 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/return_after_assign.php.inc @@ -0,0 +1,43 @@ +name = $name; + + if ($shouldReturn) { + return; + } + + $this->name = strtoupper($name); + } +} + +?> +----- +name = $name; + + if ($shouldReturn) { + return; + } + + $this->name = strtoupper($name); + } +} + +?> diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_in_foreach.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_in_foreach.php.inc new file mode 100644 index 00000000000..f9109c26264 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_in_foreach.php.inc @@ -0,0 +1,18 @@ +name = $name; + } + } +} diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_inside_closure.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_inside_closure.php.inc new file mode 100644 index 00000000000..f92434bbc0a --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_inside_closure.php.inc @@ -0,0 +1,19 @@ +setName = function () use ($name): void { + $this->name = $name; + }; + } +} diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_only_in_try.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_only_in_try.php.inc new file mode 100644 index 00000000000..abe68815c82 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_only_in_try.php.inc @@ -0,0 +1,18 @@ +name = json_encode($name, JSON_THROW_ON_ERROR); + } catch (Throwable) { + } + } +} diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_reads_default.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_reads_default.php.inc new file mode 100644 index 00000000000..341a0733b59 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_reads_default.php.inc @@ -0,0 +1,13 @@ +count = $this->count + $extra; + } +} diff --git a/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_via_offset_access.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_via_offset_access.php.inc new file mode 100644 index 00000000000..30569d913e7 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector/Fixture/skip_assign_via_offset_access.php.inc @@ -0,0 +1,16 @@ + + */ + private array $cachedByName = []; + + public function __construct(string $name) + { + $this->cachedByName[$name] = true; + } +} diff --git a/rules/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector.php b/rules/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector.php index 6dd4b633d5a..e4fc84c0e5c 100644 --- a/rules/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector.php +++ b/rules/DeadCode/Rector/Property/RemoveDefaultValueFromAssignedPropertyRector.php @@ -6,9 +6,12 @@ use PhpParser\Node; use PhpParser\Node\Expr; +use PhpParser\Node\Expr\ArrayDimFetch; +use PhpParser\Node\Expr\Assign; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassMethod; -use PhpParser\Node\Stmt\Return_; +use Rector\NodeAnalyzer\AlwaysInitializedPropertyAnalyzer; +use Rector\NodeAnalyzer\PropertyFetchAnalyzer; use Rector\PhpParser\Node\BetterNodeFinder; use Rector\Rector\AbstractRector; use Rector\TypeDeclaration\AlreadyAssignDetector\ConstructorAssignDetector; @@ -23,7 +26,9 @@ final class RemoveDefaultValueFromAssignedPropertyRector extends AbstractRector { public function __construct( private readonly ConstructorAssignDetector $constructorAssignDetector, - private readonly BetterNodeFinder $betterNodeFinder + private readonly BetterNodeFinder $betterNodeFinder, + private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer, + private readonly AlwaysInitializedPropertyAnalyzer $alwaysInitializedPropertyAnalyzer ) { } @@ -79,11 +84,6 @@ public function refactor(Node $node): ?Node return null; } - // early return can skip the assign, so the default value is still needed - if ($this->betterNodeFinder->hasInstancesOfInFunctionLikeScoped($constructClassMethod, Return_::class)) { - return null; - } - $hasChanged = false; foreach ($node->getProperties() as $property) { @@ -102,7 +102,23 @@ public function refactor(Node $node): ?Node } $propertyName = $this->getName($propertyProperty); - if (! $this->constructorAssignDetector->isPropertyAssigned($node, $propertyName)) { + + // guards against an assign that reads the default value first, e.g. $this->value = $this->value + 1 + if (! $this->constructorAssignDetector->isPropertyAssignedConditionally($node, $propertyName)) { + continue; + } + + // the property must be initialized on every path out of the constructor, + // otherwise removing the default value leaves it uninitialized + if (! $this->alwaysInitializedPropertyAnalyzer->isAlwaysInitialized( + $constructClassMethod, + $propertyName + )) { + continue; + } + + // $this->value['key'] = ... writes to the default value, it does not replace it + if ($this->isAssignedViaOffsetAccess($constructClassMethod, $propertyName)) { continue; } @@ -117,4 +133,29 @@ public function refactor(Node $node): ?Node return null; } + + private function isAssignedViaOffsetAccess(ClassMethod $constructClassMethod, string $propertyName): bool + { + $assign = $this->betterNodeFinder->findFirstInFunctionLikeScoped( + $constructClassMethod, + function (Node $node) use ($propertyName): bool { + if (! $node instanceof Assign) { + return false; + } + + $assignVar = $node->var; + if (! $assignVar instanceof ArrayDimFetch) { + return false; + } + + while ($assignVar instanceof ArrayDimFetch) { + $assignVar = $assignVar->var; + } + + return $this->propertyFetchAnalyzer->isLocalPropertyFetchName($assignVar, $propertyName); + } + ); + + return $assign instanceof Assign; + } } diff --git a/src/NodeAnalyzer/AlwaysInitializedPropertyAnalyzer.php b/src/NodeAnalyzer/AlwaysInitializedPropertyAnalyzer.php new file mode 100644 index 00000000000..e3d8d06e197 --- /dev/null +++ b/src/NodeAnalyzer/AlwaysInitializedPropertyAnalyzer.php @@ -0,0 +1,73 @@ +resolveExecutionEndScope($classMethod); + if (! $executionEndScope instanceof Scope) { + return false; + } + + return $executionEndScope->hasExpressionType(new PropertyInitializationExpr($propertyName)) + ->yes(); + } + + /** + * Scope merged from every execution end and return statement, so it describes + * what is certain once the class method has finished, no matter which path was taken + */ + private function resolveExecutionEndScope(ClassMethod $classMethod): ?MutatingScope + { + $methodReturnStatementsNode = $classMethod->getAttribute(AttributeKey::METHOD_RETURN_STATEMENTS_NODE); + if (! $methodReturnStatementsNode instanceof MethodReturnStatementsNode) { + return null; + } + + $executionEndScope = null; + + foreach ($methodReturnStatementsNode->getExecutionEnds() as $executionEndNode) { + $executionEndScope = $this->mergeScopes( + $executionEndScope, + $executionEndNode->getStatementResult() + ->getScope() + ); + } + + foreach ($methodReturnStatementsNode->getReturnStatements() as $returnStatement) { + $executionEndScope = $this->mergeScopes($executionEndScope, $returnStatement->getScope()); + } + + return $executionEndScope; + } + + private function mergeScopes(?MutatingScope $mutatingScope, Scope $scope): ?MutatingScope + { + if (! $scope instanceof MutatingScope) { + return $mutatingScope; + } + + if (! $mutatingScope instanceof MutatingScope) { + return $scope; + } + + return $mutatingScope->mergeWith($scope); + } +} diff --git a/src/NodeTypeResolver/Node/AttributeKey.php b/src/NodeTypeResolver/Node/AttributeKey.php index 33f40638ebe..01fb961a9c8 100644 --- a/src/NodeTypeResolver/Node/AttributeKey.php +++ b/src/NodeTypeResolver/Node/AttributeKey.php @@ -176,4 +176,10 @@ final class AttributeKey public const string IS_IN_TRY_BLOCK = 'is_in_try_block'; public const string NEWLINE_ON_FLUENT_CALL = 'newline_on_fluent_call'; + + /** + * PHPStan virtual node that holds every exit point of a class method, + * @see \Rector\NodeAnalyzer\AlwaysInitializedPropertyAnalyzer to read it + */ + public const string METHOD_RETURN_STATEMENTS_NODE = 'method_return_statements_node'; } diff --git a/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php b/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php index 7c73de9d1ab..d7f3220c799 100644 --- a/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php +++ b/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php @@ -83,6 +83,7 @@ use PhpParser\Node\Stmt\Unset_; use PhpParser\Node\Stmt\While_; use PhpParser\Node\UnionType; +use PhpParser\NodeFinder; use PhpParser\NodeTraverser; use PHPStan\Analyser\Fiber\FiberScope; use PHPStan\Analyser\MutatingScope; @@ -92,6 +93,7 @@ use PHPStan\Node\FunctionCallableNode; use PHPStan\Node\InstantiationCallableNode; use PHPStan\Node\MethodCallableNode; +use PHPStan\Node\MethodReturnStatementsNode; use PHPStan\Node\Printer\Printer; use PHPStan\Node\StaticMethodCallableNode; use PHPStan\Node\UnreachableStatementNode; @@ -153,8 +155,17 @@ public function processNodes( $scope = $formerMutatingScope ?? $this->scopeFactory->createFromFile($filePath); + /** + * PHPStan emits the MethodReturnStatementsNode without the ClassMethod it belongs to, + * but with its attributes copied over, so the start file position pairs them back together + * + * @var array + */ + $classMethodsByStartFilePos = []; + $nodeCallback = function (Node $node, MutatingScope $mutatingScope) use ( &$nodeCallback, + &$classMethodsByStartFilePos, $filePath, ): void { if ($mutatingScope instanceof FiberScope) { @@ -194,11 +205,21 @@ public function processNodes( return; } + // emitted once the whole class method body is resolved, so the class method is already known + if ($node instanceof MethodReturnStatementsNode) { + $this->processMethodReturnStatementsNode($node, $classMethodsByStartFilePos); + return; + } + // init current Node set Attribute // not a VirtualNode, then set scope attribute // do not return early, as its properties will be checked next if (! $node instanceof VirtualNode) { $node->setAttribute(AttributeKey::SCOPE, $mutatingScope); + + if ($node instanceof ClassMethod && $node->getStartFilePos() >= 0) { + $classMethodsByStartFilePos[$node->getStartFilePos()] = $node; + } } // handle unwrapped stmts @@ -609,6 +630,47 @@ private function processUnreachableStatementNode( ); } + /** + * @param array $classMethodsByStartFilePos + */ + private function processMethodReturnStatementsNode( + MethodReturnStatementsNode $methodReturnStatementsNode, + array $classMethodsByStartFilePos + ): void { + $classMethod = $classMethodsByStartFilePos[$methodReturnStatementsNode->getStartFilePos()] ?? null; + if (! $classMethod instanceof ClassMethod) { + return; + } + + // the node keeps every exit point scope alive, so only pay for it where a property can be initialized + if (! $this->hasPropertyAssign($classMethod)) { + return; + } + + // kept raw on purpose, merging the exit point scopes is only worth it once a rule asks for it + $classMethod->setAttribute(AttributeKey::METHOD_RETURN_STATEMENTS_NODE, $methodReturnStatementsNode); + } + + private function hasPropertyAssign(ClassMethod $classMethod): bool + { + $nodeFinder = new NodeFinder(); + + $assign = $nodeFinder->findFirst((array) $classMethod->stmts, static function (Node $node): bool { + if (! $node instanceof Assign) { + return false; + } + + $assignVar = $node->var; + while ($assignVar instanceof ArrayDimFetch) { + $assignVar = $assignVar->var; + } + + return $assignVar instanceof PropertyFetch; + }); + + return $assign instanceof Assign; + } + /** * @param callable(Node $node, MutatingScope $scope): void $nodeCallback */ diff --git a/tests/NodeAnalyzer/AlwaysInitializedPropertyAnalyzerTest.php b/tests/NodeAnalyzer/AlwaysInitializedPropertyAnalyzerTest.php new file mode 100644 index 00000000000..5b9eeb588cc --- /dev/null +++ b/tests/NodeAnalyzer/AlwaysInitializedPropertyAnalyzerTest.php @@ -0,0 +1,72 @@ +alwaysInitializedPropertyAnalyzer = $this->make(AlwaysInitializedPropertyAnalyzer::class); + $this->testingParser = $this->make(TestingParser::class); + $this->betterNodeFinder = $this->make(BetterNodeFinder::class); + } + + #[DataProvider('provideData')] + public function test(string $filePath, string $propertyName, bool $expectedIsAlwaysInitialized): void + { + $stmts = $this->testingParser->parseFileToDecoratedNodes($filePath); + + $classMethod = $this->betterNodeFinder->findFirstInstanceOf($stmts, ClassMethod::class); + $this->assertInstanceOf(ClassMethod::class, $classMethod); + + $this->assertSame( + $expectedIsAlwaysInitialized, + $this->alwaysInitializedPropertyAnalyzer->isAlwaysInitialized($classMethod, $propertyName) + ); + } + + /** + * @return iterable + */ + public static function provideData(): iterable + { + $sourceDirectory = __DIR__ . '/Source/AlwaysInitializedProperty'; + + yield [$sourceDirectory . '/DirectAssign.php', 'name', true]; + + // never touched in the constructor + yield [$sourceDirectory . '/DirectAssign.php', 'surname', false]; + + // the early return skips the assign + yield [$sourceDirectory . '/EarlyReturn.php', 'name', false]; + + yield [$sourceDirectory . '/EveryBranchAssign.php', 'name', true]; + + // the loop body might never run + yield [$sourceDirectory . '/AssignInForeach.php', 'name', false]; + } + + public function testUnresolvedClassMethodIsNotInitialized(): void + { + $classMethod = new ClassMethod('__construct'); + + $this->assertFalse($this->alwaysInitializedPropertyAnalyzer->isAlwaysInitialized($classMethod, 'name')); + } +} diff --git a/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/AssignInForeach.php b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/AssignInForeach.php new file mode 100644 index 00000000000..84780c878ae --- /dev/null +++ b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/AssignInForeach.php @@ -0,0 +1,20 @@ +name = $name; + } + } +} diff --git a/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/DirectAssign.php b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/DirectAssign.php new file mode 100644 index 00000000000..f4165571b28 --- /dev/null +++ b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/DirectAssign.php @@ -0,0 +1,17 @@ +name = $name; + } +} diff --git a/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/EarlyReturn.php b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/EarlyReturn.php new file mode 100644 index 00000000000..0297e96a2fb --- /dev/null +++ b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/EarlyReturn.php @@ -0,0 +1,19 @@ +name = $name; + } +} diff --git a/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/EveryBranchAssign.php b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/EveryBranchAssign.php new file mode 100644 index 00000000000..1c0c9f2f507 --- /dev/null +++ b/tests/NodeAnalyzer/Source/AlwaysInitializedProperty/EveryBranchAssign.php @@ -0,0 +1,21 @@ +name = 'one'; + } elseif ($type === 2) { + $this->name = 'two'; + } else { + $this->name = 'many'; + } + } +}