Reflection support for type hints and return types

[mbeccati]

A work based off of Sara's https://wiki.php.net/rfc/reflectionparameter.typehint with a twist.

I've changed ReturnTypeAnnotation back to ReturnTypeHint, as this is still what we're calling them.

The new methods are:

• ReflectionFunctionAbstract::hasReturnTypeHint()
• ReflectionFunctionAbstract::getReturnTypeHint()
• ReflectionTypeHint::isInstance()
• ReflectionTypeHint::isScalar()

References:
http://markmail.org/message/meyx6t52opv7xfbo

Btw, not trying to bypass the RFC process, but atm I don't have time to propose one.

[smalyshev]

Please do not propagate the misnomer "type hint". There's nothing of "hint" in type definition. We've made this mistake once, but we should not make it in perpetuity. We should phase out broken terminology instead.

[Kubo2]

@smalyshev:

So how would you call it then? The thing it accomplishes isn't actually done by presence of an annotation in code (or do I understand it wrong?).

[mbeccati]

@smalyshev Sure. However, I thought we settled for "hint" though, as that's the jargon that's been used extensively during the recent Scalar Type Hints civil war ;)

[nikic]

Looks okay with TypeHint changed back to TypeAnnotation. @morrisonlevi Thoughts?

[mbeccati]

@nikic isn't TypeAnnotation a bit too close to Annotations, which aren't supported yet, but something very different?

[nikic]

@mbeccati I don't really think that there's much potential for confusion in that direction. But, maybe just call it ReflectionType? Would also be a bit more general, so it could be reused for e.g. generic type arguments or whatever.

[mbeccati]

@nikic sure. But maybe we should move the discussion to internals and/or resurrect the RFC?

[nikic]

@mbeccati Yeah, feel free to bring it up on internals again. If there is no disagreement over the APIs I hope we can land this without dragging through the whole RFC cycle.

[mbeccati]

@nikic to be honest I tried a couple of times already without gathering any attention. Maybe if it comes from you we have better chances to land it ;)

[morrisonlevi]

If you look at the RFC history I've probably edited it more than Sara has :)

Personally, I don't think we should include isNull, isScalar, etc. Furthermore, there is a proposal out for union types would would basically require a new API again should it pass.

I honestly think our best solution is to ship without reflection support. Having a bad Reflection API is worse than not having one at all in my opinion; I mean this constructively. Our current API is not very good and this PR basically continues down the same path and just adds more methods.

[morrisonlevi]

Some concrete feedback on this PR:

• I don't think isInstance is appropriately named. I'm not sure isObject or isClass are any better. It's somewhat shocking to me that there isn't a well-known term for something that is either a class or an interface.
• Why is there isScalar instead of multiple options, such as isInt, isFloat, etc?
• I'm not sure why you created typehint_reference here; can you explain that?

[nikic]

@morrisonlevi I don't think shipping without reflection is an option. I've already hit an issue with a mocking framework generating invalid code because it does not respect scalar and return typehints (due to lack of reflection support for them). Reflection is not just cosmetics.

Furthermore, I do not see why this would be incompatible with union types. For them isInstance(), isArray(), etc return false and isUnion() returns true - and getUnionTypes() or whatever returns an array of ReflectionTypes representing the individual members in the union. Similar handling is possible for generics.

[mbeccati]

@morrisonlevi:

• following your reasoning, then instanceof is not appropriately named either.
• (string)$reflectionType already returns "int", "string", etc... why would we need specific isWhatever methods?
• I've reused Sara's code and expanded from there, so I can't explain that myself.

[nikic]

@mbeccati Regarding the second point, you are of course right. However this begs the question why there are isArray() and isCallable() methods as these can also be checked by comparing the type string. Maybe we should drop those two and stick with just __toString(), isNullable() and isInstance()?

class ReflectionType {
    function __toString() : string;
    function isNullable() : bool;
    function isInstance() : bool;
}

[nikic]

After further discussion, the isInstance() method could be problematic if we introduce an object type in the future. If isInstance() is used to distinguish between "special" types and class/interface types, you wouldn't want it to return true for object - but that would be weird given it's name.

As such the suggestion was to call it isClassOrInterface() instead - we don't need to save characters, better be explicit.

[nikic]

So, to summarize the current room 11 consensus:

class ReflectionType {
    function __toString(): string;
    function isClassOrInterface(): bool;
    function allowsNull(): bool;
}

Another topic that came up is what __toString() returns for hints against self and parent. Your current implementation probably returns them directly, while it would likely be better to return the resolved names. Otherwise people will likely just have to manually resolve them. Furthermore this would allow us to resolve these names at compile-time in the future.

[nikic]

Should use EMPTY_SWITCH_DEFAULT_CASE() here instead of the E_ERROR to assert that the switch enumerates all options.

[mbeccati]

@nikic I've fixed internal class support and EMPTY_SWITCH_DEFAULT_CASE().
@morrisonlevi now there's a reason to have a typehint_reference (we need both arg_info and fptr)

Re: room11 consensus, is that a thing? Internals or it didn't happen ;)

Seriously though that seems quite a big deviation from the original RFC, but I'll be happy to make the required changes if there's consensus also outside room11. About self and parent, I'm not sure. To be honest I don't actually like __toString() being the main way to get the type name; I also feel that it'd be useful to have a way to get both self/parent and the acual resolved class name if required.

