[RFC][Vote] ReflectionType Improvements

if ($type instanceof ReflectionClassType) {
    $r = $type->getClass();
}

The (currently) correct code would be:

if (class_exists($type->getName()) || interface_exists($type->getName())) {
    $r = new ReflectionClass($type->getName());
}

This seems to be combining two checks into one:

• is this a "simple / builtin type" or a "class / interface type"?
• is the class / interface referenced currently defined or autoloadable?

The first question can be answered with the existing
ReflectionType::isBuiltin() method. The fact that the second is
currently difficult to answer doesn't seem to have anything to do with
reflecting type hints.

It seems like if there was a use case for "is this type hint a currently
defined or autoloadable class / interface?" it would ideally be written
something like this:

if ( ! $type->isBuiltin() && object_type_exists($type->getName()) )

The term "object type" being bikesheddable as a way of encompassing your
hypothetical enums or other future "class-like" types. Of course, the
fact that "class_exists" and "ReflectionClass" use different definitions
of "class" already is somewhat awkward...

Or if the aim is to simplify the reflection usage, why require the if
statement at all:

try {
$r = $type->getReflectionClass();
} catch ( ReflectionException $e ) {
// type is builtin or refers to an undefined class
}

Regards,

Rowan Collins
[IMSoP]

On Mon, Jul 4, 2016 at 10:51 AM, Björn Larsson
bjorn.x.larsson@telia.com wrote:

Den 2016-06-30 kl. 23:57, skrev Nikita Popov:

The RFC for improving ReflectionType1 is now in voting phase. The
voting
window is June 30th through July 8th. I have not finished the patch but
I'll have it done before the end of voting.

Replying on-list as multiple people asked:

I'm voting against this RFC, because it introduced not only the
ReflectionNamedType class (which is reasonable), but also the
ReflectionClassType class (which is not).

My main objection to ReflectionClassType is that it is autoloading
dependent (*). Something like getReturnType() on a class type hint will
either return a ReflectionClassType if the class can be loaded, or a
ReflectionNamedType if it can't. I think this is confusing and I'm sure
that this will lead to broken code: For example, people will try to use
"$type instanceof ReflectionClassType" to check whether something is a
class type hint, while the currently correct way, which works
independently
of class loading, is to check isBuiltin() instead.

I don't think that most consumers of ReflectionType are interested in
obtaining a ReflectionClass for the type hint anyway (which is the only
functionality that ReflectionClassType provides), and if necessary this
can
be easily done in userland. There is no need to over-complicate the
ReflectionType functionality in this manner, especially with the proposed
semantics.

Nikita

Maybe one should split the vote into separate for each function.
I mean pity if vote fails because one function is not attractive while
the other one is...

What do you think?

Regards //Björn Larsson

I think that it's disappointing that I received no feedback on this
RFC at all and yet have this ratio of votes in the negative.

I have not yet voted, but I tend to be -1 on this RFC.

I'll take this opportunity to share a portion of a conversation I had
with Nikita that demonstrates why ReflectionClassType is actually
useful:

Nikita: But does it really help them? I don't see a lot of differences between these two:
if ($type instanceof ReflectionClassType) {
$r = $type->getClass();
}
// and
if (class_exists($type->getName())) {
$r = new ReflectionClass($type->getName());
}
Levi: ...[I]t will fail for interfaces.] ReflectionClass can be built for those but class_exists() will be false.
Nikita: True :)

The (currently) correct code would be:

if (class_exists($type->getName()) || interface_exists($type->getName())) {
    $r = new ReflectionClass($type->getName());
}

ACK. If this code is regarded as too verbose, a function
class_or_interface_exists() (or something like that) can easily be
defined in userland. Anyhow, using (class|interface)_exists() offers
the option to use autoloading or not, contrary to the RFC which always
uses autoloading, what might not be desired.

But if we add some other type such as enums we may have to add more
conditions. As long as ReflectionClass can be made from the type then
doing it in the engine is fully forward compatible.

Iff we add some other type, we can always reconsider to add another
subclass. In my opinion, it doesn't make sense to introduce complexity
(albeit minor in this case) to cater to potential future changes.

