Fix #63217: Constant numeric strings become integers when used as ArrayAccess offset

[rtheunissen]

See:

• https://bugs.php.net/bug.php?id=63217
• https://3v4l.org/dagUP
• https://externals.io/thread/1005

This PR is an extension and re-opening of #1649 in an attempt to fix this as part of 7.2. I've rebased on master and updated some tests that are failing. @nikic mentioned in #1649 that further changes are necessary but I'm not sure what they are or why they are necessary.

This patch does not affect the array or string API and behaviour at all - only ArrayAccess.

[rtheunissen]

@hikari-no-yume was the optimised branch the better implementation or is this the correct approach?

[hikari-no-yume]

@rtheunissen I may be remembering wrong, but I think I failed to make a properly-working optimisation.

[hikari-no-yume]

It would be ideal to somehow retain the optimisation that fixing the bug would otherwise remove, but I haven't thought of a good way to do that yet.

[rtheunissen]

@hikari-no-yume Was the existing optimisation that caused this bug simply to remove the need to use ZEND_HANDLE_NUMERIC if it already knew it didn't have to?

[hikari-no-yume]

@rtheunissen By performing ZEND_HANDLE_NUMERIC ahead of time, it wouldn't need to convert numeric strings at the point of execution or check if it needed to do so, yes.

[rtheunissen]

If this implementation fixes the bug, passes the tests, and doesn't affect performance much, it would be good to have this under 7.2 and be optimised in a minor patch later on.

[hikari-no-yume]

Yeah. Potentially a more specific optimisation could be done in Zend Optimizer.

[rtheunissen]

@nikic what were the changes you were referring to in zend_inference.c? Can we write a failing test for that or is it internal only?

[nikic]

@rtheunissen What needs be handled are cases like https://github.com/rtheunissen/php-src/blob/e67dd43f66832170575bd148b5087a95d402f105/ext/opcache/Optimizer/zend_inference.c#L1977. Currently the code assumes that if the array key is a constant string, then this string is non-numeric.

The following test case might reproduce the issue by incorrectly eliding a return type check, but I did not test. It's easiest to just check if the inference results are correct using opcache.opt_debug_level.

<?php

function test() : string {
    $array = [];
    $array["10"] = 42;
    foreach ($array as $k => $v) {
        return $k;
    }
}
test();

[rtheunissen]

@nikic I couldn't get that test case to fail. I tried some variations of it but I got the expected result in every case. Would be 💯 if you could provide a failing case for it.

I forgot to enable opcache, test is failing now, thanks @nikic.

[rtheunissen]

@nikic see 4cec140, not sure if that's correct though.

[nikic]

@rtheunissen It technically fixes the problem, but in a non-optimal way. It would be preferable to add a check whether or not the constant operand is numeric, rather than just assuming that it is. (This might be something you might want to do in general with a flag on the opline to also avoid the numeric string check at runtime in the VM as well).

Furthermore iirc there are a couple of other places where basically the same check is done in zend_inference, so there are probably some more code changes necessary.

[rtheunissen]

@nikic something like that Andrea tried here? rtheunissen@40e393e

[rtheunissen]

If we can technically fix the problem, even if it's not in an optimal way, shouldn't that be a solid place to start with a 7.2.0 for example? 7.2.1 can see the optimal solution implemented. I doubt that the current patch's direction will affect performance enough to be a deal-breaker.

I'll do some benchmarks similar to what @hikari-no-yume did in the initial request.

[rtheunissen]

See #3351