On Sat, Mar 6, 2021 at 11:56 PM Máté Kocsis <kocsismat...@gmail.com> wrote:
> 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é > 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
