Merge pull request #652 from Ocramius/fix/#646-#632-#645-only-variables-returned-by-ref-in-generated-code

#646 #632 #645 only variables should be returned by-ref in generated dynamic property access simulation code

Author: Ocramius

infection.json.dist

@@ -10,6 +10,6 @@
             "branch": "master"
         }
     },
-    "minMsi": 84.3,
+    "minMsi": 84.5,
     "minCoveredMsi": 86
 }

src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicGet.php

@@ -39,7 +39,7 @@ public function __construct(
         $callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
             PublicScopeSimulator::OPERATION_GET,
             'name',
-            'value',
+            null,
             $valueHolder,
             'returnValue',
             $originalClass

src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicIsset.php

@@ -39,7 +39,7 @@ public function __construct(
         $callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
             PublicScopeSimulator::OPERATION_ISSET,
             'name',
-            'value',
+            null,
             $valueHolder,
             'returnValue',
             $originalClass

src/ProxyManager/ProxyGenerator/AccessInterceptorValueHolder/MethodGenerator/MagicUnset.php

@@ -39,7 +39,7 @@ public function __construct(
         $callParent = PublicScopeSimulator::getPublicAccessSimulationCode(
             PublicScopeSimulator::OPERATION_UNSET,
             'name',
-            'value',
+            null,
             $valueHolder,
             'returnValue',
             $originalClass

src/ProxyManager/ProxyGenerator/Util/PublicScopeSimulator.php

@@ -8,6 +8,9 @@
 use Laminas\Code\Generator\PropertyGenerator;
 use ReflectionClass;
 
+use function array_filter;
+use function array_map;
+use function implode;
 use function sprintf;
 use function var_export;
 
@@ -30,14 +33,17 @@ class PublicScopeSimulator
      *
      * @param string            $operationType      operation to execute: one of 'get', 'set', 'isset' or 'unset'
      * @param string            $nameParameter      name of the `name` parameter of the magic method
-     * @param string|null       $valueParameter     name of the `value` parameter of the magic method
+     * @param string|null       $valueParameter     name of the `value` parameter of the magic method, only to be
+     *                                              used with $operationType 'set'
      * @param PropertyGenerator $valueHolder        name of the property containing the target object from which
      *                                              to read the property. `$this` if none provided
      * @param string|null       $returnPropertyName name of the property to which we want to assign the result of
      *                                              the operation. Return directly if none provided
      * @param string|null       $interfaceName      name of the proxified interface if any
      *
      * @throws InvalidArgumentException
+     *
+     * @psalm-param $operationType self::OPERATION_*
      */
     public static function getPublicAccessSimulationCode(
         string $operationType,
@@ -48,7 +54,6 @@ public static function getPublicAccessSimulationCode(
         ?ReflectionClass $originalClass = null
     ): string {
         $byRef  = self::getByRefReturnValue($operationType);
-        $value  = $operationType === self::OPERATION_SET ? ', $value' : '';
         $target = '$this';
 
         if ($valueHolder) {
@@ -59,27 +64,37 @@ public static function getPublicAccessSimulationCode(
             ? 'new \\ReflectionClass(get_parent_class($this))'
             : 'new \\ReflectionClass(' . var_export($originalClass->getName(), true) . ')';
 
+        $accessorEvaluation = $returnPropertyName
+            ? '$' . $returnPropertyName . ' = ' . $byRef . '$accessor();'
+            : '$returnValue = ' . $byRef . '$accessor();' . "\n\n" . 'return $returnValue;';
+
+        if ($operationType === self::OPERATION_UNSET) {
+            $accessorEvaluation = '$accessor();';
+        }
+
         return '$realInstanceReflection = ' . $originalClassReflection . ';' . "\n\n"
             . 'if (! $realInstanceReflection->hasProperty($' . $nameParameter . ')) {' . "\n"
             . '    $targetObject = ' . $target . ';' . "\n\n"
             . self::getUndefinedPropertyNotice($operationType, $nameParameter)
             . '    ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
-            . "    return;\n"
             . '}' . "\n\n"
             . '$targetObject = ' . self::getTargetObject($valueHolder) . ";\n"
-            . '$accessor = function ' . $byRef . '() use ($targetObject, $name' . $value . ') {' . "\n"
+            . '$accessor = function ' . $byRef . '() use ('
+            . implode(', ', array_map(
+                static fn (string $parameterName): string => '$' . $parameterName,
+                array_filter(['targetObject', $nameParameter, $valueParameter])
+            ))
+            . ') {' . "\n"
             . '    ' . self::getOperation($operationType, $nameParameter, $valueParameter) . "\n"
             . "};\n"
-            . self::getScopeReBind()
-            . (
-                $returnPropertyName
-                    ? '$' . $returnPropertyName . ' = ' . $byRef . '$accessor();'
-                    : '$returnValue = ' . $byRef . '$accessor();' . "\n\n" . 'return $returnValue;'
-            );
+            . self::generateScopeReBind()
+            . $accessorEvaluation;
     }
 
     /**
      * This will generate code that triggers a notice if access is attempted on a non-existing property
+     *
+     * @psalm-param $operationType self::OPERATION_*
      */
     private static function getUndefinedPropertyNotice(string $operationType, string $nameParameter): string
     {
@@ -106,6 +121,8 @@ private static function getUndefinedPropertyNotice(string $operationType, string
      * Note: if the object is a wrapper, the wrapped instance is accessed directly. If the object
      * is a ghost or the proxy has no wrapper, then an instance of the parent class is created via
      * on-the-fly unserialization
+     *
+     * @psalm-param $operationType self::OPERATION_*
      */
     private static function getByRefReturnValue(string $operationType): string
     {
@@ -126,25 +143,43 @@ private static function getTargetObject(?PropertyGenerator $valueHolder = null):
 
     /**
      * @throws InvalidArgumentException
+     *
+     * @psalm-param $operationType self::OPERATION_*
      */
     private static function getOperation(string $operationType, string $nameParameter, ?string $valueParameter): string
     {
+        if ($valueParameter !== null && $operationType !== self::OPERATION_SET) {
+            throw new InvalidArgumentException(
+                'Parameter $valueParameter should be provided (only) when $operationType === "' . self::OPERATION_SET . '"'
+                . self::class
+                . '::OPERATION_SET'
+            );
+        }
+
         switch ($operationType) {
             case self::OPERATION_GET:
                 return 'return $targetObject->$' . $nameParameter . ';';
 
             case self::OPERATION_SET:
                 if ($valueParameter === null) {
-                    throw new InvalidArgumentException('Parameter $valueParameter not provided');
+                    throw new InvalidArgumentException(
+                        'Parameter $valueParameter should be provided (only) when $operationType === "' . self::OPERATION_SET . '"'
+                        . self::class
+                        . '::OPERATION_SET'
+                    );
                 }
 
-                return 'return $targetObject->$' . $nameParameter . ' = $' . $valueParameter . ';';
+                return '$targetObject->$' . $nameParameter . ' = $' . $valueParameter . ';'
+                    . "\n\n"
+                    . '    return $targetObject->$' . $nameParameter . ';';
 
             case self::OPERATION_ISSET:
                 return 'return isset($targetObject->$' . $nameParameter . ');';
 
             case self::OPERATION_UNSET:
-                return 'unset($targetObject->$' . $nameParameter . ');';
+                return 'unset($targetObject->$' . $nameParameter . ');'
+                    . "\n\n"
+                    . '    return;';
         }
 
         throw new InvalidArgumentException(sprintf('Invalid operation "%s" provided', $operationType));
@@ -153,11 +188,13 @@ private static function getOperation(string $operationType, string $nameParamete
     /**
      * Generates code to bind operations to the parent scope
      */
-    private static function getScopeReBind(): string
+    private static function generateScopeReBind(): string
     {
-        return '$backtrace = debug_backtrace(true, 2);' . "\n"
-            . '$scopeObject = isset($backtrace[1][\'object\'])'
-            . ' ? $backtrace[1][\'object\'] : new \ProxyManager\Stub\EmptyClassStub();' . "\n"
-            . '$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));' . "\n";
+        return <<<'PHP'
+$backtrace = debug_backtrace(true, 2);
+$scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
+$accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));
+
+PHP;
     }
 }

tests/ProxyManagerTest/Functional/PublicScopeSimulatorFunctionalTest.php

@@ -0,0 +1,49 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ProxyManagerTest\ProxyGenerator\Functional;
+
+use PHPUnit\Framework\TestCase;
+use ProxyManager\ProxyGenerator\Util\PublicScopeSimulator;
+use ProxyManagerTestAsset\ClassWithMixedProperties;
+
+use function sprintf;
+
+/**
+ * @covers \ProxyManager\ProxyGenerator\Util\PublicScopeSimulator
+ * @coversNothing
+ */
+final class PublicScopeSimulatorFunctionalTest extends TestCase
+{
+    /**
+     * @group #632
+     * @group #645
+     * @group #646
+     */
+    public function testAccessingUndefinedPropertiesDoesNotLeadToInvalidByRefAccess(): void
+    {
+        /** @psalm-var ClassWithMixedProperties $sut */
+        $sut = eval(sprintf(
+            <<<'PHP'
+return new class() extends %s {
+    public function doGet($prop) : string { %s }
+    public function doSet($prop, $val) : string { %s }
+    public function doIsset($prop) : bool { %s }
+    public function doUnset($prop) : void { %s }
+};
+PHP
+            ,
+            ClassWithMixedProperties::class,
+            PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_GET, 'prop'),
+            PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_SET, 'prop', 'val'),
+            PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_ISSET, 'prop'),
+            PublicScopeSimulator::getPublicAccessSimulationCode(PublicScopeSimulator::OPERATION_UNSET, 'prop')
+        ));
+
+        self::assertSame('publicProperty0', $sut->doGet('publicProperty0'));
+        self::assertSame('bar', $sut->doSet('publicProperty0', 'bar'));
+        self::assertTrue($sut->doIsset('publicProperty0'));
+        self::assertNull($sut->doUnset('publicProperty0'));
+    }
+}

tests/ProxyManagerTest/ProxyGenerator/AccessInterceptorScopeLocalizer/MethodGenerator/MagicUnsetTest.php

@@ -39,7 +39,7 @@ public function testBodyStructure(): void
 
         self::assertSame('__unset', $magicGet->getName());
         self::assertCount(1, $magicGet->getParameters());
-        self::assertStringMatchesFormat('%a$returnValue = $accessor();%a', $magicGet->getBody());
+        self::assertStringMatchesFormat('%a$accessor();%a', $magicGet->getBody());
     }
 
     /**

tests/ProxyManagerTest/ProxyGenerator/LazyLoadingValueHolder/MethodGenerator/MagicUnsetTest.php

@@ -41,7 +41,7 @@ public function testBodyStructure(): void
             "\$this->foo && \$this->foo->__invoke(\$this->bar, \$this, '__unset', array('name' => \$name)"
             . ", \$this->foo);\n\n"
             . "if (isset(self::\$bar[\$name])) {\n    unset(\$this->bar->\$name);\n\n    return;\n}"
-            . '%areturn %s;',
+            . '%a',
             $magicIsset->getBody()
         );
     }