Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] Stricter type-checks for arithmetic/bitwise operators »

[RFC] Stricter type-checks for arithmetic/bitwise operators

php.internals

Nikita Popov

6 years ago
Hi internals, I would like to propose making the use of arithmetic/bitwise operators on arrays, resources and (non-overloaded) objects a TypeError exception: https://wiki.php.net/rfc/arithmetic_operator_type_checks This is inspired by some of the recent discussions, in particular the operator overloading RFC (where the fact that operations on non-overloaded objects still succeed with just a notice was criticized) and the increment/decrement RFC, which handles the array case of this proposal for inc/dec only. I think as-is, this RFC should be completely uncontroversial. However, there is an open question on whether we want to make this slightly more strict, in order to align the semantics with the rules for weak parameter type checks for ints/floats. If we do that, then this RFC could be a stepping stone towards making "implicit" internal casts use the (weak) parameter type checking semantics more generally, which I think would be a good idea. The current explicit cast semantics we use everywhere are too forgiving for most circumstances (e.g. an array is almost always not a reasonable input where an integer is expected). Regards, Nikita

Christoph Becker

6 years ago
On 02.04.2020 at 10:14, Nikita Popov wrote:
> I would like to propose making the use of arithmetic/bitwise operators on > arrays, resources and (non-overloaded) objects a TypeError exception: > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > This is inspired by some of the recent discussions, in particular the > operator overloading RFC (where the fact that operations on non-overloaded > objects still succeed with just a notice was criticized) and the > increment/decrement RFC, which handles the array case of this proposal for > inc/dec only. > > I think as-is, this RFC should be completely uncontroversial. However, > there is an open question on whether we want to make this slightly more > strict, in order to align the semantics with the rules for weak parameter > type checks for ints/floats. > > If we do that, then this RFC could be a stepping stone towards making > "implicit" internal casts use the (weak) parameter type checking semantics > more generally, which I think would be a good idea. The current explicit > cast semantics we use everywhere are too forgiving for most circumstances > (e.g. an array is almost always not a reasonable input where an integer is > expected).
Thanks! Generally I like that very much. I'm not quite sure, though, what to do with resource to int conversions. These are documented to be valid[1], and are sometimes deliberately used[2], and as such it might be reasonable not to throw on these conversions (maybe even change the current behavior of the ~ operator). After all, I hope we will be able to port all resources to objects sometime, and till then we could stick with more lax semantics. [1] <https://www.php.net/manual/en/language.types.integer.php> [2] <https://www.php.net/manual/en/function.ssh2-sftp.php#refsect1-function.ssh2-sftp-examples>
-- Christoph M. Becker

Nikita Popov

6 years ago
On Thu, Apr 2, 2020 at 10:35 AM Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 02.04.2020 at 10:14, Nikita Popov wrote: > > > I would like to propose making the use of arithmetic/bitwise operators on > > arrays, resources and (non-overloaded) objects a TypeError exception: > > > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > > > This is inspired by some of the recent discussions, in particular the > > operator overloading RFC (where the fact that operations on > non-overloaded > > objects still succeed with just a notice was criticized) and the > > increment/decrement RFC, which handles the array case of this proposal > for > > inc/dec only. > > > > I think as-is, this RFC should be completely uncontroversial. However, > > there is an open question on whether we want to make this slightly more > > strict, in order to align the semantics with the rules for weak parameter > > type checks for ints/floats. > > > > If we do that, then this RFC could be a stepping stone towards making > > "implicit" internal casts use the (weak) parameter type checking > semantics > > more generally, which I think would be a good idea. The current explicit > > cast semantics we use everywhere are too forgiving for most circumstances > > (e.g. an array is almost always not a reasonable input where an integer > is > > expected). > > Thanks! Generally I like that very much. I'm not quite sure, though, > what to do with resource to int conversions. These are documented to be > valid[1], and are sometimes deliberately used[2], and as such it might > be reasonable not to throw on these conversions (maybe even change the > current behavior of the ~ operator). >
While resource to int conversions are indeed valid, they are not exactly commonly used, and if they are used, I would expect an explicit (int) cast to be involved. I very seriously doubt that someone writing "$x = $y / 2" really intended this to mean "$x = resource_id($y) / 2" -- it is much more likely that a variable go mixed up. In the unlikely case that that they really did intend that, the option of writing "$resourceId = (int) $y; $x = $resourceId / 2" remains, and is much clearer. On that note, I do wonder whether we should introduce a function like get_resource_id(), that makes it more explicit that an (int) cast is supposed to fetch a resource ID. This is not a very common operation, and I suspect that many PHP programmers may not be familiar with the semantics of integer casts on resources. Using get_resource_id($file) would make the intent clearer relative to (int) $file. This is similar to the recently added get_mangled_object_vars() function, which essentially does the same as (array) $object, but makes it obvious that you really are interested in mangled properties.
> After all, I hope we will be able > to port all resources to objects sometime, and till then we could stick > with more lax semantics.
I think that's really an argument to align the behavior of resources and objects, as it means one less BC break when resources are converted to objects. Historically that's something we did in minor versions, so it's good to reduce the amount of semantics that change when such a conversion is done. Regards, Nikita

