Merge pull request #1465 from kukulich/promoted-hooks

Property hooks fixes
Roave · Dec 11, 2024 · e574da5 · e574da5
2 parents 8223078 + f9bad58
commit e574da5
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 32 deletions.
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -220,7 +220,6 @@
       <code><![CDATA[isStatic]]></code>
       <code><![CDATA[traitExists]]></code>
       <code><![CDATA[traverse]]></code>
-      <code><![CDATA[traverse]]></code>
     </ImpureMethodCall>
   </file>
   <file src="src/Reflection/ReflectionType.php">

diff --git a/src/Reflection/ReflectionClass.php b/src/Reflection/ReflectionClass.php
@@ -863,7 +863,7 @@ private function createImmediateProperties(ClassNode|InterfaceNode|TraitNode|Enu
             }
 
             foreach ($methodNode->params as $parameterNode) {
-                if ($parameterNode->flags === 0) {
+                if ($parameterNode->flags === 0 && $parameterNode->hooks === []) {
                     // No flags, no promotion
                     continue;
                 }
@@ -878,6 +878,7 @@ private function createImmediateProperties(ClassNode|InterfaceNode|TraitNode|Enu
                     $parameterNode->getAttributes(),
                     $parameterNode->type,
                     $parameterNode->attrGroups,
+                    $parameterNode->hooks,
                 );
                 $property                         = ReflectionProperty::createFromNode(
                     $reflector,

diff --git a/src/Reflection/ReflectionProperty.php b/src/Reflection/ReflectionProperty.php
@@ -723,24 +723,7 @@ private function computeImmediateVirtualBasedOnGetHook(PropertyNode $node, Node\
             return true;
         }
 
-        $visitor   = new FindingVisitor(static fn (Node $node): bool => $node instanceof Node\Expr\PropertyFetch);
-        $traverser = new NodeTraverser($visitor);
-        $traverser->traverse($getHookBody);
-
-        foreach ($visitor->getFoundNodes() as $propertyFetchNode) {
-            assert($propertyFetchNode instanceof Node\Expr\PropertyFetch);
-
-            if (
-                $propertyFetchNode->var instanceof Node\Expr\Variable
-                && $propertyFetchNode->var->name === 'this'
-                && $propertyFetchNode->name instanceof Node\Identifier
-                && $propertyFetchNode->name->name === $this->name
-            ) {
-                return false;
-            }
-        }
-
-        return true;
+        return ! $this->isHookUsingThisProperty($getHook);
     }
 
     private function computeImmediateVirtualBasedOnSetHook(Node\PropertyHook $setHook): bool
@@ -757,26 +740,29 @@ private function computeImmediateVirtualBasedOnSetHook(Node\PropertyHook $setHoo
             return false;
         }
 
-        $visitor   = new FindingVisitor(static fn (Node $node): bool => $node instanceof Node\Expr\Assign);
+        return ! $this->isHookUsingThisProperty($setHook);
+    }
+
+    private function isHookUsingThisProperty(Node\PropertyHook $hook): bool
+    {
+        $visitor   = new FindingVisitor(static fn (Node $node): bool => $node instanceof Node\Expr\PropertyFetch);
         $traverser = new NodeTraverser($visitor);
-        $traverser->traverse($setHookBody);
+        $traverser->traverse([$hook]);
 
-        foreach ($visitor->getFoundNodes() as $assigNode) {
-            assert($assigNode instanceof Node\Expr\Assign);
-            $variableToAssign = $assigNode->var;
+        foreach ($visitor->getFoundNodes() as $propertyFetchNode) {
+            assert($propertyFetchNode instanceof Node\Expr\PropertyFetch);
 
             if (
-                $variableToAssign instanceof Node\Expr\PropertyFetch
-                && $variableToAssign->var instanceof Node\Expr\Variable
-                && $variableToAssign->var->name === 'this'
-                && $variableToAssign->name instanceof Node\Identifier
-                && $variableToAssign->name->name === $this->name
+                $propertyFetchNode->var instanceof Node\Expr\Variable
+                && $propertyFetchNode->var->name === 'this'
+                && $propertyFetchNode->name instanceof Node\Identifier
+                && $propertyFetchNode->name->name === $this->name
             ) {
-                return false;
+                return true;
             }
         }
 
-        return true;
+        return false;
     }
 
     /** @return array{get?: ReflectionMethod, set?: ReflectionMethod} */

diff --git a/test/unit/Fixture/PropertyHooks.php b/test/unit/Fixture/PropertyHooks.php
@@ -128,3 +128,10 @@ interface InterfacePropertyHooks
 
     public string $readAndWriteHook { get; set; }
 }
+
+class PromotedPropertyHooks
+{
+    public function __construct(string $hook{ get {} })
+    {
+    }
+}
diff --git a/test/unit/Reflection/ReflectionPropertyTest.php b/test/unit/Reflection/ReflectionPropertyTest.php
@@ -1121,4 +1121,16 @@ public function testUseHookFromTrait(): void
         self::assertFalse($hookProperty->hasHook(ReflectionPropertyHookType::Set));
         self::assertSame('Roave\BetterReflectionTest\Fixture\PropertyHookTrait', $hookProperty->getHook(ReflectionPropertyHookType::Get)->getDeclaringClass()->getName());
     }
+
+    public function testPromotedPropertyWithHooks(): void
+    {
+        $reflector    = new DefaultReflector(new SingleFileSourceLocator(__DIR__ . '/../Fixture/PropertyHooks.php', $this->astLocator));
+        $getClassInfo = $reflector->reflectClass('Roave\BetterReflectionTest\Fixture\PromotedPropertyHooks');
+
+        $hookProperty = $getClassInfo->getProperty('hook');
+        self::assertTrue($hookProperty->isPromoted());
+        self::assertCount(1, $hookProperty->getHooks());
+        self::assertTrue($hookProperty->hasHook(ReflectionPropertyHookType::Get));
+        self::assertFalse($hookProperty->hasHook(ReflectionPropertyHookType::Set));
+    }
 }