Hi Internals,
I've just finished the first iteration of the RFC (
https://wiki.php.net/rfc/internal_method_return_types) as well as the
implementation based on the feedback Nikita and Nicolas provided. Thus, I'd
like to start the discussion phase.
Regards:
Máté
Hello Maté,
https://wiki.php.net/rfc/internal_method_return_types) as well as the
implementation based on the feedback Nikita and Nicolas provided.
Thanks for moving this topic forward, it's very much needed.
I like the word "tentative" you're using to describe the missing return
types.
I would be good with the RFC but I want to give a few different ideas to
challenge it a bit.
Here they are:
- About using "E_STRICT", I'm not really sold. I think these notices
should follow the same path as deprecations as far as reporting is
concerned. By using E_STRICT, many error handlers will throw exceptions,
while the code is fine really. Deprecations don't lead to exceptions
usually (at least they never do in any Symfony stack). Semantically, a
deprecation fits well to me also. - Suppressing the notice is very much needed, but I think we can merge
the corresponding attribute with the TentativeReturnType that you mention
in "future scope". - As a corollary, we might not need the new methods on ReflectionMethod:
instead, we might make getAttributes() return a TentativeReturnType
attribute for internal methods too.
To summarize my proposal, I think we need only one TentativeReturnType
attribute, that we would put on internal methods, and that would also be
allowed on userland methods. ReflectionMethod::gettAttributes() would
report these attributes for both internal and userland methods.
The attribute would need to convey the allowed types. This could be done
with a list of strings. Taking your example from the RFC, this would look
like this:
class MyDateTime extends DateTime{
/**
* @return DateTime|false
*/
#[TentativeReturnType(DateTime::class, 'false')] public
function modify(string $modifier) { return false; }}
In this example, this would duplicate the docblock, but many docblocks
contain extended return-type info that the engine doesn't understand
anyway (list of types, etc.)
When engine sees these attributes, it would behave as you describe in
the RFC, with one possible extension: it could also validate the
supplied list of types and check LSP on it (when there is a parent
type).
Does this make sense to you?
Nicolas
Le 6 mars 2021 à 23:56, Máté Kocsis kocsismate90@gmail.com a écrit :
Hi Internals,
I've just finished the first iteration of the RFC (
https://wiki.php.net/rfc/internal_method_return_types) as well as the
implementation based on the feedback Nikita and Nicolas provided. Thus, I'd
like to start the discussion phase.Regards:
Máté
Hi,
Two remarks:
(1) The RFC is about return type of non-final methods. The same issue might arise for typed properties, whose type are supposed to be invariant under inheritance; although it is admittedly rarely useful to redeclare them in the subclass. Should typed properties be handled in the same way, or should we simply recommend to not redeclare properties?
(2) Nicolas Grekas has beaten about E_STRICT (E_DEPRECATED is fine in this case). I’m just adding this: By reintroducing E_STRICT you are effectively reverting the Reclassify E_STRICT notices RFC, whose motivation was to “simplify our error model and resolve the currently unclear role of strict standards notices”. Thus, you should propose a clear role of those resurrected E_STRICT notices, that justifies the complication of the error model.
—Claude
Hi Nicolas and Claude,
First of all, thanks for your comments! Let me answer them one by one:
1.) A few other people off-list also shared their negative feelings about
the E_STRICT. So I'm ok to use E_DEPRECATED instead.
Now that I implemented the suppressing mechanism, I think using a separate
error type is not needed anymore that much.
2.) Unifying the TentativeReturnType and SuppressReturnTypeNotice
attributes is an interesting idea, and I would love to get
rid of the latter one. Although, my concern is that doing so would
eliminate the possibility for an overriding method to declare
its own native return types (it's possible that they've already defined
return types): so any child class could literally define
return types at its own will, while it was not possible to do before. I'm
eager to listen to any solution that would address my concerns. :)
3.) I'm not sure you noticed in the PR, but in fact, I also implemented
the TentativeReturnType attribute. Currently, it can be used
as below when DateTime::modify() is overridden to return the ?DateTime type:
class MyDateTime extends DateTime
{
#[TentativeReturnType]
public function modify(string $modifier): ?DateTime { return null; }
}
How does this syntax look like for you?
Compared to your proposal, the main difference is that the attribute
doesn't itself store any type information, but it's done among the
function information as normally. In my opinion, the current implementation
has multiple advantages over the one you proposed:
- As far as I can tell, it would be very cumbersome to parse and validate
type information from strings, rather than reusing the
capabilities we already have. But I'd be happy to be corrected if I'm wrong. - Apart from the compiler itself, I think parsing type info is more
straightforward as currently implemented for users as well as any tooling - We should also think about people who already declared return types for
overriding methods. I don't see any good reason why they
should repeat this information in the attribute. - I'm also not sure how the attribute could be used to retrieve the
tentative type? Should it return a ReflectionType? Returning
just the string representation seems like subideal for me.
4.) Claude, I think you make a good point! So non-private, untyped
properties could be converted to typed properties a little bit more
gradually
if we made this "tentative" type mechanism available for properties as
well. Coincidentally, I recently came up with the idea that I'd like to fix
type
issues with internal class properties (e.g. lots of them don't respect the
strict_types directive), and I also thought about migrating them to typed
properties for PHP 9.0. All in all, I don't want to include properties in
the current proposal, but I'd probably work on it next time.
Regards:
Máté
Claude Pache claude.pache@gmail.com ezt írta (időpont: 2021. márc. 8., H,
16:19):
Le 6 mars 2021 à 23:56, Máté Kocsis kocsismate90@gmail.com a écrit :
Hi Internals,
I've just finished the first iteration of the RFC (
https://wiki.php.net/rfc/internal_method_return_types) as well as the
implementation based on the feedback Nikita and Nicolas provided. Thus,
I'd
like to start the discussion phase.Regards:
MátéHi,
Two remarks:
(1) The RFC is about return type of non-final methods. The same issue
might arise for typed properties, whose type are supposed to be invariant
under inheritance; although it is admittedly rarely useful to redeclare
them in the subclass. Should typed properties be handled in the same way,
or should we simply recommend to not redeclare properties?(2) Nicolas Grekas has beaten about
E_STRICT(E_DEPRECATED is fine in this
case). I’m just adding this: By reintroducingE_STRICTyou are effectively
reverting the ReclassifyE_STRICTnotices RFC, whose motivation was to
“simplify our error model and resolve the currently unclear role of strict
standards notices”. Thus, you should propose a clear role of those
resurrectedE_STRICTnotices, that justifies the complication of the error
model.—Claude
Hi Máté,
1.) A few other people off-list also shared their negative feelings about
the E_STRICT. So I'm ok to use
E_DEPRECATEDinstead.
Now that I implemented the suppressing mechanism, I think using a separate
error type is not needed anymore that much.
Wonderful!
2.) Unifying the TentativeReturnType and SuppressReturnTypeNotice
attributes is an interesting idea, and I would love to get
rid of the latter one. Although, my concern is that doing so would
eliminate the possibility for an overriding method to declare
its own native return types (it's possible that they've already defined
return types): so any child class could literally define
return types at its own will, while it was not possible to do before. I'm
eager to listen to any solution that would address my concerns. :)
I'm not sure I get your concern: how would #[TentativeReturnType] conflict
with native return types?
I didn't think about it, but #[TentativeReturnType] could be useful to
raise a deprecation when a supertype wants to tell child classes that's is
going to tighten its return type, and that they should do so if theirs is
too broad. This is basically a generalization of what we're talking about,
isn't it?
3.) I'm not sure you noticed in the PR, but in fact, I also implemented
the TentativeReturnType attribute. Currently, it can be used
as below when DateTime::modify() is overridden to return the ?DateTime
type:class MyDateTime extends DateTime
{
#[TentativeReturnType]
public function modify(string $modifier): ?DateTime { return null; }
}How does this syntax look like for you?
Compared to your proposal, the main difference is that the attribute
doesn't itself store any type information, but it's done among the
function information as normally.
Oh, I didn't get this at first: you're telling that this native return type
would not be enforced when the attribute is declared? This should answer my
previous comment. I'm not sure yet this is a good idea, because it would
prevent userland libs from using this attribute in an effective way until
they bump to 8.1...
In my opinion, the current implementation
has multiple advantages over the one you proposed:
- As far as I can tell, it would be very cumbersome to parse and validate
type information from strings, rather than reusing the
capabilities we already have. But I'd be happy to be corrected if I'm
wrong.
- Apart from the compiler itself, I think parsing type info is more
straightforward as currently implemented for users as well as any tooling
Maybe Nikita can shed some light on this (the 1st item mainly)?
- We should also think about people who already declared return types for
overriding methods. I don't see any good reason why they
should repeat this information in the attribute.
In my proposal, the path for userland would be:
- in their next minor (still supporting PHP < 8.1): add the attribute;
- in their following major: user a native return type and remove the
attribute.
- I'm also not sure how the attribute could be used to retrieve the
tentative type? Should it return a ReflectionType? Returning
just the string representation seems like subideal for me.
It would contain a list of strings in my proposal, to keep things boring
and expected yes.
Nicolas
Hi Nicolas,
Oh, I didn't get this at first: you're telling that this native return
type would not be enforced when the attribute is declared? This should
answer my previous comment.
Yes, that's the case. On one hand, this solution IMO has the advantage that
its syntax integrates into the ecosystem much better, including php-src
itself (as I mentioned
in my previous mail). On the other hand, it might be a little bit deceptive
at first that the type declaration is not enforced when the attribute is
present. Nonetheless, I still prefer this solution
over storing types as a list of strings.
I'm not sure yet this is a good idea, because it would prevent userland
libs from using this attribute in an effective way until they bump to 8.1...
The main proposal is about adding return types to internal methods, and the
SuppressReturnTypeNotice attribute can already be added to any libraries.
But you are right in connection with the TentativeReturnType
attribute, libraries would have to bump their minimum PHP version
requirement to 8.1 in order to be able to add return types without a BC
impact on PHP <8.1. I think it's still the less bad option.
As this RFC's primary motivation is to prepare for adding return types to
internal methods, I'm planning to add a secondary vote about exposing
tentative return types to userland, since I'm not 100% confident that this
concept is worth the special care in PHP itself.
Máté
Hi,
I've updated the RFC with the following things:
- The proposed error level is now
E_DEPRECATEDinstead ofE_STRICT - I added a new section for exposing the behavior to userland (this is a
separate vote)
As the 2 weeks of discussion has already passed, I'd like to start the vote
in the near future, unless
any major concern arises. So it's the best time to bring these concerns up.
:)
Máté
I've updated the RFC with the following things:
- The proposed error level is now
E_DEPRECATEDinstead ofE_STRICT- I added a new section for exposing the behavior to userland (this is a
separate vote)
To build on your approach and mixing it with the need we discussed for
userland, I have a new proposal:
We should replace the SuppressReturnTypeNotice by the TentativeReturnType
one right away.
The TentativeReturnType would mean: "I'm going to add a return type to this
method in its next major".
As a consequence, for child classes of internal methods, this would
suppress the notice.
For userland, there are already ways to declare the planned return type
(aka @return in phpdoc), so we might not need any new way to declare this
from the engine pov.
As a corollary, adding this attribute should conflict with having a real
return type.
That would allow userland libs to immediately adopt the tag and be ready
for PHP 8.1.
WDYT?
For userland, there are already ways to declare the planned return type
(aka @return in phpdoc), so we might not need any new way to declare this
from the engine pov.
Yes, I think I agree, but I'd be curious about Nikita's opinion as well,
since he brought up this problem first.
We should replace the SuppressReturnTypeNotice by the TentativeReturnType
one right away.
The TentativeReturnType would mean: "I'm going to add a return type to this
method in its next major".
As a consequence, for child classes of internal methods, this would
suppress the notice.
If we get rid of the userland part of my proposal, then I think we can
really rename SuppressReturnTypeNotice
to TentativeReturnType.
As a corollary, adding this attribute should conflict with having a real
return type.
This is the only questionable part for me, because then this RFC would
cause more trouble for overriding methods
which already define a wrong return type: a deprecation notice would always
be triggered for them on PHP 8.1+
until return types are fixed or removed. And as we discussed it previously,
fixing them is not always possible.
Or is there any specific reason I'm not aware of why you favor this
behavior?
Máté
For userland, there are already ways to declare the planned return type
(aka @return in phpdoc), so we might not need any new way to declare this
from the engine pov.Yes, I think I agree, but I'd be curious about Nikita's opinion as well,
since he brought up this problem first.We should replace the SuppressReturnTypeNotice by the TentativeReturnType
one right away.The TentativeReturnType would mean: "I'm going to add a return type to
this method in its next major".
As a consequence, for child classes of internal methods, this would
suppress the notice.If we get rid of the userland part of my proposal, then I think we can
really rename SuppressReturnTypeNotice
to TentativeReturnType.
This would mean that the RFC could describe how userland (code analyzers)
should understand the TentativeReturnType when encountering one, without
asking anything from the engine when base userland classes are extended.
For internal classes, the attribute would just mean what you wrote already.
That'd be fine to me.
As a corollary, adding this attribute should conflict with having a real
return type.
This is the only questionable part for me, because then this RFC would
cause more trouble for overriding methods
which already define a wrong return type: a deprecation notice would
always be triggered for them on PHP 8.1+
until return types are fixed or removed. And as we discussed it
previously, fixing them is not always possible.
Or is there any specific reason I'm not aware of why you favor this
behavior?
To me, overriding methods that define a wrong return type should remove
their return type as a fix, and replace it with the attribute. Removing a
return type is allowed by LSP, so this should be just fine.
Nicolas