Repository: CuyZ/Valinor (php)

Issue:
Crash / Infinite loop when target class constructor accepts `self`
Hi, stumbled into this:

```php
<?php declare(strict_types=1);

require_once __DIR__ . '/vendor/autoload.php';

class Test {
    public function __construct(string|self $value) {
        // copy-constructor-ish
    }
}

$mapper = (new CuyZ\Valinor\MapperBuilder)->mapper();

$test = $mapper->map(Test::class, ['value' => 'foo']); // string -> OK

$test = $mapper->map(Test::class, ['value' => $test]); // self -> OK

$test = $mapper->map(Test::class, ['value' => null]); // null -> FAIL
```

```
$ php test.php
PHP Fatal error:  Uncaught Error: Xdebug has detected a possible infinite loop, and aborted your script ...
```

Without Xdebug it just keeps running.
Infinite loop somewhere
Hello @romm, just wanted to let you know there's an infinite loop somewhere in https://github.com/CuyZ/Valinor/compare/1.14.2...1.14.3

The diff is huge but maybe you already have an idea where the issue is. Otherwise, I'll find it ~tomorrow~ today.

Pull request description:
fix: handle self-referential types in object constructors
Fixes #541
Fixes #606

Patch:
diff --git a/src/Mapper/Object/Argument.php b/src/Mapper/Object/Argument.php
index 339b563..e173483 100644
--- a/src/Mapper/Object/Argument.php
+++ b/src/Mapper/Object/Argument.php
@@ -12,8 +12,12 @@ use CuyZ\Valinor\Type\Type;
 /** @internal */
 final class Argument
 {
+    /** @var non-empty-string */
     private string $name;
 
+    /** @var non-empty-string */
+    private string $signature;
+
     private Type $type;
 
     private mixed $defaultValue = null;
@@ -22,15 +26,20 @@ final class Argument
 
     private Attributes $attributes;
 
-    public function __construct(string $name, Type $type)
+    /**
+     * @param non-empty-string $name
+     * @param non-empty-string $signature
+     */
+    public function __construct(string $name, string $signature, Type $type)
     {
-        $this->type = $type;
         $this->name = $name;
+        $this->signature = $signature;
+        $this->type = $type;
     }
 
     public static function fromParameter(ParameterDefinition $parameter): self
     {
-        $instance = new self($parameter->name, $parameter->type);
+        $instance = new self($parameter->name, $parameter->signature, $parameter->type);
         $instance->attributes = $parameter->attributes;
 
         if ($parameter->isOptional) {
@@ -43,7 +52,7 @@ final class Argument
 
     public static function fromProperty(PropertyDefinition $property): self
     {
-        $instance = new self($property->name, $property->type);
+        $instance = new self($property->name, $property->signature, $property->type);
         $instance->attributes = $property->attributes;
 
         if ($property->hasDefaultValue) {
@@ -54,11 +63,22 @@ final class Argument
         return $instance;
     }
 
+    /**
+     * @return non-empty-string
+     */
     public function name(): string
     {
         return $this->name;
     }
 
+    /**
+     * @return non-empty-string
+     */
+    public function signature(): string
+    {
+        return $this->signature;
+    }
+
     public function type(): Type
     {
         return $this->type;
diff --git a/src/Mapper/Object/ArgumentsValues.php b/src/Mapper/Object/ArgumentsValues.php
index d75a0ea..47fbb2c 100644
--- a/src/Mapper/Object/ArgumentsValues.php
+++ b/src/Mapper/Object/ArgumentsValues.php
@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace CuyZ\Valinor\Mapper\Object;
 
+use Countable;
 use CuyZ\Valinor\Mapper\Tree\Shell;
 use CuyZ\Valinor\Type\CompositeTraversableType;
 use CuyZ\Valinor\Type\Types\ArrayKeyType;
@@ -19,7 +20,7 @@ use function is_array;
  *
  * @implements IteratorAggregate<Argument>
  */
-final class ArgumentsValues implements IteratorAggregate
+final class ArgumentsValues implements IteratorAggregate, Countable
 {
     /** @var array<mixed> */
     private array $value = [];
@@ -129,6 +130,11 @@ final class ArgumentsValues implements IteratorAggregate
         return [$name => $value];
     }
 
+    public function count(): int
+    {
+        return count($this->arguments);
+    }
+
     public function getIterator(): Traversable
     {
         yield from $this->arguments;
diff --git a/src/Mapper/Object/Factory/StrictTypesObjectBuilderFactory.php b/src/Mapper/Object/Factory/StrictTypesObjectBuilderFactory.php
index 7aa8b9e..6a375e5 100644
--- a/src/Mapper/Object/Factory/StrictTypesObjectBuilderFactory.php
+++ b/src/Mapper/Object/Factory/StrictTypesObjectBuilderFactory.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace CuyZ\Valinor\Mapper\Object\Factory;
 
 use CuyZ\Valinor\Definition\ClassDefinition;
+use CuyZ\Valinor\Mapper\Object\Argument;
 use CuyZ\Valinor\Mapper\Object\Exception\PermissiveTypeNotAllowed;
 use CuyZ\Valinor\Type\CompositeType;
 use CuyZ\Valinor\Type\Type;
@@ -24,25 +25,23 @@ final class StrictTypesObjectBuilderFactory implements ObjectBuilderFactory
             $arguments = $builder->describeArguments();
 
             foreach ($arguments as $argument) {
-                $argumentSignature = $builder->signatureForArgument($argument->name());
-
-                $this->checkPresenceOfPermissiveType($argumentSignature, $argument->type());
+                $this->checkPresenceOfPermissiveType($argument, $argument->type());
             }
         }
 
         return $builders;
     }
 
-    private function checkPresenceOfPermissiveType(string $signature, Type $type): void
+    private function checkPresenceOfPermissiveType(Argument $argument, Type $type): void
     {
         if ($type instanceof CompositeType) {
             foreach ($type->traverse() as $subType) {
-                self::checkPresenceOfPermissiveType($signature, $subType);
+                self::checkPresenceOfPermissiveType($argument, $subType);
             }
         }
 
         if ($type instanceof MixedType || $type instanceof UndefinedObjectType) {
-            throw new PermissiveTypeNotAllowed($signature, $type);
+            throw new PermissiveTypeNotAllowed($argument->signature(), $type);
         }
     }
 }
diff --git a/src/Mapper/Object/FunctionObjectBuilder.php b/src/Mapper/Object/FunctionObjectBuilder.php
index 5516953..1d36058 100644
--- a/src/Mapper/Object/FunctionObjectBuilder.php
+++ b/src/Mapper/Object/FunctionObjectBuilder.php
@@ -71,9 +71,4 @@ final class FunctionObjectBuilder implements ObjectBuilder
     {
         return $this->function->definition->signature;
     }
-
-    public function signatureForArgument(string $argumentName): string
-    {
-        return $this->function->definition->parameters->get($argumentName)->signature;
-    }
 }
diff --git a/src/Mapper/Object/MethodObjectBuilder.php b/src/Mapper/Object/MethodObjectBuilder.php
index a29815d..25f25fd 100644
--- a/src/Mapper/Object/MethodObjectBuilder.php
+++ b/src/Mapper/Object/MethodObjectBuilder.php
@@ -40,9 +40,4 @@ final class MethodObjectBuilder implements ObjectBuilder
     {
         return "$this->className::$this->methodName()";
     }
-
-    public function signatureForArgument(string $argumentName): string
-    {
-        return $this->parameters->get($argumentName)->signature;
-    }
 }
diff --git a/src/Mapper/Object/NativeConstructorObjectBuilder.php b/src/Mapper/Object/NativeConstructorObjectBuilder.php
index 5c31828..808a16f 100644
--- a/src/Mapper/Object/NativeConstructorObjectBuilder.php
+++ b/src/Mapper/Object/NativeConstructorObjectBuilder.php
@@ -36,9 +36,4 @@ final class NativeConstructorObjectBuilder implements ObjectBuilder
     {
         return $this->class->methods->constructor()->signature;
     }
-
-    public function signatureForArgument(string $argumentName): string
-    {
-        return $this->class->methods->constructor()->parameters->get($argumentName)->signature;
-    }
 }
