On Sun, Aug 21, 2016 at 5:46 PM, Levi Morrison <le...@php.net> wrote:
>
>
> On Sun, Aug 21, 2016 at 9:42 AM, Marco Pivetta <ocram...@gmail.com> wrote:
>
>> Including the question mark still breaks code-gen based on simple
>> nullable types. PHP 7.0.x-compliant code will do following:
>>
>> - reflect function foo(Bar $bar = null) {}
>> - cast ReflectionType to string
>> - crash (unrecognized `?` symbol)
>>
>> That's part of what I tested on Friday on just one of the many libraries
>> involved.
>>
>> The new API already includes `ReflectionType#allowsNull()`, so that's
>> sufficient to build forward-compliant codegen. Breaking BC is a no-go
>> though.
>>
>> Marco Pivetta
>>
>> http://twitter.com/Ocramius
>>
>> http://ocramius.github.com/
>>
>> On Sun, Aug 21, 2016 at 5:39 PM, Levi Morrison <le...@php.net> wrote:
>>
>>>
>>>
>>> On Sun, Aug 21, 2016 at 9:07 AM, Levi Morrison <le...@php.net> wrote:
>>>
>>>>
>>>>
>>>> On Sun, Aug 21, 2016 at 8:37 AM, Aaron Piotrowski <aa...@trowski.com>
>>>> wrote:
>>>>
>>>>> Hi Marco,
>>>>>
>>>>> > On Aug 19, 2016, at 1:31 PM, Marco Pivetta <ocram...@gmail.com>
>>>>> wrote:
>>>>> >
>>>>> > Hi Aaron et all,
>>>>> >
>>>>> > I tried to implement support for 7.1 in zend-code as a start:
>>>>> >
>>>>> > https://github.com/zendframework/zend-code/pull/87
>>>>> >
>>>>> > A few issues arise:
>>>>> >
>>>>> > * `ReflectionType#__toString()` is too volatile, especially if we
>>>>> want to
>>>>> > support multiple versions of PHP, therefore it's a good idea to not
>>>>> think
>>>>> > too much about it, and instead deprecate it. Most issues I had while
>>>>> > working with the feature were related with string formatting, and
>>>>> that's
>>>>> > simply gotta die: just using a more specific API should cut it
>>>>> (getName,
>>>>> > getClass, isNullable, etc. As few strings as possible, please!).
>>>>> > * A page where we can see the current state of the `ReflectionType`
>>>>> API
>>>>> > (and its subtypes) would be golden.
>>>>> > * `ReflectionType#__toString()` seems to crash in very interesting
>>>>> ways
>>>>> > when `?string` is reflected (see issue above - couldn't isolate
>>>>> precisely)
>>>>> >
>>>>>
>>>>> I've reverted the changes so that `ReflectionType::__toString()` is
>>>>> now identical to 7.0, including *not* prepending a ? for nullable types.
>>>>> The method is now just an alias of `ReflectionNamedType::getName()`.
>>>>>
>>>>> `ReflectionType::__toString()` should be discouraged for code
>>>>> generation going forward, as it seems there's just not a way to add type
>>>>> features in a BC way. My attempt to incorporate nullable types in a way
>>>>> that would allow for even more complex types such as
>>>>> `callable(?\Type\Name,
>>>>> ?bool)` just caused too many problems.
>>>>>
>>>>> > I am currently going through the changes, and just figured that 7.1
>>>>> implements https://wiki.php.net/rfc/reflectiontypeimprovements, even
>>>>> though the RFC was declined:
>>>>> >
>>>>> > ./sapi/cli/php -r 'class Foo { public function bar() : ?Foo {} }
>>>>> var_dump((new ReflectionMethod("Foo", "bar"))->getReturnType());'
>>>>> > object(ReflectionNamedType)#2 (0) {
>>>>> > }
>>>>>
>>>>> Only `ReflectionNamedType` was added so the object returned from
>>>>> parameter and return types could have a `getName()` method. The rest of
>>>>> the
>>>>> RFC was not implemented. This should be completely BC while allowing
>>>>> future
>>>>> types like unions or callables. See some discussion here:
>>>>> https://github.com/php/php-src/pull/2068
>>>>>
>>>>> Aaron Piotrowski
>>>>>
>>>>>
>>>>> --
>>>>> PHP Internals - PHP Runtime Development Mailing List
>>>>> To unsubscribe, visit: http://www.php.net/unsub.php
>>>>>
>>>>
>>>> This is too big of a revert. You must attempt to generate exactly what
>>>> was written in user code which requires the prepended question mark.
>>>>
>>>
>>> To clarify we do resolve certain things such as names which are aliased
>>> by use and the like. But we can't resolve all names such as self and parent
>>> because of traits. The use of toString is to dump out the type information
>>> in a string that would be suitable for regenerating that exact type
>>> declaration only. It is not meant to be parsed or analyzed. It was designed
>>> this way from the beginning, and any deviation of this is a misuse. By not
>>> prepending the ? we will break other libs, so it's a BC break either way.
>>> Please put the question mark back as discussed in the beginning. Do not
>>> include the leading slash.
>>>
>>
>>
> If you are on 7.1 and you generate code for an earlier version of PHP
> that's not really a break...
>
If you install a library on 7.1 and it stops working for existing
7.0-compliant code (you just want to run the same exact code, but on 7.1),
then this is a break.
>
> This is the ultimate point: if we don't prepend the question mark it will
> break code.
>
This is the impedence here: `ReflectionType` is a *REFLECTION*, not a code
generator. Don't think of `ReflectionType#__toString()` as a way to
generate code: it is merely a string representation of the reflectoion.
Current code generators don't use that type directly anyway: they read it,
pass it through a dozen layers that do whatever they need to do, and then
it may come out on the other side in whatever shape fits the generator
most. The original string representation is not used in the output
representation anyway.
> If do prepend the question mark it will break code. It's a BC break
> whether it is done or not.
>
It won't break BC for existing code, but codegen for nullable/void needs
adaptation. This was already underway on most code generators I work with,
and this happens at every PHP version, and it is *OK*. New features => new
codegen/changes, this is normal/understood/accepted.
> So what to do? How to pick which is correct? In my opinion we should
> preserve the original intention of the code. Thus we should prepend the
> question mark.
>
Take php 7 code, make sure it works on 7.1 as it did before.
Take php 7.1 code => requires library adaptation for forward compliance.
The confusion here is between BC and FC.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
