Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType` #902

Ocramius · 2021-12-05T08:07:42Z

Fixes #901

Context

Currently, when running reflection on an A|null type, BetterReflection produces a Roave\BetterReflection\Reflection\ReflectionUnionType:

var_dump(
    get_class(
        (new DefaultReflector(new StringSourceLocator(
            <<<'PHP'
<?php
interface A {}
final class AClass {
    private A|null $typed;
}
PHP
            ,
            (new BetterReflection())->astLocator()
        )))
            ->reflectClass('AClass')
            ->getProperty('typed')
            ->getType()
    )
);

produces

string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType"

In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3

<?php

interface A {}
interface B {}

class Implementation
{
    function foo(A|null $param) { throw new Exception(); }
    function bar(A|B|null $param) { throw new Exception(); }
}

var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType());
var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType());

produces:

object(ReflectionNamedType)#2 (0) {
}
object(ReflectionUnionType)#1 (0) {
}

This means that a UnionType AST node composed of just null plus another type should be converted into
a ReflectionNamedType, for the sake of compatibility with upstream (this patch does that).

This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )

Ocramius · 2021-12-05T08:13:37Z

This makes a few tests fail, because of obvious translation details:

There were 4 failures:

1) Roave\BetterReflectionTest\SourceLocator\SourceStubber\PhpStormStubsSourceStubberTest::testFunctionInPhpVersion with data set #15 ('dom_import_simplexml', 70000, true, 'DOMElement|null')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'DOMElement|null'
+'?DOMElement'

/home/runner/work/BetterReflection/BetterReflection/test/unit/SourceLocator/SourceStubber/PhpStormStubsSourceStubberTest.php:933

2) Roave\BetterReflectionTest\SourceLocator\SourceStubber\PhpStormStubsSourceStubberTest::testPropertyInPhpVersion with data set #4 ('PDOException', 'errorInfo', 80100, true, 'array|null')
PDOException::$errorInfo
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'array|null'
+'?array'

/home/runner/work/BetterReflection/BetterReflection/test/unit/SourceLocator/SourceStubber/PhpStormStubsSourceStubberTest.php:889

3) Roave\BetterReflectionTest\SourceLocator\SourceStubber\PhpStormStubsSourceStubberTest::testPropertyInPhpVersion with data set #8 ('DOMNode', 'parentNode', 80100, true, 'DOMNode|null')
DOMNode::$parentNode
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'DOMNode|null'
+'?DOMNode'

/home/runner/work/BetterReflection/BetterReflection/test/unit/SourceLocator/SourceStubber/PhpStormStubsSourceStubberTest.php:889

4) Roave\BetterReflectionTest\SourceLocator\SourceStubber\PhpStormStubsSourceStubberTest::testFunctionParameterInPhpVersion with data set #3 ('bcscale', 'scale', 80000, true, 'int|null', true)
bcscale::$scale
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'int|null'
+'?int'

I fear this inconsistency between php-src pre-union-type and post-union-types will be problematic long-term...

Ocramius · 2021-12-05T08:24:08Z

TBH, I really wish this was the complete opposite, and that nullable ReflectionNamedType didn't exist :|

Ocramius · 2021-12-05T08:34:30Z

Ok, looking at this from the perspective of Roave/BackwardCompatibilityCheck#324, and I think it would be simpler to never have a mix of union and nullable type: only ever have union types, then change this behavior in the Adapter/ layer.

@kukulich WDYT?

src/Reflection/ReflectionType.php

kukulich · 2021-12-05T08:50:27Z

@Ocramius Yes, I don't like ReflectionNamedType with allowsNull as well. I expect other tools like PHPStan has to deal with both variants anyway so it should be probably ok when BR returns only union types. Do I have right @ondrejmirtes ?

Ocramius · 2021-12-05T08:55:28Z

I'm currently working on:

making this consistent in roave/better-reflection internals
making sure the /Adapter layer still operates as it would in php-src

…tionNamedType` Fixes #901 ## Context Currently, when running reflection on an `A|null` type, BetterReflection produces a `Roave\BetterReflection\Reflection\ReflectionUnionType`: ```php var_dump( get_class( (new DefaultReflector(new StringSourceLocator( <<<'PHP' <?php interface A {} final class AClass { private A|null $typed; } PHP , (new BetterReflection())->astLocator() ))) ->reflectClass('AClass') ->getProperty('typed') ->getType() ) ); ``` produces ``` string(53) "Roave\BetterReflection\Reflection\ReflectionUnionType" ``` In PHP-SRC, this behavior is different: https://3v4l.org/gMA4T#v8.1rc3 ```php <?php interface A {} interface B {} class Implementation { function foo(A|null $param) { throw new Exception(); } function bar(A|B|null $param) { throw new Exception(); } } var_dump((new ReflectionParameter([Implementation::class, 'foo'], 0))->getType()); var_dump((new ReflectionParameter([Implementation::class, 'bar'], 0))->getType()); ``` produces: ``` object(ReflectionNamedType)#2 (0) { } object(ReflectionUnionType)#1 (0) { } ``` This means that a `UnionType` AST node composed of just `null` plus another type should be converted into a `ReflectionNamedType`, for the sake of compatibility with upstream (this patch does that). This is ugly, but will (for now) avoid some bad issues in downstream handling (presently blocking Roave/BackwardCompatibilityCheck#324 )

