[RFC] Consistent type errors for internal functions

Girgias

7 years ago

On Tue, 5 Feb 2019 at 12:22, Nikita Popov <nikita.ppv@gmail.com> wrote:

> Hi internals,
>
> I'd like to bring forward the following proposal for PHP 8, which will make
> (zpp) parameter parsing failures always result in a TypeError (rather than
> generating a warning+null, depending on circumstances):
>
> https://wiki.php.net/rfc/consistent_type_errors
>
> The goal here is to remove one of the inconsistencies between user-defined
> and internal functions, and to put us in a position where we can actually
> start specifying type information in arginfo without fear of breaking
> things.
>
> Regards,
> Nikita
>

I'm all for it but what is the scope of the RFC?
Is it all core functions, bundled extension functions, or all extension
functions?
Also does this means that there will be argument type hinting in core
functions that
could be found out via reflection?

Best regards

George P. Banyard

Nikita Popov

7 years ago

On Tue, Feb 5, 2019 at 1:23 PM Girgias <george.banyard@gmail.com> wrote:

> On Tue, 5 Feb 2019 at 12:22, Nikita Popov <nikita.ppv@gmail.com> wrote:
>
>> Hi internals,
>>
>> I'd like to bring forward the following proposal for PHP 8, which will
>> make
>> (zpp) parameter parsing failures always result in a TypeError (rather than
>> generating a warning+null, depending on circumstances):
>>
>> https://wiki.php.net/rfc/consistent_type_errors
>>
>> The goal here is to remove one of the inconsistencies between user-defined
>> and internal functions, and to put us in a position where we can actually
>> start specifying type information in arginfo without fear of breaking
>> things.
>>
>> Regards,
>> Nikita
>>
>
> I'm all for it but what is the scope of the RFC?
> Is it all core functions, bundled extension functions, or all extension
> functions?
>

It affects all internal functions using the zpp APIs, which covers pretty
much all core functions, bundled extension functions and third-party
extension functions.

> Also does this means that there will be argument type hinting in core
> functions that
> could be found out via reflection?
>

Not as a direct result of the proposal, but the RFC does remove the big
blocker for it. Once it lands we need one more change (don't actually
verify arginfo types for internal functions to avoid double type checking),
and then we can start adding the necessary type information. That will also
take some work, as we have many functions, but it's something everyone can
help with.

Nikita

Girgias

7 years ago

On Tue, 5 Feb 2019 at 14:29, Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Tue, Feb 5, 2019 at 1:23 PM Girgias <george.banyard@gmail.com> wrote:
>
>> On Tue, 5 Feb 2019 at 12:22, Nikita Popov <nikita.ppv@gmail.com> wrote:
>>
>>> [. . .]
>>>
>>
>> I'm all for it but what is the scope of the RFC?
>> Is it all core functions, bundled extension functions, or all extension
>> functions?
>>
>
> It affects all internal functions using the zpp APIs, which covers pretty
> much all core functions, bundled extension functions and third-party
> extension functions.
>

Thanks for the clarification, that's great

Also does this means that there will be argument type hinting in core

>> functions that
>> could be found out via reflection?
>>
>
> Not as a direct result of the proposal, but the RFC does remove the big
> blocker for it. Once it lands we need one more change (don't actually
> verify arginfo types for internal functions to avoid double type checking),
> and then we can start adding the necessary type information. That will also
> take some work, as we have many functions, but it's something everyone can
> help with.
>
> Nikita
>