diff --git a/src/Mapper/Object/NativeEnumObjectBuilder.php b/src/Mapper/Object/NativeEnumObjectBuilder.php
index 2004475..d94903f 100644
--- a/src/Mapper/Object/NativeEnumObjectBuilder.php
+++ b/src/Mapper/Object/NativeEnumObjectBuilder.php
@@ -30,7 +30,7 @@ class NativeEnumObjectBuilder implements ObjectBuilder
 
         $this->enum = $type;
         $this->arguments = new Arguments(
-            new Argument('value', $argumentType)
+            new Argument('value', $type->className() . '::$value', $argumentType)
         );
     }
 
@@ -48,9 +48,4 @@ class NativeEnumObjectBuilder implements ObjectBuilder
     {
         return $this->enum->readableSignature();
     }
-
-    public function signatureForArgument(string $argumentName): string
-    {
-        return $this->enum->className() . '::$value';
-    }
 }
diff --git a/src/Mapper/Object/ObjectBuilder.php b/src/Mapper/Object/ObjectBuilder.php
index 4936f67..d5fd023 100644
--- a/src/Mapper/Object/ObjectBuilder.php
+++ b/src/Mapper/Object/ObjectBuilder.php
@@ -18,9 +18,4 @@ interface ObjectBuilder
      * @return non-empty-string
      */
     public function signature(): string;
