Merge pull request #902 from Roave/feature/#901-ensure-that-simple-un…

…ion-types-are-turned-into-nullable-reflection-named-types Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType`
Roave · Dec 5, 2021 · 2b64de3 · 2b64de3
2 parents ff18794 + 33a5645
commit 2b64de3
Show file tree

Hide file tree

Showing 26 changed files with 305 additions and 107 deletions.
diff --git a/src/Reflection/Adapter/ReflectionEnum.php b/src/Reflection/Adapter/ReflectionEnum.php
@@ -509,7 +509,7 @@ public function isBacked(): bool
     public function getBackingType(): ?ReflectionNamedType
     {
         if ($this->betterReflectionEnum->isBacked()) {
-            return new ReflectionNamedType($this->betterReflectionEnum->getBackingType());
+            return new ReflectionNamedType($this->betterReflectionEnum->getBackingType(), false);
         }
 
         return null;

diff --git a/src/Reflection/Adapter/ReflectionNamedType.php b/src/Reflection/Adapter/ReflectionNamedType.php
@@ -12,7 +12,7 @@
  */
 final class ReflectionNamedType extends CoreReflectionNamedType
 {
-    public function __construct(private BetterReflectionNamedType $betterReflectionType)
+    public function __construct(private BetterReflectionNamedType $betterReflectionType, private bool $allowsNull)
     {
     }
 
@@ -23,12 +23,13 @@ public function getName(): string
 
     public function __toString(): string
     {
-        return $this->betterReflectionType->__toString();
+        return ($this->allowsNull ? '?' : '')
+            . $this->betterReflectionType->__toString();
     }
 
     public function allowsNull(): bool
     {
-        return $this->betterReflectionType->allowsNull();
+        return $this->allowsNull;
     }
 
     public function isBuiltin(): bool

diff --git a/src/Reflection/Adapter/ReflectionType.php b/src/Reflection/Adapter/ReflectionType.php
@@ -9,6 +9,10 @@
 use Roave\BetterReflection\Reflection\ReflectionNamedType as BetterReflectionNamedType;
 use Roave\BetterReflection\Reflection\ReflectionUnionType as BetterReflectionUnionType;
 
+use function array_filter;
+use function array_values;
+use function count;
+
 abstract class ReflectionType extends CoreReflectionType
 {
     public static function fromTypeOrNull(BetterReflectionNamedType|BetterReflectionUnionType|BetterReflectionIntersectionType|null $betterReflectionType): ReflectionUnionType|ReflectionNamedType|ReflectionIntersectionType|null
@@ -18,13 +22,27 @@ public static function fromTypeOrNull(BetterReflectionNamedType|BetterReflection
         }
 
         if ($betterReflectionType instanceof BetterReflectionUnionType) {
+            // php-src has this weird behavior where a union type composed of a single type `T`
+            // together with `null` means that a `ReflectionNamedType` for `?T` is produced,
+            // rather than `T|null`. This is done to keep BC compatibility with PHP 7.1 (which
+            // introduced nullable types), but at reflection level, this is mostly a nuisance.
+            // In order to keep parity with core, we stashed this weird behavior in here.
+            $nonNullTypes = array_values(array_filter(
+                $betterReflectionType->getTypes(),
+                static fn (BetterReflectionNamedType $type): bool => $type->getName() !== 'null',
+            ));
+
+            if ($betterReflectionType->allowsNull() && count($nonNullTypes) === 1) {
+                return new ReflectionNamedType($nonNullTypes[0], true);
+            }
+
             return new ReflectionUnionType($betterReflectionType);
         }
 
         if ($betterReflectionType instanceof BetterReflectionIntersectionType) {
             return new ReflectionIntersectionType($betterReflectionType);
         }
 
-        return new ReflectionNamedType($betterReflectionType);
+        return new ReflectionNamedType($betterReflectionType, false);
     }
 }
diff --git a/src/Reflection/ReflectionIntersectionType.php b/src/Reflection/ReflectionIntersectionType.php
@@ -22,7 +22,7 @@ public function __construct(
         ReflectionParameter|ReflectionMethod|ReflectionFunction|ReflectionEnum|ReflectionProperty $owner,
         IntersectionType $type,
     ) {
-        parent::__construct($reflector, $owner, false);
+        parent::__construct($reflector, $owner);
 
         $this->types = array_filter(
             array_map(static fn (Node\Identifier|Node\Name $type): ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType => ReflectionType::createFromNode($reflector, $owner, $type), $type->types),
@@ -38,6 +38,11 @@ public function getTypes(): array
         return $this->types;
     }
 
+    public function allowsNull(): bool
+    {
+        return false;
+    }
+
     public function __toString(): string
     {
         return implode('&', array_map(static fn (ReflectionNamedType $type): string => $type->__toString(), $this->types));

diff --git a/src/Reflection/ReflectionNamedType.php b/src/Reflection/ReflectionNamedType.php
@@ -40,9 +40,8 @@ public function __construct(
         Reflector $reflector,
         ReflectionParameter|ReflectionMethod|ReflectionFunction|ReflectionEnum|ReflectionProperty $owner,
         Identifier|Name $type,
-        bool $allowsNull,
     ) {
-        parent::__construct($reflector, $owner, $allowsNull);
+        parent::__construct($reflector, $owner);
 
         $this->name = $type->toString();
     }
@@ -102,13 +101,13 @@ public function getClass(): ReflectionClass
         throw new LogicException(sprintf('The type %s cannot be resolved to class', $this->name));
     }
 
-    public function __toString(): string
+    public function allowsNull(): bool
     {
-        $name = '';
-        if ($this->allowsNull()) {
-            $name .= '?';
-        }
+        return false;
+    }
 
-        return $name . $this->getName();
+    public function __toString(): string
+    {
+        return $this->getName();
     }
 }
diff --git a/src/Reflection/ReflectionType.php b/src/Reflection/ReflectionType.php
@@ -16,7 +16,6 @@ abstract class ReflectionType
     protected function __construct(
         protected Reflector $reflector,
         protected ReflectionParameter|ReflectionMethod|ReflectionFunction|ReflectionEnum|ReflectionProperty $owner,
-        private bool $allowsNull,
     ) {
     }
 
@@ -27,32 +26,36 @@ public static function createFromNode(
         Reflector $reflector,
         ReflectionParameter|ReflectionMethod|ReflectionFunction|ReflectionEnum|ReflectionProperty $owner,
         Identifier|Name|NullableType|UnionType|IntersectionType $type,
-        bool $forceAllowsNull = false,
+        bool $allowsNull = false,
     ): ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType {
-        $allowsNull = $forceAllowsNull;
         if ($type instanceof NullableType) {
             $type       = $type->type;
             $allowsNull = true;
         }
 
         if ($type instanceof Identifier || $type instanceof Name) {
-            return new ReflectionNamedType($reflector, $owner, $type, $allowsNull);
+            if ($allowsNull) {
+                return new ReflectionUnionType(
+                    $reflector,
+                    $owner,
+                    new UnionType([$type, new Identifier('null')]),
+                );
+            }
+
+            return new ReflectionNamedType($reflector, $owner, $type);
         }
 
         if ($type instanceof IntersectionType) {
             return new ReflectionIntersectionType($reflector, $owner, $type);
         }
 
-        return new ReflectionUnionType($reflector, $owner, $type, $allowsNull);
+        return new ReflectionUnionType($reflector, $owner, $type);
     }
 
     /**
-     * Does the parameter allow null?
+     * Does the type allow null?
      */
-    public function allowsNull(): bool
-    {
-        return $this->allowsNull;
-    }
+    abstract public function allowsNull(): bool;
 
     /**
      * Convert this string type to a string

diff --git a/src/Reflection/ReflectionUnionType.php b/src/Reflection/ReflectionUnionType.php
@@ -21,9 +21,8 @@ public function __construct(
         Reflector $reflector,
         ReflectionParameter|ReflectionMethod|ReflectionFunction|ReflectionEnum|ReflectionProperty $owner,
         UnionType $type,
-        bool $allowsNull,
     ) {
-        parent::__construct($reflector, $owner, $allowsNull);
+        parent::__construct($reflector, $owner);
 
         $this->types = array_filter(
             array_map(static fn (Node\Identifier|Node\Name $type): ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType => ReflectionType::createFromNode($reflector, $owner, $type), $type->types),
@@ -39,6 +38,17 @@ public function getTypes(): array
         return $this->types;
     }
 
+    public function allowsNull(): bool
+    {
+        foreach ($this->types as $type) {
+            if ($type->getName() === 'null') {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     public function __toString(): string
     {
         return implode('|', array_map(static fn (ReflectionType $type): string => $type->__toString(), $this->types));

diff --git a/src/Reflection/StringCast/ReflectionFunctionStringCast.php b/src/Reflection/StringCast/ReflectionFunctionStringCast.php
@@ -72,6 +72,12 @@ private static function parametersToString(ReflectionFunction $functionReflectio
 
     private static function returnTypeToString(ReflectionFunction $methodReflection): string
     {
-        return $methodReflection->getReturnType()?->__toString() ?? '';
+        $type = $methodReflection->getReturnType();
+
+        if ($type === null) {
+            return '';
+        }
+
+        return ReflectionTypeStringCast::toString($type);
     }
 }
diff --git a/src/Reflection/StringCast/ReflectionMethodStringCast.php b/src/Reflection/StringCast/ReflectionMethodStringCast.php
@@ -126,6 +126,12 @@ private static function parametersToString(ReflectionMethod $methodReflection):
 
     private static function returnTypeToString(ReflectionMethod $methodReflection): string
     {
-        return $methodReflection->getReturnType()?->__toString() ?? '';
+        $type = $methodReflection->getReturnType();
+
+        if ($type === null) {
+            return '';
+        }
+
+        return ReflectionTypeStringCast::toString($type);
     }
 }
diff --git a/src/Reflection/StringCast/ReflectionParameterStringCast.php b/src/Reflection/StringCast/ReflectionParameterStringCast.php
@@ -34,11 +34,13 @@ public static function toString(ReflectionParameter $parameterReflection): strin
 
     private static function typeToString(ReflectionParameter $parameterReflection): string
     {
-        if (! $parameterReflection->hasType()) {
+        $type = $parameterReflection->getType();
+
+        if ($type === null) {
             return '';
         }
 
-        return (string) $parameterReflection->getType() . ' ';
+        return ReflectionTypeStringCast::toString($type) . ' ';
     }
 
     private static function valueToString(ReflectionParameter $parameterReflection): string

diff --git a/src/Reflection/StringCast/ReflectionPropertyStringCast.php b/src/Reflection/StringCast/ReflectionPropertyStringCast.php
@@ -29,7 +29,7 @@ public static function toString(ReflectionProperty $propertyReflection): string
             self::visibilityToString($propertyReflection),
             $propertyReflection->isStatic() ? ' static' : '',
             $propertyReflection->isReadOnly() ? ' readonly' : '',
-            $type !== null ? sprintf(' %s', $type->__toString()) : '',
+            $type !== null ? sprintf(' %s', ReflectionTypeStringCast::toString($type)) : '',
             $propertyReflection->getName(),
         );
     }

diff --git a/src/Reflection/StringCast/ReflectionTypeStringCast.php b/src/Reflection/StringCast/ReflectionTypeStringCast.php
@@ -0,0 +1,42 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Roave\BetterReflection\Reflection\StringCast;
+
+use Roave\BetterReflection\Reflection\ReflectionIntersectionType;
+use Roave\BetterReflection\Reflection\ReflectionNamedType;
+use Roave\BetterReflection\Reflection\ReflectionUnionType;
+
+use function array_filter;
+use function array_values;
+use function count;
+
+/**
+ * @internal
+ */
+final class ReflectionTypeStringCast
+{
+    public static function toString(
+        ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType $type,
+    ): string {
+        if ($type instanceof ReflectionUnionType) {
+            // php-src has this weird behavior where a union type composed of a single type `T`
+            // together with `null` means that a `ReflectionNamedType` for `?T` is produced,
+            // rather than `T|null`. This is done to keep BC compatibility with PHP 7.1 (which
+            // introduced nullable types), but at reflection level, this is mostly a nuisance.
+            // In order to keep parity with core `Reflector#__toString()` behavior, we stashed
+            // this weird behavior in here.
+            $nonNullTypes = array_values(array_filter(
+                $type->getTypes(),
+                static fn (ReflectionNamedType $type): bool => $type->getName() !== 'null',
+            ));
+
+            if ($type->allowsNull() && count($nonNullTypes) === 1) {
+                return '?' . $nonNullTypes[0]->__toString();
+            }
+        }
+
+        return $type->__toString();
+    }
+}
diff --git a/test/compat/ResolvePHP7Types.phpt b/test/compat/ResolvePHP7Types.phpt
@@ -24,36 +24,28 @@ $functionInfo = $reflector->reflectFunction('myFunction');
 $returnType = $functionInfo->getReturnType();
 
 var_dump([
-    'builtIn' => $returnType->isBuiltin(),
     'type' => $returnType->__toString(),
 ]);
 
 array_map(function (\Roave\BetterReflection\Reflection\ReflectionParameter $param) {
     $type = $param->getType();
 
     var_dump([
-        'builtIn' => $type->isBuiltin(),
         'type' => $type->__toString(),
     ]);
 }, $functionInfo->getParameters());
 
 ?>
 --EXPECTF--
