Everything

php

Reflection changes due to allow null + false as standalone types RFC

php.internals

Girgias

4 years ago

Hello internals,

During the review of the implementation of the RFC which introduces null
and false as standalone types there has been a point raised about the
changes made to reflection. [1]

The current implementation is what follows the RFC, namely to make
false|null return a ReflectionUnionType instead of a ReflectionNamedType.
Because of this, I made the rendering of this type union to be false|null
instead of ?false.

Moreover, the question is if this union type should not be consistent with
other types and produce a ReflectionNamedType, and have all of the nullable
types move to ReflectionUnionType at the same time.

I don't mind either way, but as this was voted part of the RFC I feel this
should be discussed by internals to see if changing the Reflection
semantics to not align with the RFC is okay.

As there are currently the following options:
- Keep as the RFC stated with some minor implementation complexity which
will be resolved when the Reflection changes are made to all other nullable
types.
- Ignore the Reflection changes of the RFC and align the union type with
the current behaviour
- Break BC and change all nullable types to return a ReflectionUnionType

(I just added the last option for the sake of completeness, but don't think
breaking BC is the way to go here).

Best regards,

George P. Banyard

[1] https://github.com/php/php-src/pull/7546#discussion_r837900447

Marco Pivetta

4 years ago

Probably best with consistency?

> - Ignore the Reflection changes of the RFC and align the union type with

the current behaviour

This one, specifically.

I'd love to see all nullable types become
`ReflectionUnionType(ReflectionNamedType($t), ReflectionNamedType(null))`,
but that would be a BC break for later on :-)

Marco Pivetta

https://twitter.com/Ocramius

https://ocramius.github.io/

On Tue, 5 Apr 2022 at 13:58, G. P. B. <george.banyard@gmail.com> wrote:

Guilliam Xavier

4 years ago

Hi George,

> https://github.com/php/php-src/pull/7546#discussion_r837900447

Thanks for asking (even if voted).

On Tue, Apr 5, 2022 at 2:01 PM Marco Pivetta <ocramius@gmail.com> wrote:

> Probably best with consistency?
>
>  > - Ignore the Reflection changes of the RFC and align the union type with
> the current behaviour
>
> This one, specifically.
>

I agree with Marco (who I know works on reflection-related projects).

To expand a bit:

when reading the RFC, "the notable exception that `null|false` will produce
a ReflectionUnionType instead of a ReflectionNamedType contrary to other
`null|T` types" seemed legit because
- `T|false` currently always produces a ReflectionUnionType
- `null|T` producing a ReflectionNamedType (like `?T`) was only decided
*for backwards-compatibility reasons* [
https://wiki.php.net/rfc/union_types_v2#reflection]
(moreover the initial RFC [https://wiki.php.net/rfc/null-standalone-type]
only proposed standalone `null`, not `false`, so didn't allow `?false`)

but now looking at the implementation, I don't like the special casing.

Regards,

--
Guilliam Xavier

Girgias

4 years ago

I have just merged the PR with the amended Reflection changes:
https://github.com/php/php-src/commit/6039c07a3afd64a42c9b9f1ed994ca971db67a1e

Best regards,

George P. Banyard