-
-    /**
-     * @return non-empty-string
-     */
-    public function signatureForArgument(string $argumentName): string;
 }
diff --git a/src/Mapper/Object/ReflectionObjectBuilder.php b/src/Mapper/Object/ReflectionObjectBuilder.php
index 7e895f6..54eccbf 100644
--- a/src/Mapper/Object/ReflectionObjectBuilder.php
+++ b/src/Mapper/Object/ReflectionObjectBuilder.php
@@ -39,9 +39,4 @@ final class ReflectionObjectBuilder implements ObjectBuilder
     {
         return $this->class->name . ' (properties)';
     }
-
-    public function signatureForArgument(string $argumentName): string
-    {
-        return $this->class->properties->get($argumentName)->signature;
-    }
 }
diff --git a/src/Mapper/Tree/Builder/ObjectNodeBuilder.php b/src/Mapper/Tree/Builder/ObjectNodeBuilder.php
index e154cb7..3bbbcf6 100644
--- a/src/Mapper/Tree/Builder/ObjectNodeBuilder.php
+++ b/src/Mapper/Tree/Builder/ObjectNodeBuilder.php
@@ -10,6 +10,8 @@ use CuyZ\Valinor\Mapper\Object\Exception\CannotFindObjectBuilder;
 use CuyZ\Valinor\Mapper\Object\Exception\InvalidSource;
 use CuyZ\Valinor\Mapper\Object\Factory\ObjectBuilderFactory;
 use CuyZ\Valinor\Mapper\Object\ObjectBuilder;
+use CuyZ\Valinor\Mapper\Tree\Exception\CircularDependencyDetected;
+use CuyZ\Valinor\Mapper\Tree\Exception\InvalidNodeValue;
 use CuyZ\Valinor\Mapper\Tree\Message\ErrorMessage;
 use CuyZ\Valinor\Mapper\Tree\Message\Message;
 use CuyZ\Valinor\Mapper\Tree\Message\UserlandError;
@@ -106,7 +108,28 @@ final class ObjectNodeBuilder implements NodeBuilder
                 $child = $child->withValue($arguments->getValue($name));
             }
 