Christoph Becker

6 years ago
On 02.04.2020 at 10:47, Nikita Popov wrote:
> On Thu, Apr 2, 2020 at 10:35 AM Christoph M. Becker <cmbecker69@gmx.de> > wrote: > >> On 02.04.2020 at 10:14, Nikita Popov wrote: >> >>> I would like to propose making the use of arithmetic/bitwise operators on >>> arrays, resources and (non-overloaded) objects a TypeError exception: >>> >>> https://wiki.php.net/rfc/arithmetic_operator_type_checks >> >> Thanks! Generally I like that very much. I'm not quite sure, though, >> what to do with resource to int conversions. These are documented to be >> valid[1], and are sometimes deliberately used[2], and as such it might >> be reasonable not to throw on these conversions (maybe even change the >> current behavior of the ~ operator). > > While resource to int conversions are indeed valid, they are not exactly > commonly used, and if they are used, I would expect an explicit (int) cast > to be involved. I very seriously doubt that someone writing "$x = $y / 2" > really intended this to mean "$x = resource_id($y) / 2" -- it is much more > likely that a variable go mixed up. In the unlikely case that that they > really did intend that, the option of writing "$resourceId = (int) $y; $x = > $resourceId / 2" remains, and is much clearer. > > On that note, I do wonder whether we should introduce a function like > get_resource_id(), that makes it more explicit that an (int) cast is > supposed to fetch a resource ID. This is not a very common operation, and I > suspect that many PHP programmers may not be familiar with the semantics of > integer casts on resources. Using get_resource_id($file) would make the > intent clearer relative to (int) $file. This is similar to the recently > added get_mangled_object_vars() function, which essentially does the same > as (array) $object, but makes it obvious that you really are interested in > mangled properties.
I think something like get_resource_id() would be a good addition.
>> After all, I hope we will be able >> to port all resources to objects sometime, and till then we could stick >> with more lax semantics. > > I think that's really an argument to align the behavior of resources and > objects, as it means one less BC break when resources are converted to > objects. Historically that's something we did in minor versions, so it's > good to reduce the amount of semantics that change when such a conversion > is done.
Good point indeed! So +1 from me on the RFC. Not absolutely sure about the open questing regarding going one step further, but tentatively also +1 on that. Thanks, Christoph

Girgias

