Zend: Add zend_try_get_double

[LamentXU123]

See php/php-tasks#32. Given now we have corresponding try functions to almost every zval_get_TYPE functions except double, it is reasonable to add zend_try_get_double. A quick search shows that there are 62 occasions of zend_get_double in the code base.

A real-world bug example is in #23384 (comment)_

cc @Girgias

I almost copy-paste the implementation of zend_try_get_long for this function.

[devnexen]

that s a nice addition !

[Girgias]

We don't check this for zval_get_double, and I don't think we should do this here either. If you hand an UNDEF zval there are bigger issues at hand that need to be investigated.

[LamentXU123]

I manually add this case :| But this makes debug easier so sure.

[Girgias]

It seems the reason we do this in the other ones is for VM optimizations (which probably would be good to add a comment using the commit description as to why IS_UNDEF is checked there)

[LamentXU123]

I will also add a comment in code to make it clear.

[Girgias]

We shouldn't be reading an uninitialized typed property, that's a bug in the calling code.

[LamentXU123]

Okay I take some hour to learn this stuff. AFAIK it is reasonable to remove IS_UNDEF. I don't know anything about VM optimizations, but I can tell that this:

case IS_UNDEF:
    *failed = true;
    return 0.0;

Is completely the wrong semantic. IS_UNDEF should not be considered as 0.0. This

a. makes debugging harder cuz this is hiding the bug and makes the code base even more unpredictable.
b. makes no sense because IS_UNDEF isn't any value and shouldn't be considered as 0 anyways.

Instead if we remove this logic, this falls into

default:
    ZEND_UNREACHABLE();

That immediately catch the bug. And is more nicer semantically because trying to turn it into a double value is indeed unreachable behavior.

I know we have *failed = true. But IMO this is more like indicating a error when turning the value. I think if you are trying to get a double from an UNDEF type, the problem is way more serious than that and errors need to be thrown here.

Also the bug you've mentioned make sense too. This isn't correct logic anyways.