[VOTE] Warn about invalid strings in arithmetic

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

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