Another (minor) issue I have with the RFC is the introduction of
ReflectionNamedType. A simpler alternative appears to be to add the
getName() method to ReflectionType; for unnamed types this could simply
return (string) $reflectionType. That might not be the most farsighted
design decision, though.

--
Christoph M. Becker

On Mon, Jul 4, 2016 at 10:51 AM, Björn Larsson
bjorn.x.larsson@telia.com wrote:

Den 2016-06-30 kl. 23:57, skrev Nikita Popov:

The RFC for improving ReflectionType1 is now in voting phase. The
voting
window is June 30th through July 8th. I have not finished the patch but
I'll have it done before the end of voting.

Replying on-list as multiple people asked:

I'm voting against this RFC, because it introduced not only the
ReflectionNamedType class (which is reasonable), but also the
ReflectionClassType class (which is not).

My main objection to ReflectionClassType is that it is autoloading
dependent (*). Something like getReturnType() on a class type hint will
either return a ReflectionClassType if the class can be loaded, or a
ReflectionNamedType if it can't. I think this is confusing and I'm sure
that this will lead to broken code: For example, people will try to use
"$type instanceof ReflectionClassType" to check whether something is a
class type hint, while the currently correct way, which works
independently
of class loading, is to check isBuiltin() instead.

I don't think that most consumers of ReflectionType are interested in
obtaining a ReflectionClass for the type hint anyway (which is the only
functionality that ReflectionClassType provides), and if necessary this
can
be easily done in userland. There is no need to over-complicate the
ReflectionType functionality in this manner, especially with the
proposed
semantics.

Nikita

Maybe one should split the vote into separate for each function.
I mean pity if vote fails because one function is not attractive while
the other one is...

What do you think?

Regards //Björn Larsson

I think that it's disappointing that I received no feedback on this
RFC at all and yet have this ratio of votes in the negative.

I'll take this opportunity to share a portion of a conversation I had
with Nikita that demonstrates why ReflectionClassType is actually
useful:

Nikita: But does it really help them? I don't see a lot of differences
between these two:
if ($type instanceof ReflectionClassType) {
$r = $type->getClass();
}
// and
if (class_exists($type->getName())) {
$r = new ReflectionClass($type->getName());
}
Levi: ...[I]t will fail for interfaces.] ReflectionClass can be built
for those but class_exists() will be false.
Nikita: True :)

The (currently) correct code would be:

if (class_exists($type->getName()) ||

interface_exists($type->getName())) {
$r = new ReflectionClass($type->getName());
}

But if we add some other type such as enums we may have to add more
conditions. As long as ReflectionClass can be made from the type then
doing it in the engine is fully forward compatible.

I hope people who voted no will share why they have done so, but I
also hope they'll switch to yes if they voted no because they don't
see it as being useful.

To add to this, another issue we discussed OTR but didn't really come to a
conclusion on, is the handling of self and parent type hints by
ReflectionClassType.

On the one hand, ReflectionClassType can be advantageous here, because it
can resolve self/parent itself and return the appropriate ReflectionClass.
Otherwise the user would have to do this themselves (if getting a
ReflectionClass is a goal).

On the other hand, self/parent are not always statically resolvable. For
methods in traits self does not refer to the trait, but rather to the class
the trait will be used in. This means that when reflecting on a self type
on a trait you wouldn't get a ReflectionClassType (for something that is
clearly a class type hint). A similar issue also exists for self/parent on
closures.

Regards,
Nikita

Hi Levi,

Additionally, I propose adding a getName() method to ReflectionType that returns only the name of the type, regardless of nullability. Casting should not be required to get information from an object, but currently this is the only way to get the type name from ReflectionType. Most other reflection classes include a getName() method, this seems to have been an oversight.

This wasn't an oversight. If we add union or intersection types then
not all types are named (for instance ArrayAccess & Countable & Traversable is not a named type). This is why it doesn't exist on the
base ReflectionType.

Good point, then I agree getName() should be in an extending class as in the RFC.

I have surveyed some of the people who have voted no. Their reasons
vary but based on these conversations it seems to me that by dropping
ReflectionClassType and the associated autoloading mechanism that
overall we'd be happier. I do agree with Aaron that at least some
changes really need to go into 7.1. How do people feel about my
proposal to just drop autoloading and ReflectionClassType?

This sounds reasonable to me.

Aaron Piotrowski