[VOTE] Warn about invalid strings in arithmetic

php.internals

Andrew Faulds

10 years ago
Hi everyone, I previously opened voting on the "Warn about invalid strings in arithmetic" RFC, but I had to cancel it because of the issue of inconsistent handling of scientific notation strings. Since then, the RFC has been updated to deal with that issue. This has meant that the RFC no longer just concerns adding warnings to operators, but also fixing the inconsistency with how scientific notation strings are converted in PHP. This has been previously discussed on the mailing list. Now that I have finally completed my patch for the language specification for the RFC, I feel I can open voting once again. So, voting on this RFC will start today (2016-03-20), and will end on the Monday after next (2016-03-28). You can find the RFC page here, which contains the voting widget: https://wiki.php.net/rfc/invalid_strings_in_arithmetic Please read over the RFC and cast your vote. Thank you!
-- Andrea Faulds https://ajf.me/

Patrick ALLAERT

10 years ago
Le dim. 20 mars 2016 à 03:36, Andrea Faulds <ajf@ajf.me> a écrit :
> Hi everyone, > > I previously opened voting on the "Warn about invalid strings in > arithmetic" RFC, but I had to cancel it because of the issue of > inconsistent handling of scientific notation strings. > > Since then, the RFC has been updated to deal with that issue. This has > meant that the RFC no longer just concerns adding warnings to operators, > but also fixing the inconsistency with how scientific notation strings > are converted in PHP. This has been previously discussed on the mailing > list. > > Now that I have finally completed my patch for the language > specification for the RFC, I feel I can open voting once again. > > So, voting on this RFC will start today (2016-03-20), and will end on > the Monday after next (2016-03-28). > > You can find the RFC page here, which contains the voting widget: > > https://wiki.php.net/rfc/invalid_strings_in_arithmetic > > Please read over the RFC and cast your vote. > > Thank you! > -- > Andrea Faulds > https://ajf.me/ > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php
Hi Andrea, Nice work. I'm globally +0.7 on it, there is however a few things that are unclear to me: * What happens with an empty string? Warning, notice or even nothing? * Would: 42 + ""; produce the same thing than: 42 + null; currently, both are quiet, but both doesn't mean something. * You don't mention the <, >, <= and >= operators, is that intentional? php > var_dump( 42 < "42 bananas" ); bool(false) php > var_dump( 42 <= "42 bananas" ); bool(true) php > var_dump( 42 > "42 bananas" ); bool(false) php > var_dump( 42 >= "42 bananas" ); bool(true) It would be illogic, IMHO, to not take those operators into account and would introduce some inconsistencies, that is why I voted "No" although I'm generally ok with the whole idea. Could you clarify those points? I guess you can not put them in the RFC while in voting phase? I personally don't mind and could easily change my vote provided that I have answers to my questions :) Cheers, Patrick

Andrew Faulds

10 years ago
Hi Patrick, Patrick ALLAERT wrote:
> Hi Andrea, > > Nice work. > > I'm globally +0.7 on it, there is however a few things that are unclear to > me: > > * What happens with an empty string? Warning, notice or even nothing? > > * Would: > 42 + ""; > produce the same thing than: > 42 + null; > currently, both are quiet, but both doesn't mean something.
"" is non-numeric, so (42 + "") produces a warning. This RFC doesn't touch non-string values, however, so (42 + null) continues to produce no error: $ sapi/cli/php -r 'var_dump(42 + "");' Warning: A non-numeric value encountered in Command line code on line 1 int(42) $ sapi/cli/php -r 'var_dump(42 + null);' int(42)
> * You don't mention the <, >, <= and >= operators, is that intentional? > php > var_dump( 42 < "42 bananas" ); > bool(false) > php > var_dump( 42 <= "42 bananas" ); > bool(true) > php > var_dump( 42 > "42 bananas" ); > bool(false) > php > var_dump( 42 >= "42 bananas" ); > bool(true)
The RFC does mention them in the "Unaffected PHP Functionality" section: "It also does not impact the behaviour of type juggling for comparisons." I can't entirely blame you for missing that, though. So, those examples you provided will continue to not produce errors. This is deliberate.
> It would be illogic, IMHO, to not take those operators into account and > would introduce some inconsistencies, that is why I voted "No" although I'm > generally ok with the whole idea.
I might like it if we change the behaviour of those operators, but I don't think this is the RFC to do it in. This RFC adds E_NOTICE and E_WARNING errors to certain operators that take numbers as inputs and produce numbers as outputs. Arguably, strings which are only partly a number, or not a number, are incorrect input for these operators, or at the very least unlikely to be intentional on the part of the programmer. Therefore, warning the programmer that the inputs given were invalid is arguably helpful. I chose to produce E_NOTICEs and E_WARNINGs because they are less likely to cause backwards-compatibility issues than, for example, a TypeError, and this is important given the release process RFC forbids us from breaking compatibility in a minor release (like 7.1). While I think my rationale above for adding warnings makes sense for number operators, I don't think it works for the comparison ones. The comparison operators all take two values which each can be of any type, and there are no particular expectations about what their input should be. Since non-well-formed numeric strings aren't invalid input, we don't have the case to warn about them here. What *is* a problem for the comparison operators is how they interpret numeric strings, or even that they interpret them at all. A warning also doesn't help here. So what would? Well, we could decide on some more sensible interpretation of numeric strings. But that would mean that the behaviour of the comparison operators would change, a significant backwards-compatibility break, and a particularly bad one because it's completely silent: there's no runtime warning that the behaviour has changed, and no IDE or static analyser could warn you where your code might have been affected. It's possible to make these kinds of changes sometimes, but we must be very careful about it, and it shouldn't be done in a minor release like 7.1. It would also be a controversial change, so putting it in this RFC might doom the other changes it makes. I hope this makes it clearer why I didn't touch the comparison operators in this RFC. I think they're a separate, but related issue. Thanks for asking.
-- Andrea Faulds https://ajf.me/