…justing expectations in stubber tests

…on types When an union type of `T|null` or `?T` is met, BetterReflection will (from now on) keep a `ReflectionUnionType` internally, while exposing a `ReflectionNamedType` with `ReflectionNamedType#allowsNull() === true` only at adapter level. While this is a BC break, it leads to a much cleaner API around handling `null` types, and inspecting types for type analysis. Ref: #902 (comment) Ref: Roave/BackwardCompatibilityCheck#324

Ocramius · 2021-12-05T10:09:33Z

@kukulich I've now rewritten the patch:

ReflectionUnionType is kept in internals, when ?T and T|null are met (no longer produces a ReflectionNamedType)
ReflectionNamedType is exposed at the adapter level, to make sure we stay compatible with the reflection API

…on suffices This test was breaking because `Roave\BetterReflection\Reflection\ReflectionUnionType::isBuiltin()` was being called: such a method does not exist though, because it only exists on `ReflectionNamedType`. That's because the test is designed without type inheritance in mind: removing the assertions is sufficient here.

…P 8.1 through the adapter layer (for compatibility) Nullable types in BetterReflection are represented as union types: using the adapter layer as part of the test, even if just for a small string cast, helps us avoiding custom code here.

This change strictly depends on Roave/BetterReflection#902, and once that's merged, new `infection/infection` mutations should appear, showing us code that can safely be removed.

kukulich · 2021-12-05T10:38:49Z

LGTM.

Just one idea: Shouldn't we normalize/sort types in ReflectionUnionType to make sure we always have consistent output, eg. null as the last type of union? However, not probably important.

Ocramius · 2021-12-05T10:42:24Z

@kukulich probably, but my batteries for this are depleted for today :D

Core seems to be doing it: https://3v4l.org/jcq9S

Ocramius · 2021-12-05T10:43:16Z

Thanks for the review, @kukulich!

…ve/BetterReflection#902

drupol · 2021-12-05T13:27:35Z

LGTM.

Just one idea: Shouldn't we normalize/sort types in ReflectionUnionType to make sure we always have consistent output, eg. null as the last type of union? However, not probably important.

I know that phpcsfixer has a rule for that, here it is: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/doc/rules/phpdoc/phpdoc_types_order.rst

This change strictly depends on Roave/BetterReflection#902, and once that's merged, new `infection/infection` mutations should appear, showing us code that can safely be removed.

…ve/BetterReflection#902

Ocramius added the bug label Dec 5, 2021

Ocramius added this to the 5.0.0 milestone Dec 5, 2021

Ocramius mentioned this pull request Dec 5, 2021

PHP 8.0 and PHP 8.1 support, BetterReflection 5.x upgrade Roave/BackwardCompatibilityCheck#324

Merged

7 tasks

Ocramius commented Dec 5, 2021

View reviewed changes

src/Reflection/ReflectionType.php Outdated Show resolved Hide resolved

Ocramius added 3 commits December 5, 2021 11:08

We now convert UnionType([T, int]) to ReflectionNamedProperty: ad…

0d78168

…justing expectations in stubber tests

Ocramius force-pushed the feature/#901-ensure-that-simple-union-types-are-turned-into-nullable-reflection-named-types branch from f86963a to 14d3d5a Compare December 5, 2021 10:08

Ocramius added 2 commits December 5, 2021 11:17

Ocramius force-pushed the feature/#901-ensure-that-simple-union-types-are-turned-into-nullable-reflection-named-types branch from b4c0c02 to 33a5645 Compare December 5, 2021 10:19

Ocramius requested a review from kukulich December 5, 2021 10:29

Ocramius self-assigned this Dec 5, 2021

Ocramius merged commit 2b64de3 into 5.0.x Dec 5, 2021

Ocramius deleted the feature/#901-ensure-that-simple-union-types-are-turned-into-nullable-reflection-named-types branch December 5, 2021 10:43

Ocramius added a commit to Roave/BackwardCompatibilityCheck that referenced this pull request Dec 5, 2021

Upgraded to latest roave/better-reflection:5.0.x-dev to include Roa…

19580dd

…ve/BetterReflection#902

uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this pull request Aug 6, 2024

Upgraded to latest roave/better-reflection:5.0.x-dev to include Roa…

3d31999

…ve/BetterReflection#902

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType` #902

Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType` #902

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021 •

edited

Loading

kukulich commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

kukulich commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

drupol commented Dec 5, 2021 •

edited

Loading

Ensure that simple union types like A|null yield a nullable ReflectionNamedType #902

Ensure that simple union types like A|null yield a nullable ReflectionNamedType #902

Conversation

Ocramius commented Dec 5, 2021

Context

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021 • edited Loading

kukulich commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

kukulich commented Dec 5, 2021

Ocramius commented Dec 5, 2021

Ocramius commented Dec 5, 2021

drupol commented Dec 5, 2021 • edited Loading

Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType` #902

Ensure that simple union types like `A|null` yield a nullable `ReflectionNamedType` #902

Ocramius commented Dec 5, 2021 •

edited

Loading

drupol commented Dec 5, 2021 •

edited

Loading