diff --git a/rules-tests/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector/Fixture/skip_property_self_referenced_in_arrow_function.php.inc b/rules-tests/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector/Fixture/skip_property_self_referenced_in_arrow_function.php.inc new file mode 100644 index 000000000..fbf2281ac --- /dev/null +++ b/rules-tests/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector/Fixture/skip_property_self_referenced_in_arrow_function.php.inc @@ -0,0 +1,23 @@ +container = new LazyContainer([ + 'self' => fn (): LazyContainer => $this->container, + ]); + } + + public function test() + { + $this->assertTrue(true); + } +} diff --git a/rules-tests/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector/Source/LazyContainer.php b/rules-tests/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector/Source/LazyContainer.php new file mode 100644 index 000000000..4bbcfc68c --- /dev/null +++ b/rules-tests/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector/Source/LazyContainer.php @@ -0,0 +1,14 @@ + $factories + */ + public function __construct( + private array $factories + ) { + } +} diff --git a/rules/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector.php b/rules/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector.php index d077cfba5..da392e12b 100644 --- a/rules/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector.php +++ b/rules/CodeQuality/Rector/Class_/NarrowUnusedSetUpDefinedPropertyRector.php @@ -6,6 +6,8 @@ use PhpParser\Node; use PhpParser\Node\Expr; +use PhpParser\Node\Expr\ArrowFunction; +use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\Variable; @@ -124,7 +126,7 @@ public function refactor(Node $node): ?Node // referenced inside a nested closure that may run after setUp() - turning it into a local // variable would leave it out of the closure scope, as there is no "use" binding to add it - if ($this->isPropertyUsedInNestedClosure($setUpClassMethod, $propertyName)) { + if ($this->isPropertyUsedInUnsafeNestedFunction($setUpClassMethod, $propertyName)) { continue; } @@ -196,28 +198,26 @@ private function isPropertyUsedOutsideSetUpClassMethod( return $isPropertyUsed; } - private function isPropertyUsedInNestedClosure(ClassMethod $setUpClassMethod, string $propertyName): bool + private function isPropertyUsedInUnsafeNestedFunction(ClassMethod $setUpClassMethod, string $propertyName): bool { - // only regular closures are unsafe: they do not capture outer variables without an - // explicit "use" binding. Arrow functions auto-capture by value, so narrowing is safe there. - $closures = $this->nodeFinder->findInstanceOf($setUpClassMethod, Closure::class); - - foreach ($closures as $closure) { - $usedPropertyFetch = $this->nodeFinder->findFirst($closure, function (Node $node) use ( - $propertyName - ): bool { - if (! $node instanceof PropertyFetch) { - return false; - } - - if (! $this->isName($node->var, 'this')) { - return false; - } + // a regular closure does not capture outer variables without an explicit "use" binding, + // so turning the property into a local variable always leaves it undefined inside the closure + foreach ($this->nodeFinder->findInstanceOf($setUpClassMethod, Closure::class) as $closure) { + if ($this->refersToThisProperty($closure, $propertyName)) { + return true; + } + } - return $this->isName($node->name, $propertyName); - }); + // an arrow function auto-captures by value at definition time; that only matches the original + // lazy "$this->property" read when the property assignment is already complete before the arrow + // function is defined. A later or wrapping assignment (e.g. a self-referencing type definition) + // would capture a not-yet-assigned variable + foreach ($this->nodeFinder->findInstanceOf($setUpClassMethod, ArrowFunction::class) as $arrowFunction) { + if (! $this->refersToThisProperty($arrowFunction, $propertyName)) { + continue; + } - if ($usedPropertyFetch instanceof PropertyFetch) { + if (! $this->isPropertyAssignedBefore($setUpClassMethod, $propertyName, $arrowFunction)) { return true; } } @@ -225,6 +225,57 @@ private function isPropertyUsedInNestedClosure(ClassMethod $setUpClassMethod, st return false; } + private function refersToThisProperty(Node $node, string $propertyName): bool + { + $propertyFetch = $this->nodeFinder->findFirst($node, function (Node $subNode) use ($propertyName): bool { + if (! $subNode instanceof PropertyFetch) { + return false; + } + + if (! $this->isName($subNode->var, 'this')) { + return false; + } + + return $this->isName($subNode->name, $propertyName); + }); + + return $propertyFetch instanceof PropertyFetch; + } + + private function isPropertyAssignedBefore( + ClassMethod $setUpClassMethod, + string $propertyName, + ArrowFunction $arrowFunction + ): bool { + $arrowFunctionStartTokenPos = $arrowFunction->getStartTokenPos(); + + $assignBefore = $this->nodeFinder->findFirst($setUpClassMethod, function (Node $node) use ( + $propertyName, + $arrowFunctionStartTokenPos + ): bool { + if (! $node instanceof Assign) { + return false; + } + + if (! $node->var instanceof PropertyFetch) { + return false; + } + + if (! $this->isName($node->var->var, 'this')) { + return false; + } + + if (! $this->isName($node->var->name, $propertyName)) { + return false; + } + + // assignment fully completes before the arrow function starts + return $node->getEndTokenPos() < $arrowFunctionStartTokenPos; + }); + + return $assignBefore instanceof Assign; + } + private function shouldSkipProperty( bool $isFinalClass, Property $property,