6 years ago
On Thu, 2 Apr 2020 at 10:14, Nikita Popov <nikita.ppv@gmail.com> wrote:
> Hi internals, > > I would like to propose making the use of arithmetic/bitwise operators on > arrays, resources and (non-overloaded) objects a TypeError exception: > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > This is inspired by some of the recent discussions, in particular the > operator overloading RFC (where the fact that operations on non-overloaded > objects still succeed with just a notice was criticized) and the > increment/decrement RFC, which handles the array case of this proposal for > inc/dec only. > > I think as-is, this RFC should be completely uncontroversial. However, > there is an open question on whether we want to make this slightly more > strict, in order to align the semantics with the rules for weak parameter > type checks for ints/floats. >
On this topic maybe it would be good time to bring back again the Saner string to number comparison RFC, at least in some form? https://wiki.php.net/rfc/string_to_number_comparison Or is this open question meant to replace it? And maybe in some other form revive this RFC about allowing trailing white-spaces in numeric string, because IMHO only accepting those which have white-spaces in front is kinda weird.
> If we do that, then this RFC could be a stepping stone towards making > "implicit" internal casts use the (weak) parameter type checking semantics > more generally, which I think would be a good idea. The current explicit > cast semantics we use everywhere are too forgiving for most circumstances > (e.g. an array is almost always not a reasonable input where an integer is > expected). > > Regards, > Nikita >
Other than this a solid +1 from me. George P. Banyard

Andrew Faulds

6 years ago
Hi Nikita, Nikita Popov wrote:
> Hi internals, > > I would like to propose making the use of arithmetic/bitwise operators on > arrays, resources and (non-overloaded) objects a TypeError exception: > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > This is inspired by some of the recent discussions, in particular the > operator overloading RFC (where the fact that operations on non-overloaded > objects still succeed with just a notice was criticized) and the > increment/decrement RFC, which handles the array case of this proposal for > inc/dec only.
Sounds good!
> I think as-is, this RFC should be completely uncontroversial. However, > there is an open question on whether we want to make this slightly more > strict, in order to align the semantics with the rules for weak parameter > type checks for ints/floats.
Regarding that, the RFC says we could “Make non-numeric string operands throwing. Non-numeric here means not starting with a digit (optionally preceded by whitespace). This would not apply to operators that have special behavior for strings, such as string increment.” I know you used “such as” there, but maybe you should specifically point out the bitwise case. I think increment and the bitwise operators are the only special cases for strings?
> If we do that, then this RFC could be a stepping stone towards making > "implicit" internal casts use the (weak) parameter type checking semantics > more generally, which I think would be a good idea. The current explicit > cast semantics we use everywhere are too forgiving for most circumstances > (e.g. an array is almost always not a reasonable input where an integer is > expected).
That would be nice! We currently have essentially three (actually more but these are the big ones) different “weak” behaviours: function arguments and return values, non-comparison operators, and comparisons, and I doubt people who don't follow internals are fully aware of the differences, so it would be good to eventually have just one set of rules. I guess https://wiki.php.net/rfc/invalid_strings_in_arithmetic was already a step in this direction and your RFC is a further development of it. Thanks, Andrea

Rowan Collins

6 years ago
Hi Nikita, On Thu, 2 Apr 2020 at 09:14, Nikita Popov <nikita.ppv@gmail.com> wrote:
> I would like to propose making the use of arithmetic/bitwise operators on > arrays, resources and (non-overloaded) objects a TypeError exception: > > https://wiki.php.net/rfc/arithmetic_operator_type_checks >
Thanks for writing this up; one of the conclusions when revising my inc/dec RFC was that this should be proposed, but I've not had the energy to follow through. Discovering that objects become int(1) was a big WTF for me. I'd happily see that throw an error even under an explicit cast - "(string)new class{}" is currently an Error, but "(int)new class{}" and "(float)new class{}" are only a Notice.Would it be possible to throw an Error in this case without fixing the comparison operator quirk you noted in rfc/engine_warnings? I initially thought resources made sense as they are, but like you I concluded that the only real use is to get the ID itself, so explicit casts are enough. There's a possibility that someone used to JS might write $resource+0 instead of (int)$resource out of habit, but it doesn't seem particularly likely, and is easy to fix. get_resource_id() is a good idea, too; for similar reasons, I've often wished objects with __toString() aliased it to a more specific method, rather than it being the only way to get a certain representation. While the behaviour of other types such as strings would be nice to revisit, I think it's worth keeping this RFC to arrays, objects, and resources, because other cases have a lot more to consider in terms of detail and backward compatibility impact. Regards,
-- Rowan Tommins [IMSoP]

Nikita Popov