-            $children[] = $rootBuilder->build($child);
+            // This whole block is needed to detect object circular dependencies
+            // and prevent infinite loops.
+            if ($rootBuilder->typeWasSeen($type)) {
+                // An exception is thrown only when the type of the property is
+                // literally the same as the type of the object being built.
+                // Otherwise, the property type might be a union, for instance,
+                // so we do not want to stop the script execution right away
+                // because the value might be valid.
+                if (count($arguments) === 1 && $type instanceof ObjectType) {
+                    throw new CircularDependencyDetected($argument);
+                }
+
+                $children[] = TreeNode::error($shell, new InvalidNodeValue($type));
+            } else {
+                $childBuilder = $rootBuilder;
+
+                if ($type->matches($shell->type())) {
+                    $childBuilder = $rootBuilder->withTypeAsCurrentRoot($type);
+                }
+
+                $children[] = $childBuilder->build($child);
+            }
         }
 
         return $children;
diff --git a/src/Mapper/Tree/Builder/RootNodeBuilder.php b/src/Mapper/Tree/Builder/RootNodeBuilder.php
index de45741..724cd51 100644
--- a/src/Mapper/Tree/Builder/RootNodeBuilder.php
+++ b/src/Mapper/Tree/Builder/RootNodeBuilder.php
@@ -6,10 +6,16 @@ namespace CuyZ\Valinor\Mapper\Tree\Builder;
 
 use CuyZ\Valinor\Mapper\Tree\Exception\MissingNodeValue;
 use CuyZ\Valinor\Mapper\Tree\Shell;
+use CuyZ\Valinor\Type\Type;
 
 /** @internal */
 final class RootNodeBuilder
 {
+    /**
+     * This property is used to detect circular references for objects.
+     */
+    public Type $currentRootType;
+
     public function __construct(private NodeBuilder $root) {}
 
     public function build(Shell $shell): TreeNode
@@ -24,4 +30,18 @@ final class RootNodeBuilder
 
         return $this->root->build($shell, $this);
     }
+
+    public function withTypeAsCurrentRoot(Type $type): self
+    {
+        $self = clone $this;
+        $self->currentRootType = $type;
+
+        return $self;
+    }
+
+    public function typeWasSeen(Type $type): bool
+    {
+        return isset($this->currentRootType)
+            && $type->toString() === $this->currentRootType->toString();
+    }
 }
diff --git a/src/Mapper/Tree/Exception/CircularDependencyDetected.php b/src/Mapper/Tree/Exception/CircularDependencyDetected.php
new file mode 100644
index 0000000..d2aacac
--- /dev/null
+++ b/src/Mapper/Tree/Exception/CircularDependencyDetected.php
@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+namespace CuyZ\Valinor\Mapper\Tree\Exception;
+
+use CuyZ\Valinor\Mapper\Object\Argument;
+use LogicException;
+
+/** @internal */
+final class CircularDependencyDetected extends LogicException
+{
+    public function __construct(Argument $argument)
+    {
+        parent::__construct(
+            "Circular dependency detected for `{$argument->signature()}`.",
+            1739903374,
+        );
+    }
+}


Test patch:
diff --git a/tests/Fake/Mapper/Object/FakeObjectBuilder.php b/tests/Fake/Mapper/Object/FakeObjectBuilder.php
index 64abc9a..148184b 100644
--- a/tests/Fake/Mapper/Object/FakeObjectBuilder.php
+++ b/tests/Fake/Mapper/Object/FakeObjectBuilder.php
@@ -24,9 +24,4 @@ final class FakeObjectBuilder implements ObjectBuilder
     {
         return 'FakeObjectBuilder';
     }
-
-    public function signatureForArgument(string $argumentName): string
-    {
-        return 'FakeSignatureForArgument';
-    }
 }
diff --git a/tests/Integration/Mapping/SingleNodeMappingTest.php b/tests/Integration/Mapping/SingleNodeMappingTest.php
index 4e86fa0..e40c2c9 100644
--- a/tests/Integration/Mapping/SingleNodeMappingTest.php
+++ b/tests/Integration/Mapping/SingleNodeMappingTest.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace CuyZ\Valinor\Tests\Integration\Mapping;
 
 use CuyZ\Valinor\Mapper\MappingError;
+use CuyZ\Valinor\Mapper\Tree\Exception\CircularDependencyDetected;
 use CuyZ\Valinor\Tests\Integration\IntegrationTestCase;
 use CuyZ\Valinor\Tests\Integration\Mapping\Fixture\SimpleObject;
 use PHPUnit\Framework\Attributes\DataProvider;