Björn Larsson

10 years ago
Den 2016-03-24 kl. 21:29, skrev Andrea Faulds:
> Hi Patrick, > > Patrick ALLAERT wrote: >> Hi Andrea, >> >> Nice work. >> >> I'm globally +0.7 on it, there is however a few things that are >> unclear to >> me: >> >> * What happens with an empty string? Warning, notice or even nothing? >> >> * Would: >> 42 + ""; >> produce the same thing than: >> 42 + null; >> currently, both are quiet, but both doesn't mean something. > > "" is non-numeric, so (42 + "") produces a warning. This RFC doesn't > touch non-string values, however, so (42 + null) continues to produce > no error: > > $ sapi/cli/php -r 'var_dump(42 + "");' > > Warning: A non-numeric value encountered in Command line code on > line 1 > int(42) > > $ sapi/cli/php -r 'var_dump(42 + null);' > int(42) > >> * You don't mention the <, >, <= and >= operators, is that intentional? >> php > var_dump( 42 < "42 bananas" ); >> bool(false) >> php > var_dump( 42 <= "42 bananas" ); >> bool(true) >> php > var_dump( 42 > "42 bananas" ); >> bool(false) >> php > var_dump( 42 >= "42 bananas" ); >> bool(true) > > The RFC does mention them in the "Unaffected PHP Functionality" > section: "It also does not impact the behaviour of type juggling for > comparisons." > > I can't entirely blame you for missing that, though. > > So, those examples you provided will continue to not produce errors. > This is deliberate. > >> It would be illogic, IMHO, to not take those operators into account and >> would introduce some inconsistencies, that is why I voted "No" >> although I'm >> generally ok with the whole idea. > > I might like it if we change the behaviour of those operators, but I > don't think this is the RFC to do it in. > > This RFC adds E_NOTICE and E_WARNING errors to certain operators that > take numbers as inputs and produce numbers as outputs. Arguably, > strings which are only partly a number, or not a number, are incorrect > input for these operators, or at the very least unlikely to be > intentional on the part of the programmer. Therefore, warning the > programmer that the inputs given were invalid is arguably helpful. I > chose to produce E_NOTICEs and E_WARNINGs because they are less likely > to cause backwards-compatibility issues than, for example, a > TypeError, and this is important given the release process RFC forbids > us from breaking compatibility in a minor release (like 7.1). > > While I think my rationale above for adding warnings makes sense for > number operators, I don't think it works for the comparison ones. The > comparison operators all take two values which each can be of any > type, and there are no particular expectations about what their input > should be. Since non-well-formed numeric strings aren't invalid input, > we don't have the case to warn about them here. > > What *is* a problem for the comparison operators is how they interpret > numeric strings, or even that they interpret them at all. A warning > also doesn't help here. So what would? Well, we could decide on some > more sensible interpretation of numeric strings. But that would mean > that the behaviour of the comparison operators would change, a > significant backwards-compatibility break, and a particularly bad one > because it's completely silent: there's no runtime warning that the > behaviour has changed, and no IDE or static analyser could warn you > where your code might have been affected. It's possible to make these > kinds of changes sometimes, but we must be very careful about it, and > it shouldn't be done in a minor release like 7.1. It would also be a > controversial change, so putting it in this RFC might doom the other > changes it makes. > > I hope this makes it clearer why I didn't touch the comparison > operators in this RFC. I think they're a separate, but related issue. > > Thanks for asking. >
Came to think on a conversation with Sara G last year about introducing the following operators, namely >==, <== & <==> working exactly like the existing ones but without type juggling. Maybe that would alleviate some of the issue here, giving new tools at hand when writing new or updating existing code? Regards //Björn

Yasuo Ohgaki

