Fix enum to bool comparison

[iluuu1994]

The compiler compiles $enum == true to ZEND_BOOL, which always returns true for objects (with the default cast_object handler). However, when compared to a statically unknown value $enum == $true, the resulting opcode ZEND_IS_EQUAL would call the objects compare handler.

The zend_objects_not_comparable() handler, which is installed for enums and other internal classes, blanketly returns false. This does not match the ZEND_BOOL semantics.

We now handle bools in zend_objects_not_comparable() explicitly.

Fixes GH-16954

[iluuu1994]

@IMSoP I decided only to handle bool for now. IMO, handling string would also be reasonable, but it's not something I would like to do in supported branches. I also think it may be best to discuss it on the list, as it is not as obvious of a bug as the bool case.

[dstogov]

This affects many internal classes.
I think this change is too dangerous especially for stable branch.

May be this should be done for enums only?
I don't see a lot of sense in comparison bools with objects, that can't be cast to bool.

[iluuu1994]

@dstogov $value == true is compiled to ZEND_BOOL, which does a cast_object to bool. However, $value == $true gets compiled to ZEND_IS_EQUAL, which calls the compare handler that by default (zend_std_cast_object_tostring) also calls cast_object. However, with zend_objects_not_comparable(), only the ZEND_IS_EQUAL case returns false in all cases, while ZEND_BOOL continues to work normally. Having these two cases diverge is very confusing.

I don't see a lot of sense in comparison bools with objects, that can't be cast to bool.

None of our extensions prevent casting to bool. It's just ZEND_IS_EQUAL that fails. Failing when casting to bool would be very questionable IMO, and already error with a very rare E_RECOVERABLE_ERROR when passing the value to an if, for example. The exact issue here is that the behavior of (bool) $value and if ($value) is not the same as $value == true, which should IMO be semantically equivalent.

I think this change is too dangerous especially for stable branch.

I'm happy merging this for master only, but I do think that the proposed behavior is the correct one. If you like, I can start a discussion on the list.

[dstogov]

I don't care about this a lot and I won't object against merging this into PHP-8.3.
Just discuss this with some one else to avoid mistake.

[iluuu1994]

@bwoebi @arnaud-lb @nielsdos Maybe one of you has some opinions. 🙂

[nielsdos]

I think the proposed behaviour is right, but I'm a bit worried as well for stable branches.
People depend on all sorts of strange behaviour, either knowingly or unknowingly.
This could in theory also affect 3rd party extensions, but probably not in practice.

[iluuu1994]

I'm happy postponing for master and documenting the change accordingly.

[arnaud-lb]

The proposed behavior also looks good to me as it's consistent with the current behavior when the boolean operand is a literal. It also matches the documented behavior of bool == anything, which is to cast the non-bool operand to bool before comparison, as this is exactly what the change does.

One possible concern is that having a special case in zend_objects_not_comparable() can be surprising. Given the definition of true == anything, this could be fixed in zend_compare() instead, by calling the cast handler there. Or we could rename zend_objects_not_comparable().

I agree that postponing for master is less risky.

[bwoebi]

I agree with the proposed behaviour. I think it would be fine to change in PHP 8.4 given its recency, but 8.3 is probably too risky.

[Girgias]

If this is merged into 8.4 please amend the migration guide and/or open an issue on doc-en.

[iluuu1994]

@arnaud-lb I added a new commit implementing your suggestion. In synthetic benchmarks, object comparison gets slightly slower, while other comparisons (that don't go through the fast path) get slightly faster (e.g. numeric string to int comparison). My intuition is that the latter is more common, so I'm happy going with your solution.

[arnaud-lb]

Thank you! This looks good to me.