@@ -78,6 +79,15 @@ final class SingleNodeMappingTest extends IntegrationTestCase
         }
     }
 
+    public function test_single_argument_with_self_type_throws_exception(): void
+    {
+        $this->expectException(CircularDependencyDetected::class);
+        $this->expectExceptionCode(1739903374);
+        $this->expectExceptionMessage('Circular dependency detected for `' . SinglePropertyWithSelfType::class . '::$value`.');
+
+        $this->mapperBuilder()->mapper()->map(SinglePropertyWithSelfType::class, 'foo');
+    }
+
     public static function single_property_and_constructor_parameter_data_provider(): iterable
     {
         yield 'Single scalar property' => [
@@ -110,6 +120,12 @@ final class SingleNodeMappingTest extends IntegrationTestCase
         yield 'Single constructor array parameter with array containing entry with same key as parameter name' => [
             SingleConstructorArrayParameter::class, ['value' => 'foo', 'otherValue' => 'bar'],
         ];
+        yield 'Single union property with self type' => [
+            SingleUnionPropertyWithSelfType::class, 'foo',
+        ];
+        yield 'Single constructor with union parameter with self type' => [
+            SingleConstructorUnionParameterWithSelfType::class, 'foo',
+        ];
     }
 
     public static function single_property_and_constructor_parameter_with_default_value_data_provider(): iterable
@@ -174,3 +190,21 @@ class SingleConstructorParameterWithDefaultValue extends SingleScalarPropertyWit
         $this->value = $value;
     }
 }
+
+class SingleUnionPropertyWithSelfType
+{
+    public self|string $value;
+}
+
+class SingleConstructorUnionParameterWithSelfType extends SingleUnionPropertyWithSelfType
+{
+    public function __construct(self|string $value)
+    {
+        $this->value = $value;
+    }
+}
+
+class SinglePropertyWithSelfType
+{
+    public self $value;
+}
diff --git a/tests/Unit/Mapper/Object/NativeEnumObjectBuilderTest.php b/tests/Unit/Mapper/Object/NativeEnumObjectBuilderTest.php
index 03f9163..a0d5691 100644
--- a/tests/Unit/Mapper/Object/NativeEnumObjectBuilderTest.php
+++ b/tests/Unit/Mapper/Object/NativeEnumObjectBuilderTest.php
@@ -15,6 +15,6 @@ final class NativeEnumObjectBuilderTest extends TestCase
     {
         $builder = new NativeEnumObjectBuilder(EnumType::native(PureEnum::class));
 
-        self::assertSame(PureEnum::class . '::$value', $builder->signatureForArgument('value'));
+        self::assertSame(PureEnum::class . '::$value', $builder->describeArguments()->at(0)->signature());
     }
 }
diff --git a/tests/Unit/Mapper/Tree/Builder/RootNodeBuilderTest.php b/tests/Unit/Mapper/Tree/Builder/RootNodeBuilderTest.php
new file mode 100644
index 0000000..a5f1df2
--- /dev/null
+++ b/tests/Unit/Mapper/Tree/Builder/RootNodeBuilderTest.php
@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace CuyZ\Valinor\Tests\Unit\Mapper\Tree\Builder;
+
+use CuyZ\Valinor\Mapper\Tree\Builder\RootNodeBuilder;
+use CuyZ\Valinor\Tests\Fake\Mapper\Tree\Builder\FakeNodeBuilder;
+use CuyZ\Valinor\Tests\Fake\Type\FakeType;
+use PHPUnit\Framework\TestCase;
+
+final class RootNodeBuilderTest extends TestCase
+{
+    public function test_with_type_as_current_root_returns_clone(): void
+    {
+        $rootNodeBuilderA = new RootNodeBuilder(new FakeNodeBuilder());
+        $rootNodeBuilderB = $rootNodeBuilderA->withTypeAsCurrentRoot(new FakeType());
+
+        self::assertNotSame($rootNodeBuilderA, $rootNodeBuilderB);
+    }
+}