[morrisonlevi]

To clarify the Room 11 consensus: that includes Nikita Popov, Anthony Ferrara, myself and possibly Bob Weinand (can't remember if he agreed it was good or not; sorry if I'm putting words in your mouth, Bob). Github was not allowing me to post comments at the time, anyway.

The main intention of what Nikita has shared here is that it provides the baseline functionality without getting in the way of future changes or repeating past mistakes. Keep in mind this minimalism is probably required to get this included in PHP 7.0, given that we are after the feature freeze.

[mbeccati]

@nikic @morrisonlevi I've updated the PR. For now I've added a specific test for self/parent to ensure they are returned as-is. Maybe we could add getReflectionClass() that returns the resolved class/interface.

[morrisonlevi]

Just do new ReflectionClass((string) $type); no need for getReflectionClass(). That's my $0.02, anyway.

I only skimmed it, but the patch looks good.

[mbeccati]

@morrisonlevi so you don't think there is a case for keeping self/parent. Well, in that case:

mbeccati@4d6d4f3

[morrisonlevi]

I don't think we currently preserve parent and self on parameter types, do we?

Edit: It seems we do; casting a ReflectionParameter results in something like this:

Parameter #0 [ self $a ]

I will try talking to a few others tomorrow and see what everyone thinks. Shooting from the hip I think I'd prefer to not disclose self or parent at all, instead fully expanding out the type name.

[mbeccati]

@morrisonlevi TBH I wasn't even aware that self/parent were legal parameter type "hints". In any case I've added some tests to cover them as well (to both branches).

[mbeccati]

@morrisonlevi any further feedback?

[nikic]

And I concur with that proposal ;) For the record, the idea behind this is as follows:

By adding something like isClassOrInterface() we're pretty much hardcoding against the current situation. If we add enums, that method name is broken. isClassLike(), while better, still has fundamentally the same issue. If we introduce some kind of type alias syntax like type iterable = array|Traversable then isClassLike() would be wrong as well. In the end the only thing we can distinguish is whether something is a special builtin type or if it isn't. Everything else we do not know statically.

The choice of isBuiltin() over isPrimitive() is that our docs have a pretty clear definition of what a primitive type is and stuff like callable or void doesn't fit in that category.

[mbeccati]

@nikic @morrisonlevi Updated and rebased, even though it looks like github master is out-of-sync atm.

In any case, please do not merge this one, I can manually merge https://github.com/mbeccati/php-src/tree/reflection-typehints-sth-v6 which has @sgolemon 's original implementation plus a squashed commit of everything I did on top of her work.

[morrisonlevi]

You can squash these and force-push to this branch; github will handle it properly. I doubt that anyone who has this branched checked out will care (or should be able to figure out how to get the latest).

[morrisonlevi]

I would also recommend renaming ext/reflection/tests/parameters_typehint.phpt to ext/reflection/tests/parameters_types.phpt.

[mbeccati]

@morrisonlevi sure, I didn't do it right away to make your review easier. It's squashed now and the test is called ReflectionType_001.phpt, to match the existing ones.

[nikic]

There is no need to differentiate between internal/userland here (and in getType), as the structures are compatible (by design). So it's okay to directly check param->arg_info->type_hint.

[mbeccati]

@nikic fixed leak and all tests are now passing. Tbh I haven't digged into the trampoline stuff too much, but

mbeccati@0e7821c#diff-39e3ad3b3135c65cc314ca09cc835530L2989

was required to properly pick the class names. However the same check is missing from most of the similar snippets in other parts of the file, so I'm wondering if I did something wrong.

[nikic]

Hm, I hadn't thought of the problem with the class name for via-handler functions. The check you added will not generally work, because the function __invoke proxies to could also be an internal function (if created via getClosure). Not sure how to fix that just now.

[nikic]

@dstogov What do you think about this problem? Basically the comment in http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_closures.c#210 is not true as far as Reflection is concerned. Do we need an extra flag to distinguish whether it's user or internal arginfo or do you see some other way to solve this?

[nikic]

As far as I'm concerned you can merge this PR -- the via-handler issue is a tangential problem we can resolve later.

[Majkl578]

I think there should be a method to get the name of the type, without explicit string cast, like ReflectionType::getName(). (Or did I just miss it?)

Also ReflectionType::getClass() : ReflectionClass might be very handy. (Similar to ReflectionParameter::getClass()).

[mbeccati]

@nikic will do in the next few days.

@Majkl578 TBH, I find the requirement to use a string cast a bit weird too. But that's how the RFC worked and I wanted to deviate from it as little as possible.

[morrisonlevi]

Just a reminder: this needs to be a minimal set of functionality for 7.0
that can be expanded and built onto later. It is intentionally small so as
to not interfere with other features such as Enums or Union types which may
not have a simple string name or ReflectionClass.

On Sun, Jun 7, 2015, 3:58 PM Matteo Beccati notifications@github.com
wrote:

@nikic https://github.com/nikic will do in the next few days.

@Majkl578 https://github.com/Majkl578 TBH, I find the requirement to
use a string cast a bit weird too. But that's how the RFC worked and I
wanted to deviate from it as little as possible.

—
Reply to this email directly or view it on GitHub
#1190 (comment).

[mbeccati]

Merged in 11ece75