-array(2) {
-  ["builtIn"]=>
-  bool(true)
+array(1) {
   ["type"]=>
   string(4) "bool"
 }
-array(2) {
-  ["builtIn"]=>
-  bool(true)
+array(1) {
   ["type"]=>
   string(3) "int"
 }
-array(2) {
-  ["builtIn"]=>
-  bool(true)
+array(1) {
   ["type"]=>
-  string(7) "?string"
+  string(11) "string|null"
 }
diff --git a/test/unit/Reflection/Adapter/ReflectionEnumTest.php b/test/unit/Reflection/Adapter/ReflectionEnumTest.php
@@ -648,8 +648,10 @@ public function testGetBackingTypeForBackedEnum(): void
             ->willReturn($betterReflectionNamedType);
 
         $reflectionEnumAdapter = new ReflectionEnumAdapter($betterReflectionEnum);
+        $backingType           = $reflectionEnumAdapter->getBackingType();
 
-        self::assertInstanceOf(ReflectionNamedTypeAdapter::class, $reflectionEnumAdapter->getBackingType());
+        self::assertInstanceOf(ReflectionNamedTypeAdapter::class, $backingType);
+        self::assertFalse($backingType->allowsNull());
     }
 
     public function testHasConstantWithEnumCase(): void

diff --git a/test/unit/Reflection/Adapter/ReflectionNamedTypeTest.php b/test/unit/Reflection/Adapter/ReflectionNamedTypeTest.php
@@ -37,11 +37,27 @@ public function testCoreReflectionMethods(string $methodName): void
         self::assertSame(ReflectionNamedTypeAdapter::class, $reflectionTypeAdapterReflection->getMethod($methodName)->getDeclaringClass()->getName());
     }
 
+    public function testWillRenderNullabilityMarkerWhenGiven(): void
+    {
+        $reflectionStub = $this->createMock(BetterReflectionNamedType::class);
+        $reflectionStub->method('__toString')
+            ->willReturn('foo');
+
+        self::assertSame('foo', (new ReflectionNamedTypeAdapter($reflectionStub, false))->__toString());
+        self::assertSame('?foo', (new ReflectionNamedTypeAdapter($reflectionStub, true))->__toString());
+    }
+
+    public function testWillReportThatItAcceptsOrRejectsNull(): void
+    {
+        $reflectionStub = $this->createMock(BetterReflectionNamedType::class);
+
+        self::assertFalse((new ReflectionNamedTypeAdapter($reflectionStub, false))->allowsNull());
+        self::assertTrue((new ReflectionNamedTypeAdapter($reflectionStub, true))->allowsNull());
+    }
+
     public function methodExpectationProvider(): array
     {
         return [
-            ['__toString', null, 'int', []],
-            ['allowsNull', null, true, []],
             ['isBuiltin', null, true, []],
             ['getName', null, 'int', []],
         ];
@@ -67,7 +83,7 @@ public function testAdapterMethods(string $methodName, ?string $expectedExceptio
             $this->expectException($expectedException);
         }
 
-        $adapter = new ReflectionNamedTypeAdapter($reflectionStub);
+        $adapter = new ReflectionNamedTypeAdapter($reflectionStub, false);
         $adapter->{$methodName}(...$args);
     }
 }