[Request][Discussion] Double value as array key improvement

> > Sounds good to me. Something you might want to consider is to also throw a > warning if the floating point number is not an exact integer. For example > allow a silent cast of 42.0 to 42, but throw a warning if 42.5 is used as > an index (or worse, 42.999999999, in which case it likely isn't doing what > the programmer thinks it's doing).

> On Aug 19, 2017, at 4:30 PM, Nikita Popov <nikita.ppv@gmail.com> wrote: > > On Thu, Aug 17, 2017 at 5:03 PM, Andrew Nester <andrew.nester.dev@gmail.com <mailto:andrew.nester.dev@gmail.com>> wrote: > > > > 13 авг. 2017 г., в 21:39, Andrew Nester <andrew.nester.dev@gmail.com <mailto:andrew.nester.dev@gmail.com>> написал(а): > > > > > > > >> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru <mailto:newaltgroup@bk.ru>> написал(а): > >> > >> > >>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru <mailto:newaltgroup@bk.ru>> wrote: > >>> > >>> Hello everyone! > >>> > >>> I was working on following request https://bugs.php.net/bug.php?id=75053 <https://bugs.php.net/bug.php?id=75053> which resulted in following pull request https://github.com/php/php-src/pull/2676 <https://github.com/php/php-src/pull/2676> > >>> > >>> The problem here is following: when we’re using large numbers as array index when adding new elements it could overwrite already existing value. > >>> Assume we have 2 indexes 5076964154930102272 and 999999999999999999999999999999 with different value set for them. > >>> > >>> Because 999999999999999999999999999999 is larger than maximum long int number for 64-bit systems, it will be converted to double. (corresponding code here https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648 <https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648>) > >>> But when double value is used as array indexes, it is converted to long integer. (f.e., code is here https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573 <https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573>) > >>> At this case it causes overflow and we’ve got index equal to 5076964154930102272 and as a result - we’re overwriting previously set value. > >>> > >>> My suggestion is following: > >>> 1) when double key is less than maximum possible long integer - convert it to integer > >>> 2) if it’s larger - convert it to string. > >>> > >>> That’s what implemented in proposed PR. > >>> > >>> Another possible option is just to throw warning in this case (proposed by Nikita Popov) > >>> > >>> I would happy to hear any feedback and suggestions about this solution. > >>> Thanks! > >> > >> Here is the alternative solution which emits E_WARNING in case of integer array index overflow. > >> https://github.com/php/php-src/pull/2677 <https://github.com/php/php-src/pull/2677> > > > > My preferred solution is 2nd one (emitting warning) as it more obvious for users, doesn't break previous behaviour. > > > > Cheers, > > Andrew > > Hello internals! > > I was working on solution for the problem of double to int conversion for array indices and would like to create an RFC for proposed solution - emitting warning when integer overflow happens during double to int conversion. > > Does it look like good idea? > > Thanks! > > Sounds good to me. Something you might want to consider is to also throw a warning if the floating point number is not an exact integer. For example allow a silent cast of 42.0 to 42, but throw a warning if 42.5 is used as an index (or worse, 42.999999999, in which case it likely isn't doing what the programmer thinks it's doing). > > Nikita >

> Hi there, > > Andrew Nester wrote: > > > > > > Hello internals! > > > > I was working on solution for the problem of double to int conversion > for array indices and would like to create an RFC for proposed solution - > emitting warning when integer overflow happens during double to int > conversion. > > > > Does it look like good idea? > > > > Thanks! > > > > It's certainly an improvement over silently truncate-wrapping like at > present. > > But one question I might have is whether there's any benefit to it just > being a warning rather than an Error. Surely nobody would rely on this > behaviour? I can't imagine a use-case where it doing % 2^64 on your > indices is helpful. Who would want the code execution to continue beyond > that point? Will someone want to silence this error? > > Compare how the `int` type declaration on userland functions handles > floats like these: it throws a TypeError. Internal functions *do* throw > a warning, but they don't execute their body and return NULL instead > (they do throw a TypeError in strict mode). > > If you threw some kind of Error instead, I can't imagine it causing > backwards-compatibility problems in practice, and at least personally, I > think it would be the better choice. But I wrote the RFC that made > passing floats > PHP_INT_MAX or < PHP_INT_MIN to functions with integer > parameters produce those errors, so of course I'd say that… > > Anyway, thanks for bringing this up! It's something that I've wanted to > fix since a long time ago and had forgotten about. >