[RFC] [Discussion] Adding return types to internal methods

Hello Maté, https://wiki.php.net/rfc/internal_method_return_types) as well as the

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

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. [Reclassify E_STRICT notices]: https://wiki.php.net/rfc/reclassify_e_strict —Claude

On Sat, Mar 6, 2021 at 11:56 PM Máté Kocsis <kocsismate90@gmail.com> wrote:

Thanks Máté for the RFC, and thanks Nicolas for your comments. I think it's worth noting that this RFC tries to address a specific case of a more general problem: PHP's strict LSP checks make it hard to change method signatures, in ways that may be perceived as harmless. I think the two important ones are: 1. The one addressed by the RFC is addition of a return type (or more generally, restriction of a return type). This is generally done when the method was already guaranteed to return that type, and this fact was not explicitly declared previously. Direct users of the API are not affected by the change. Overriding methods can be affected in two ways: They made use of the lose API contract and returned a different type, in which case the implementation needs to be changed. Or, and this is almost always the case, they already adhere to the stricter API contract, but now need to explicitly declare this fact. 2. The second one not addressed here is the addition of optional parameters. Take for example the recent change of mysqli::execute() to mysqli::execute(?array $params = null). Once again, direct users of the API are not affected by the change. Overriding methods are affected because they need to pass through the new parameter. As this is an addition of a new optional feature, not passing through the optional parameter only becomes a problem if the new optional feature is actually used. Whether not passing it is "harmless" depends on whether the API declarer and the API user are the same. The commonality is that both of these changes have no impact on direct API consumers, only on inheritors (to give a counter example: Adding a new required parameter breaks direct consumers). In both cases it is technically easy to adjust the overriding APIs to comply with the new signature -- the problem is purely one of backwards compatibility guarantees provided to downstream consumers. Having said all that, I don't really see a solution for the general problem, short of allowing methods to opt-out of LSP checks entirely. I think doing so would be a very bad idea, because the functionality would certainly get misused for other purposes. As such, let's get back to the return types... I think as far as the original motivation is concerned (adding return types to internal methods), the RFC as proposed does well. The only thing I would change is to add the ReflectionMethod::getTentativeReturnType() part independently of the userland aspect. I believe it's important that this is exposed in reflection, even if the more general userland functionality is not provided. For example, if you have a mock generator, you'll probably want to either add the tentative return type as a real return type to mocks, or at least automatically add the #[SuppressReturnTypeNotice] attribute. I'm more ambivalent about the userland side of this proposal. As Nicolas has already pointed out, the RFC as proposed requires the library to have a PHP 8.1 minimum version requirement to make use of this functionality. And libraries requiring PHP 8.1 are likely not the target audience for this feature -- at least not when it comes to adding return types at all, there might still be a use when it comes to changing them in the future, as typesystem capabilities are expanded. I don't think it makes sense to introduce any solution in this space that requires PHP 8.1. If we introduce something for the userland side of things, then it should be compatible with older versions. Nicolas' suggestion of declaring the new return type inside the attribute #[TentativeReturnType("Foo|Bar")] would certainly work, and allow us to provide the same degree of functionality as the internal variant. That is, we could perform a variance check against the new tentative signature and throw a deprecation notice on mismatch. However, just like Máté, I don't really look forward to parsing information out of type strings. It's manageable now, but keeping in mind future extensions of the typesystem, dealing with #[TentativeReturnType("callable(Foo<T : int|string> &...)")] will be significantly less pleasant. That leaves Nicolas' last suggestion of not even trying to specify the type in a way that is understood by the engine -- just indicate that the type is going to change, and leave the actual type specification to @return. In this case the attribute would not have any actual effect from the engine perspective (apart from the interaction with the internal case). Which begs the question, why does this need to be part of PHP core at all? Is it just a matter of standardizing on a particular attribute? If we go down that route, I would suggest not to call this #[TentativeReturnType], but something like #[ReturnTypeWillChange]. I also don't think it's necessary to restrict this to methods that don't currently declare a return type, as it may be necessary to narrow a declared return type at a later time (e.g. due to typesystem improvements). For example: class A { /** @return int[] */ #[ReturnTypeWillChange] public function method(): array {} } class B extends A { // Accepted, static analyzers happy. public function method(): int[] {} } class C extends A { // Accepted, but static analyzers will warn. public function method(): array {} } class D extends A { // Not accepted, as it violates the proper array type. public function method() {} } So tl;dr I would either go with only the internal part of the proposal, or follow Nicolas' last suggestion, or some variant thereon. The version that's currently RFC doesn't seem particularly useful due to the version requirement. Regards, Nikita