10 years ago
Hi, On Fri, Mar 25, 2016 at 6:25 AM, Björn Larsson <bjorn.x.larsson@telia.com> wrote:
> Came to think on a conversation with Sara G last year about introducing > the following operators, namely >==, <== & <==> working exactly like the > existing ones but without type juggling.
>==, <== were not introduced because results are not useful.
For instance, (10 >== "0") === false does not make much sense with PHP. It would not solve problem, but creates new hard to debug problem. However, raising errors and (10 >== "0") === null make sense. IMO. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Andrew Faulds

10 years ago
Hi Björn, Björn Larsson wrote:
> Den 2016-03-24 kl. 21:29, skrev Andrea Faulds: > Came to think on a conversation with Sara G last year about introducing > the following operators, namely >==, <== & <==> working exactly like the > existing ones but without type juggling. > > Maybe that would alleviate some of the issue here, giving new tools at > hand when writing new or updating existing code?
This is an interesting idea. === and !== avoid type juggling by returning FALSE if the types of their operands don't match, so what would these hypothetical strict versions of < and > do in that situation? One approach would be to order by type, so any integer < any float < any string, for example. I think I'd like that, it'd be more predictable than PHP's current comparison operators, where "1" < "a" < "b" < 1 < "2" < 2. I'm not sure about those symbols, though. Given their appearance I'd think >== and <== would be strict versions of >= and <=, even though I think you're proposing them to be strict versions of > and <. But surely something could be figured out. Thanks!
-- Andrea Faulds https://ajf.me/

Patrick ALLAERT

10 years ago
Le jeu. 24 mars 2016 à 21:29, Andrea Faulds <ajf@ajf.me> a écrit :
> Hi Patrick, > > Patrick ALLAERT wrote: > > Hi Andrea, > > > > Nice work. > > > > I'm globally +0.7 on it, there is however a few things that are unclear > to > > me: > > > > * What happens with an empty string? Warning, notice or even nothing? > > > > * Would: > > 42 + ""; > > produce the same thing than: > > 42 + null; > > currently, both are quiet, but both doesn't mean something. > > "" is non-numeric, so (42 + "") produces a warning. This RFC doesn't > touch non-string values, however, so (42 + null) continues to produce no > error: > > $ sapi/cli/php -r 'var_dump(42 + "");' > > Warning: A non-numeric value encountered in Command line code on line > 1 > int(42) > > $ sapi/cli/php -r 'var_dump(42 + null);' > int(42) > > > * You don't mention the <, >, <= and >= operators, is that intentional? > > php > var_dump( 42 < "42 bananas" ); > > bool(false) > > php > var_dump( 42 <= "42 bananas" ); > > bool(true) > > php > var_dump( 42 > "42 bananas" ); > > bool(false) > > php > var_dump( 42 >= "42 bananas" ); > > bool(true) > > The RFC does mention them in the "Unaffected PHP Functionality" section: > "It also does not impact the behaviour of type juggling for comparisons." > > I can't entirely blame you for missing that, though. >
I searched on the "<" / ">" chars, that's how I missed it. My bad!
> So, those examples you provided will continue to not produce errors. > This is deliberate. > > > It would be illogic, IMHO, to not take those operators into account and > > would introduce some inconsistencies, that is why I voted "No" although > I'm > > generally ok with the whole idea. > > I might like it if we change the behaviour of those operators, but I > don't think this is the RFC to do it in. > > This RFC adds E_NOTICE and E_WARNING errors to certain operators that > take numbers as inputs and produce numbers as outputs. Arguably, strings > which are only partly a number, or not a number, are incorrect input for > these operators, or at the very least unlikely to be intentional on the > part of the programmer. Therefore, warning the programmer that the > inputs given were invalid is arguably helpful. I chose to produce > E_NOTICEs and E_WARNINGs because they are less likely to cause > backwards-compatibility issues than, for example, a TypeError, and this > is important given the release process RFC forbids us from breaking > compatibility in a minor release (like 7.1). > > While I think my rationale above for adding warnings makes sense for > number operators, I don't think it works for the comparison ones. The > comparison operators all take two values which each can be of any type, > and there are no particular expectations about what their input should > be. Since non-well-formed numeric strings aren't invalid input, we don't > have the case to warn about them here. > > What *is* a problem for the comparison operators is how they interpret > numeric strings, or even that they interpret them at all. A warning also > doesn't help here. So what would? Well, we could decide on some more > sensible interpretation of numeric strings. But that would mean that the > behaviour of the comparison operators would change, a significant > backwards-compatibility break, and a particularly bad one because it's > completely silent: there's no runtime warning that the behaviour has > changed, and no IDE or static analyser could warn you where your code > might have been affected. It's possible to make these kinds of changes > sometimes, but we must be very careful about it, and it shouldn't be > done in a minor release like 7.1. It would also be a controversial > change, so putting it in this RFC might doom the other changes it makes. > > I hope this makes it clearer why I didn't touch the comparison operators > in this RFC. I think they're a separate, but related issue. > > Thanks for asking. > > -- > Andrea Faulds > https://ajf.me/
I understand and follow you on this. Thanks for clearing everything out. +1 :) Patrick