core: Warn when non-representable floats are coerced to int #19760
Conversation
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
from
bf74b39 to
d34d8ff
Compare
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
from
d34d8ff to
6651e83
Compare
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
2 times, most recently
from
1bbbd2d to
997a22f
Compare
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
6 times, most recently
from
a9fd944 to
30d87e1
Compare
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
from
30d87e1 to
acba14f
Compare
Zend/zend_operators.c
Outdated
| ZEND_API void ZEND_COLD zend_oob_string_to_long_error(double d) | ||
| { | ||
| zend_error_unchecked(E_WARNING, "non-representable float-string %.*H was cast to int", -1, d); |
There was a problem hiding this comment.
Nit: It would be helpful if we could print the original string here
There was a problem hiding this comment.
I don't think it is possible for a non-float string to reach this part 🤔 but I could still add it as there only 2 calls to it.
There was a problem hiding this comment.
What I mean is that for code like echo (int) "10000000000000000000.0";, a warning such as non-representable float-string 10000000000000000000.0 was cast to int would be slightly more helpful than non-representable float-string 1.0E+19 was cast to int.
There was a problem hiding this comment.
Right, that is indeed true. Will see what I can do :)
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
from
acba14f to
c54fc7a
Compare
Girgias
requested review from
bukka,
derickr,
devnexen and
dstogov
as code owners
|
Non-representable float is incorrect wording and very confusing. The float is representable, it just doesn't fit when you cast. |
Would:
be better? |
Yes |
Girgias
force-pushed
the
8.5-warn-oob-float-to-int
branch
from
539331a to
f754909
Compare
It doesn't replace the existing deprecation as this is only for implicit coercions to integer, this is for explicit casts for values that are not representable by a PHP int, and is what is specified in https://wiki.php.net/rfc/warnings-php-8-5#casting_out_of_range_floats_to_int |
In the tests (e.g. |
This is because the bitwise operators do an integer cast internally. And the value that is being cast is out of range, if you replace the value with The operations that emit the deprecation introduced by https://wiki.php.net/rfc/implicit-float-int-deprecate had as a primary intention to prevent floats with fractional parts to be coerced implicitly into an integer as you lose the fractional part. I am struggling to see what the issue here is, this RFC indicates that explicit casts (be that made by the engine or userland) should warn. Requiring that explicit casts warn is a stronger requirement than implicit casts, and having diverging behaviour between implicit and explicit casts doesn't make sense to me. So yes, an implicit cast of an OOB float now warns, but implicit casts of representable floats continues to emit a deprecation while explicit casts of representable floats continue to be silent. |
There was a problem hiding this comment.
RM review: is this new warning meant to replace the existing deprecation? That doesn't appear to have been part of the RFC
It doesn't replace the existing deprecation as this is only for implicit coercions to integer, this is for explicit casts for values that are not representable by a PHP int, and is what is specified in https://wiki.php.net/rfc/warnings-php-8-5#casting_out_of_range_floats_to_int
In the tests (e.g.
Zend/tests/bitwise_not_precision_exception.phpt) the output changes to replace the existing deprecation with a new warning, with no input code change, implying that the deprecation is replaced in at least some cases with a new warningThis is because the bitwise operators do an integer cast internally. And the value that is being cast is out of range, if you replace the value with
25.6you would still get the deprecation rather than a warning. This is also the case for the%operator or any other internal function that does a force cast to int.The operations that emit the deprecation introduced by https://wiki.php.net/rfc/implicit-float-int-deprecate had as a primary intention to prevent floats with fractional parts to be coerced implicitly into an integer as you lose the fractional part. By design this also handles floats that are not representable as ints.
I am struggling to see what the issue here is, this RFC indicates that explicit casts (be that made by the engine or userland) should warn. Requiring that explicit casts warn is a stronger requirement than implicit casts, and having diverging behaviour between implicit and explicit casts doesn't make sense to me.
So yes, an implicit cast of an OOB float now warns, but implicit casts of representable floats continues to emit a deprecation while explicit casts of representable floats continue to be silent.
No issue I guess, I was just confused as to if this was expected because there were existing warnings that were being replaced
RM approval, technical review not performed
…n/max int ranges (xabbuh) This PR was merged into the 6.4 branch. Discussion ---------- [DoctrineBridge][Yaml] don't cast strings exceeding the min/max int ranges | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT see php/php-src#19760 and https://wiki.php.net/rfc/warnings-php-8-5#casting_out_of_range_floats_to_int Commits ------- bac84d1 don't cast strings exceeding the min/max int ranges
4 participants
RFC: https://wiki.php.net/rfc/warnings-php-8-5#casting_out_of_range_floats_to_int