I suppose if this RFC gets voted (and implemented) there should be enough
(maybe I'm wrong) time to also implement this.

Really like this RFC even with the potential BC break as there are Static
analysis tools (such as PHPStan, Psalm and forgot the third major one)
which can help point out potential type errors.

Best regards

George P. Banyard

Andrew Faulds

7 years ago

Hi Nikita,

Nikita Popov wrote:

> I'd like to bring forward the following proposal for PHP 8, which will make
> (zpp) parameter parsing failures always result in a TypeError (rather than
> generating a warning+null, depending on circumstances):
>
> https://wiki.php.net/rfc/consistent_type_errors

I like this proposal. IMO PHP's E_WARNING + NULL is the worst of its
“Keep Calm and Carry On” (sorry) behaviours, it would be nice to get rid
of it for good, rather than just in the comfy world of strict_types=1.

> The goal here is to remove one of the inconsistencies between user-defined
> and internal functions, and to put us in a position where we can actually
> start specifying type information in arginfo without fear of breaking
> things.

Regrettably, as I pointed out to you via another channel, that idea also
faces the problem of the other deliberate inconsistency I am responsible
for in userland scalar type declarations, namely that the non-nullable
variety of those reject null as a valid value, unlike internal functions
which will happily coerce it. It's funny to mention this here, as the
E_WARNING + NULL behaviour your RFC would drop was a primary
justification of mine for making NULL special here. Unfortunately it's
not the only case, I'm sure uncountably much PHP code relies on things
like strlen($_GET['nonexistent']) working… but I digress.

Thanks,
Andrea

Nicolas Grekas

7 years ago

Hi Nikita

https://wiki.php.net/rfc/consistent_type_errors

Would it make sense and be possible to trigger a deprecation notice in PHP
7.4?

That might help the ecosystem move forward in a smooth way instead of
experimenting the failure when actually moving to 8.

Nicolas

Nikita Popov

7 years ago

On Wed, Feb 6, 2019 at 7:30 AM Nicolas Grekas <nicolas.grekas@gmail.com>
wrote:

> Hi Nikita
>
>
> https://wiki.php.net/rfc/consistent_type_errors
>
>
> Would it make sense and be possible to trigger a deprecation notice in PHP
> 7.4?
>
> That might help the ecosystem move forward in a smooth way instead of
> experimenting the failure when actually moving to 8.
>
> Nicolas
>

I don't think so. We generally consider a warning a "harder" error than a
deprecation (unlike deprecations, they are part of the default
error_reporting level), so I don't think replacing or adding a deprecation
would do much here. The only way you can be unaware of problems resulting
from this change is if you are deliberately suppressing warnings, in which
case chances are very good that you are also suppressing deprecations.

Nikita

Nicolas Grekas

7 years ago

> https://wiki.php.net/rfc/consistent_type_errors
>>
>>
>> Would it make sense and be possible to trigger a deprecation notice in
>> PHP 7.4?
>>
>> That might help the ecosystem move forward in a smooth way instead of
>> experimenting the failure when actually moving to 8.
>>
>
> We generally consider a warning a "harder" error than a deprecation
>

Do all situations that will throw after the RFC trigger a warning right
now? Eg var_dump(substr(null, 1)); doesn't trigger anything but returns
false - does that mean it will not throw a TypeError in your proposal?

> if you are deliberately suppressing warnings, in which case chances are
> very good that you are also suppressing deprecations.
>

This assumption is wrong in the Symfony ecoystem!
We built the deprecation framework around silenced (and unsilenced)
deprecations, which are all caught and logged.
So it would make a huge difference actually, at least for situations where
no warning is triggered (see substr example above).

Nicolas

Nikita Popov

7 years ago

On Wed, Feb 6, 2019 at 9:43 AM Nicolas Grekas <nicolas.grekas@gmail.com>
wrote:

>
> https://wiki.php.net/rfc/consistent_type_errors
>>>
>>>
>>> Would it make sense and be possible to trigger a deprecation notice in
>>> PHP 7.4?
>>>
>>> That might help the ecosystem move forward in a smooth way instead of
>>> experimenting the failure when actually moving to 8.
>>>
>>
>> We generally consider a warning a "harder" error than a deprecation
>>
>
> Do all situations that will throw after the RFC trigger a warning right
> now? Eg var_dump(substr(null, 1)); doesn't trigger anything but returns
> false - does that mean it will not throw a TypeError in your proposal?
>

That's correct, it will not throw a TypeError. If the call currently
succeeds, then it will continue to succeed. substr(null, 1) is considered a
perfectly valid substr() call right now.

if you are deliberately suppressing warnings, in which case chances are

> very good that you are also suppressing deprecations.
>
> This assumption is wrong in the Symfony ecoystem!
> We built the deprecation framework around silenced (and unsilenced)
> deprecations, which are all caught and logged.
> So it would make a huge difference actually, at least for situations where
> no warning is triggered (see substr example above).
>

Right, but a warning is always triggered here. If there were no warning,
then yes, I'd of course agree that this should throw a deprecation first.

Nikita

Nikita Popov

7 years ago

On Tue, Feb 5, 2019 at 12:22 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

> Hi internals,
>
> I'd like to bring forward the following proposal for PHP 8, which will
> make (zpp) parameter parsing failures always result in a TypeError (rather
> than generating a warning+null, depending on circumstances):
>
> https://wiki.php.net/rfc/consistent_type_errors
>
> The goal here is to remove one of the inconsistencies between user-defined
> and internal functions, and to put us in a position where we can actually
> start specifying type information in arginfo without fear of breaking
> things.
>
> Regards,
> Nikita
>

Heads up: This is headed for voting on Tuesday, assuming no significant
issues are raised in the meantime.

Nikita

Ben Ramsey

7 years ago

> On Feb 5, 2019, at 05:22, Nikita Popov <nikita.ppv@gmail.com> wrote:
>
> Hi internals,
>
> I'd like to bring forward the following proposal for PHP 8, which will make
> (zpp) parameter parsing failures always result in a TypeError (rather than
> generating a warning+null, depending on circumstances):
>
> https://wiki.php.net/rfc/consistent_type_errors
>
> The goal here is to remove one of the inconsistencies between user-defined
> and internal functions, and to put us in a position where we can actually
> start specifying type information in arginfo without fear of breaking
> things.

I like this RFC, and from a user perspective, this consistency is much-needed.

While warnings don’t affect continued processing, uncaught TypeErrors do. What is the recommended upgrade path for users? How will this affect the silence @-operator, since I’m sure many users use that to squelch these warnings, and they accept the null or false return value as acceptable for processing the rest of the script.

Cheers,
Ben

Benjamin Eberlei

7 years ago

Ben Ramsey <ben@benramsey.com> schrieb am Sa. 16. Feb. 2019 um 18:35:

> > On Feb 5, 2019, at 05:22, Nikita Popov <nikita.ppv@gmail.com> wrote:
> >
> > Hi internals,
> >
> > I'd like to bring forward the following proposal for PHP 8, which will
> make
> > (zpp) parameter parsing failures always result in a TypeError (rather
> than
> > generating a warning+null, depending on circumstances):
> >
> > https://wiki.php.net/rfc/consistent_type_errors
> >
> > The goal here is to remove one of the inconsistencies between
> user-defined
> > and internal functions, and to put us in a position where we can actually
> > start specifying type information in arginfo without fear of breaking
> > things.
>
>
> I like this RFC, and from a user perspective, this consistency is
> much-needed.
>
> While warnings don’t affect continued processing, uncaught TypeErrors do.
> What is the recommended upgrade path for users? How will this affect the
> silence @-operator, since I’m sure many users use that to squelch these
> warnings, and they accept the null or false return value as acceptable for
> processing the rest of the script.
>
> Cheers,
> Ben

This exception is only for argument errors. Fopen would continue to warn +
false when file doesnt exist, which is what @ is nostly used for (using
fopen as example)

Ben Ramsey

7 years ago

> On Feb 16, 2019, at 12:28, Benjamin Eberlei <kontakt@beberlei.de> wrote:
>
> This exception is only for argument errors. Fopen would continue to warn + false when file doesnt exist, which is what @ is nostly used for (using fopen as example)

I’ve seen many cases where it’s understood that the function might receive the wrong type, and the developer decides this is okay, so they use the @-operator to ignore the warning and allow the script to continue processing.

Nikita Popov

7 years ago

On Sat, Feb 16, 2019 at 6:34 PM Ben Ramsey <ben@benramsey.com> wrote:

> > On Feb 5, 2019, at 05:22, Nikita Popov <nikita.ppv@gmail.com> wrote:
> >
> > Hi internals,
> >
> > I'd like to bring forward the following proposal for PHP 8, which will
> make
> > (zpp) parameter parsing failures always result in a TypeError (rather
> than
> > generating a warning+null, depending on circumstances):
> >
> > https://wiki.php.net/rfc/consistent_type_errors
> >
> > The goal here is to remove one of the inconsistencies between
> user-defined
> > and internal functions, and to put us in a position where we can actually
> > start specifying type information in arginfo without fear of breaking
> > things.
>
>
> I like this RFC, and from a user perspective, this consistency is
> much-needed.
>
> While warnings don’t affect continued processing, uncaught TypeErrors do.
> What is the recommended upgrade path for users? How will this affect the
> silence @-operator, since I’m sure many users use that to squelch these
> warnings, and they accept the null or false return value as acceptable for
> processing the rest of the script.
>
> Cheers,
> Ben
>

The recommended upgrade path here is basically "deal with it". If a
codebase does this (intentionally calling functions with invalid params and
suppressing errors) a lot, then it might make sense to register an error
handler that collects all "expected parameter" style errors in a production
environment, to easily identify all code that needs to be fixed. The error
messages have a consistent format, so it should be simple to identify and
eliminate all warnings of this type.

Nikita

Christian Schneider

7 years ago

Am 17.02.2019 um 17:24 schrieb Nikita Popov <nikita.ppv@gmail.com>:

> The recommended upgrade path here is basically "deal with it". If a
> codebase does this (intentionally calling functions with invalid params and
> suppressing errors) a lot, then it might make sense to register an error
> handler that collects all "expected parameter" style errors in a production
> environment, to easily identify all code that needs to be fixed. The error
> messages have a consistent format, so it should be simple to identify and
> eliminate all warnings of this type.

I have an example where this might be harder than necessary:
I'm importing data from an external source. Now in the real-world the import data files can once in a blue moon contain bogus data, e.g. there could be an array instead of a string in one single entry and I'm doing a strlen() on it.

Previously this triggered a Warning but the import job completed for all the other entries while still informing me that something was amiss.

The new way leaves me with three options:
1) My import jobs stops at the single broken entry
2) I can add type-checks for every single field
3) I can start wrapping code with try/catch

