[RFC] Always generate fatal error for incompatible method signatures

  105158
April 9, 2019 08:25 nikita.ppv@gmail.com (Nikita Popov)
Hi internals,

A small cleanup RFC for PHP 8: https://wiki.php.net/rfc/lsp_errors

This makes all incompatible method signature (LSP) errors fatal, rather
than only warning in some cases. Especially after
https://wiki.php.net/rfc/parameter-no-type-variance I don't think there's
any reason to keep this backdoor in place.

Regards,
Nikita
  105162
April 9, 2019 09:39 bishop@php.net (Bishop Bettini)
On Tue, Apr 9, 2019 at 4:25 AM Nikita Popov ppv@gmail.com> wrote:

> Hi internals, > > A small cleanup RFC for PHP 8: https://wiki.php.net/rfc/lsp_errors > > This makes all incompatible method signature (LSP) errors fatal, rather > than only warning in some cases. Especially after > https://wiki.php.net/rfc/parameter-no-type-variance I don't think there's > any reason to keep this backdoor in place. >
Thank you, Nikita. +1 from me. This inconsistency was a personal WTF moment, 3 years ago: https://stackoverflow.com/q/37889466/2908724 On Tue, Apr 9, 2019 at 4:25 AM Nikita Popov ppv@gmail.com> wrote:
> Hi internals, > > A small cleanup RFC for PHP 8: https://wiki.php.net/rfc/lsp_errors > > This makes all incompatible method signature (LSP) errors fatal, rather > than only warning in some cases. Especially after > https://wiki.php.net/rfc/parameter-no-type-variance I don't think there's > any reason to keep this backdoor in place. > > Regards, > Nikita >
  105169
April 9, 2019 10:58 gadelat@gmail.com (Gabriel O)
I see you are trying to solve inconsistency, but i would do it opposite way. LSP errors should be warnings, they are not show stopper errors. It’s very much possible code violating LSP keeps working correctly. And in modern PHP development, everybody considers warnings as show stoppers anyway, so impact here is only in production. When some LSP breakage sneaks unnoticed into production and my INI settings just silence these and log them, I would prefer it to not crash there, because problem with these show only when expecting parent but receiving child, not when child breaks LSP but its nowhere used in place of parent.

And this RFC conveniently shows only big LSP violation examples like array -> int, but not widely used narrowing like mixed/object -> specific instance.

> On 9. Apr 2019, at 10:25, Nikita Popov ppv@gmail.com> wrote: > > Hi internals, > > A small cleanup RFC for PHP 8: https://wiki.php.net/rfc/lsp_errors > > This makes all incompatible method signature (LSP) errors fatal, rather > than only warning in some cases. Especially after > https://wiki.php.net/rfc/parameter-no-type-variance I don't think there's > any reason to keep this backdoor in place. > > Regards, > Nikita
  105177
April 9, 2019 14:47 Danack@basereality.com (Dan Ackroyd)
On Tue, 9 Apr 2019 at 11:58, Gabriel O <gadelat@gmail.com> wrote:

> And this RFC conveniently shows only big LSP violation examples like array -> int, but not widely used narrowing like mixed/object -> specific instance.
Type narrowing or contravariant parameters is properly supported for PHP 7.4: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters , which is why this RFC doesn't need to cover them. cheers Dan Ack
  105178
April 9, 2019 15:00 gadelat@gmail.com (Gabriel O)
I believe rfc deals with reverse situation

On 9 April 2019 4:47:50 PM Dan Ackroyd <Danack@basereality.com> wrote:

> On Tue, 9 Apr 2019 at 11:58, Gabriel O <gadelat@gmail.com> wrote: > >> And this RFC conveniently shows only big LSP violation examples like array >> -> int, but not widely used narrowing like mixed/object -> specific instance. > > > Type narrowing or contravariant parameters is properly supported for > PHP 7.4: > https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters > , which is why this RFC doesn't need to cover them. > > cheers > Dan > Ack
  105180
April 9, 2019 15:35 levim@php.net (Levi Morrison)
On Tue, Apr 9, 2019 at 9:00 AM Gabriel O <gadelat@gmail.com> wrote:
> > I believe rfc deals with reverse situation > > On 9 April 2019 4:47:50 PM Dan Ackroyd <Danack@basereality.com> wrote: > > > On Tue, 9 Apr 2019 at 11:58, Gabriel O <gadelat@gmail.com> wrote: > > > >> And this RFC conveniently shows only big LSP violation examples like array > >> -> int, but not widely used narrowing like mixed/object -> specific instance. > > > > > > Type narrowing or contravariant parameters is properly supported for > > PHP 7.4: > > https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters > > , which is why this RFC doesn't need to cover them. > > > > cheers > > Dan > > Ack
If you want the reverse to be true, then your code has bugs waiting to show themselves. The earlier we can catch these bugs, the better. One thing to note is that return type information does permit optimizations in some cases. For instance, I believe if a private method has a return type constraint of int, then the optimizer can rule out certain edge cases, and in some cases use type specialized handlers. We can do these sorts of optimizations in more places if the LSP issues are always errors, instead of warnings. So for both correctness and performance, I support this change.
  105181