6 years ago
On Thu, Apr 2, 2020 at 3:11 PM Rowan Tommins <rowan.collins@gmail.com> wrote:
> Hi Nikita, > > On Thu, 2 Apr 2020 at 09:14, Nikita Popov <nikita.ppv@gmail.com> wrote: > > > I would like to propose making the use of arithmetic/bitwise operators on > > arrays, resources and (non-overloaded) objects a TypeError exception: > > > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > > > > Thanks for writing this up; one of the conclusions when revising my inc/dec > RFC was that this should be proposed, but I've not had the energy to follow > through. > > Discovering that objects become int(1) was a big WTF for me. I'd happily > see that throw an error even under an explicit cast - "(string)new class{}" > is currently an Error, but "(int)new class{}" and "(float)new class{}" are > only a Notice.Would it be possible to throw an Error in this case without > fixing the comparison operator quirk you noted in rfc/engine_warnings? >
In preparation for this change, I've decoupled the error reporting from the cast logic, so it is now possible to make just the cast throw, without influencing comparison. However, there might be exception safety concerns, along the same lines as https://wiki.php.net/rfc/tostring_exceptions for the string cast case. Nikita

Nikita Popov

6 years ago
On Thu, Apr 2, 2020 at 3:11 PM Rowan Tommins <rowan.collins@gmail.com> wrote:
> Hi Nikita, > > On Thu, 2 Apr 2020 at 09:14, Nikita Popov <nikita.ppv@gmail.com> wrote: > > > I would like to propose making the use of arithmetic/bitwise operators on > > arrays, resources and (non-overloaded) objects a TypeError exception: > > > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > > > > Thanks for writing this up; one of the conclusions when revising my inc/dec > RFC was that this should be proposed, but I've not had the energy to follow > through. > > Discovering that objects become int(1) was a big WTF for me. I'd happily > see that throw an error even under an explicit cast - "(string)new class{}" > is currently an Error, but "(int)new class{}" and "(float)new class{}" are > only a Notice.Would it be possible to throw an Error in this case without > fixing the comparison operator quirk you noted in rfc/engine_warnings? > > I initially thought resources made sense as they are, but like you I > concluded that the only real use is to get the ID itself, so explicit casts > are enough. There's a possibility that someone used to JS might write > $resource+0 instead of (int)$resource out of habit, but it doesn't seem > particularly likely, and is easy to fix. get_resource_id() is a good idea, > too; for similar reasons, I've often wished objects with __toString() > aliased it to a more specific method, rather than it being the only way to > get a certain representation. > > While the behaviour of other types such as strings would be nice to > revisit, I think it's worth keeping this RFC to arrays, objects, and > resources, because other cases have a lot more to consider in terms of > detail and backward compatibility impact. >
I've now update the RFC to move the open question into future scope, as I agree that this is a larger topic that should be discussed separately. I plan to open voting on this RFC in a couple of days, if there are no further concerns. Nikita

Terje Slettebø

6 years ago
Nikita Popov wrote:
> I would like to propose making the use of arithmetic/bitwise operators > on arrays, resources and (non-overloaded) objects a TypeError > exception: > > https://wiki.php.net/rfc/arithmetic_operator_type_checks > > This is inspired by some of the recent discussions, in particular the > operator overloading RFC (where the fact that operations on > non-overloaded objects still succeed with just a notice was > criticized) and the increment/decrement RFC, which handles the array > case of this proposal for inc/dec only. > > I think as-is, this RFC should be completely uncontroversial. However, > there is an open question on whether we want to make this slightly > more strict, in order to align the semantics with the rules for weak > parameter type checks for ints/floats. > > If we do that, then this RFC could be a stepping stone towards making > "implicit" internal casts use the (weak) parameter type checking > semantics more generally, which I think would be a good idea. The > current explicit cast semantics we use everywhere are too forgiving > for most circumstances (e.g. an array is almost always not a > reasonable input where an integer is expected).
I definitely support this proposal, and I'd love the "strict_operators" directive as well, but that's a different proposal, and a different discussion. The current proposal seems, in the words of the proposal, to be pretty uncontroversial: It basically corrects a bunch of WTFs in PHP's type system. Regards, Terje