Neither of the three options are very appealing to me.
Option 1) might be appropriate when you are importing data which life and death depends on but for lots of stuff you're more interested in having the other 99% of your data updated instead of failing completely.
Option 2) sounds like the most defensive way of programming but it adds lots of boiler-plate code and doesn't feel phpish to me.
Option 3) is Java's RuntimeExceptions madness all over again which is surprisingly had to get right (where to catch what).

I'm a bit surprised the PHP community had such a change of mind concerning Exception in the core and BC breaks over the last years but as the vote seems very much to be in favour of this change I guess I'll have to bite the bullet once PHP 8 arrives.

Regards,
- Chris

Rowan Collins

7 years ago

On 1 March 2019 12:02:29 GMT+00:00, Christian Schneider <cschneid@cschneid.com> wrote:

>I have an example where this might be harder than necessary:
>I'm importing data from an external source. Now in the real-world the
>import data files can once in a blue moon contain bogus data, e.g.
>there could be an array instead of a string in one single entry and I'm
>doing a strlen() on it.
>
>Previously this triggered a Warning but the import job completed for
>all the other entries while still informing me that something was
>amiss.

The problem with Warnings is that processing doesn't just continue for all the *other* entries, it continues with *this entry that turned out to be bogus*. So now rather than a missing record, you have a record containing a 0, or the word "array", etc; and the Warning probably won't tell you which record it was that failed.

>The new way leaves me with three options:
>1) My import jobs stops at the single broken entry
>2) I can add type-checks for every single field
>3) I can start wrapping code with try/catch

Option 2 can be stated differently as "add validation for each input record": this isn't the same kind of type check as a library function picking up coding mistakes, it's handling of untrusted input.

Option 3 can be thought of as the transactional approach: if an error happens within a logical record, you can log that record to a reject file and continue with the next record.

There is definitely more effort to doing one of these compared to not doing it, but both seem preferable to letting bad data through, whether the language forces you to do something or not.

Regards,

--
Rowan Collins
[IMSoP]