April 9, 2019 16:05 gadelat@gmail.com (Gabriel O)
> If you want the reverse to be true, then your code has bugs waiting to > show themselves
I don’t, I just don’t want fatal errors in production when I upgrade library I extend. Let me keep logging such issues and fix them later, instead of producing hard crash for customers.
> The earlier we can catch these bugs, the better.
What prevents somebody to catch warnings? This RFC is about hard crash that I disagree with. This type of issue is problematic only when expecting parent but injecting child.
> On 9. Apr 2019, at 17:35, Levi Morrison <levim@php.net> wrote: > > On Tue, Apr 9, 2019 at 9:00 AM Gabriel O <gadelat@gmail.com> wrote: >> >> I believe rfc deals with reverse situation >> >> On 9 April 2019 4:47:50 PM Dan Ackroyd <Danack@basereality.com> wrote: >> >>> On Tue, 9 Apr 2019 at 11:58, Gabriel O <gadelat@gmail.com> wrote: >>> >>>> And this RFC conveniently shows only big LSP violation examples like array >>>> -> int, but not widely used narrowing like mixed/object -> specific instance. >>> >>> >>> Type narrowing or contravariant parameters is properly supported for >>> PHP 7.4: >>> https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters >>> , which is why this RFC doesn't need to cover them. >>> >>> cheers >>> Dan >>> Ack > > If you want the reverse to be true, then your code has bugs waiting to > show themselves. The earlier we can catch these bugs, the better. > > One thing to note is that return type information does permit > optimizations in some cases. For instance, I believe if a private > method has a return type constraint of int, then the optimizer can > rule out certain edge cases, and in some cases use type specialized > handlers. We can do these sorts of optimizations in more places if the > LSP issues are always errors, instead of warnings. > > So for both correctness and performance, I support this change.
  105209
April 10, 2019 14:48 theodorejb@outlook.com (Theodore Brown)
On Tue, Apr 9, 2019 at 11:05 AM Gabriel O <gadelat@gmail.com> wrote:

>> On 9. Apr 2019, at 17:35, Levi Morrison <levim@php.net> wrote: >> >> If you want the reverse to be true, then your code has bugs waiting to >> show themselves. The earlier we can catch these bugs, the better. >> >> One thing to note is that return type information does permit >> optimizations in some cases. For instance, I believe if a private >> method has a return type constraint of int, then the optimizer can >> rule out certain edge cases, and in some cases use type specialized >> handlers. We can do these sorts of optimizations in more places if the >> LSP issues are always errors, instead of warnings. >> >> So for both correctness and performance, I support this change. > > >> If you want the reverse to be true, then your code has bugs waiting to >> show themselves > > I don’t, I just don’t want fatal errors in production when I upgrade > library I extend. Let me keep logging such issues and fix them later, > instead of producing hard crash for customers. > >> The earlier we can catch these bugs, the better. > > What prevents somebody to catch warnings? This RFC is about hard crash > that I disagree with. This type of issue is problematic only when > expecting parent but injecting child.
Have you considered testing your code in a dev environment after upgrading a library you extend, prior to pushing it to production? If you blindly push library updates to production without testing, there are many other types of errors you could run into besides this. I for one would welcome consistent fatal errors for LSP violations, to catch these kind of bugs as early as possible during development.
  105233
April 11, 2019 08:36 sebastian@php.net (Sebastian Bergmann)
Am 09.04.2019 um 10:25 schrieb Nikita Popov:
> A small cleanup RFC for PHP 8: https://wik +1
  105327
April 22, 2019 08:04 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Apr 9, 2019 at 10:25 AM Nikita Popov ppv@gmail.com> wrote:

> Hi internals, > > A small cleanup RFC for PHP 8: https://wiki.php.net/rfc/lsp_errors > > This makes all incompatible method signature (LSP) errors fatal, rather > than only warning in some cases. Especially after > https://wiki.php.net/rfc/parameter-no-type-variance I don't think there's > any reason to keep this backdoor in place. >
Heads up: Going to vote tomorrow. Nikita