Inconsistency between (bool)$foo and $foo==true for enums and other "uncomparable" objects

[IMSoP]

Description

Problem Description

For enums and several internal object types, casting to boolean yields true, but comparing against true (with ==) does not.

This leads to some very confusing inconsistencies, as user "Crapulam" pointed out on Reddit:

enum TestEnum
{
    case A;
}

$truthy = true;

var_dump(TestEnum::A == true); // bool(true)
var_dump(TestEnum::A == $truthy); // bool(false)

Demo: https://3v4l.org/OOTcA

What's happening is that the compiler has a special case to treat $foo == true as (bool)$foo, so ends up running the object's cast_object handler instead of the compare handler. The enum implementation has inconsistent handler definitions, leading to the wonky result.

Note that this is not unique to enums, but happens for various other objects, e.g. XMLParser: https://3v4l.org/ZqWRK

Analysis

The base class entry for enums sets the compare handler to zend_objects_not_comparable, causing all comparisons to be false; but it leaves other handlers, including cast_object at default:

php-src/Zend/zend_enum.c

Lines 167 to 169 in be27cbd

	memcpy(&zend_enum_object_handlers, &std_object_handlers, sizeof(zend_object_handlers));
	zend_enum_object_handlers.clone_obj = NULL;
	zend_enum_object_handlers.compare = zend_objects_not_comparable;

That leaves the cast handler as zend_std_cast_object_tostring, which handles checking for __toString, but also treats all objects as true when cast or coerced to boolean:

php-src/Zend/zend_object_handlers.c

Lines 1745 to 1747 in cb9785a

	case _IS_BOOL:
	ZVAL_TRUE(writeobj);
	return SUCCESS;

The zend_objects_not_comparable handler doesn't have any logic at all:

php-src/Zend/zend_object_handlers.c

Lines 1611 to 1614 in cb9785a

	ZEND_API int zend_objects_not_comparable(zval *o1, zval *o2)
	{
	return ZEND_UNCOMPARABLE;
	}

That means it skips the normal logic in zend_std_compare_objects which would call the cast handler when comparing to a non-object:

php-src/Zend/zend_object_handlers.c

Lines 1493 to 1495 in cb9785a

	if (Z_TYPE_P(o1) != Z_TYPE_P(o2)) {
	/* Object and non-object */
	zval casted;

As a final piece of the puzzle, this special case was first added by Nikita in 8.0.3 to fix a regression caused by resource-to-object conversion on sockets: cb9785a It's now used for a bunch of different "opaque" objects.

The specific requirement was that two different sockets should not compare equal with ==

Suggested Fix

Theoretically, we could keep the current comparison, and fix the cast behaviour to match. This would be straight-forward for enums, because they explicitly forbid __toString, so could just error for all casts. However, this would be a rather visible BC break, because if ( $foo ) is frequently used as a lazy emptiness test.

Probably the safer way around is to fix the comparison, by making the compare handler for some or all "non-comparable" objects still use the standard casting logic when comparing to non-objects.

My gut feel is that we could just expand the zend_objects_not_comparable callback to share the "object and non-object" block from the top of zend_std_compare_objects.

That's still a small compatibility break if someone is relying on the current behaviour for any of the affected objects. Probably safe for 8.5, but not 8.4.x

PHP Version

PHP 8.0.3 onwards

Operating System

No response

[bwoebi]

I think the proper fix for this would be defaulting to the zend_is_true handling whenever objects are to be compared against booleans.

[iluuu1994]

I think the proper fix for this would be defaulting to the zend_is_true handling whenever objects are to be compared against booleans.

I agree. zend_objects_not_comparable() means any two instances of the class are not comparable. Truthy/falsy semantics should not change. @IMSoP Would you like to send a PR? If not, I can do so.

[IMSoP]

@iluuu1994 Please go ahead; the two minor questions in my mind are:

1. Should we just fix booleans, or also include the __toString handling from the standard handler? I suspect most if not all "incomparable" classes are also final (and enums explicitly block __toString), but there might be some edge case that comes back to bite us.
2. Is this safe to merge as a bug fix to all active branches, or is it a behaviour change which will have to wait for 8.5?

[iluuu1994]

@IMSoP Handling of string is fair, I think there's not much reason to diverge from the default behavior either, if implicit coercion to string happens in other places anyway. Another inconsistency is that comparable objects produce warnings when compared to int or double, while uncomparable objects confusingly does not.

https://3v4l.org/Em7ni

enum E {
    case C;
}

class C {}
$c = new C();

var_dump(E::C < 42);
var_dump(E::C > 42);
var_dump($c < 42);
var_dump($c > 42);

bool(false)
bool(false)

Notice: Object of class C could not be converted to int
bool(true)

Notice: Object of class C could not be converted to int
bool(false)

This seems backwards. Since we're always returning false and operands(+operation) become non-commutative, a warning seems reasonable.

I wonder if the most consistent thing would be to simply return false when both values are objects, and fall back to zend_std_compare_objects() for everything else.

[iluuu1994]

Oh, and the same goes for ==. Classes get a warning when compared to ints, indicating that this condition will never succeed. Enums do not, where maybe a warning would be more useful, as people may expect them to be equal to their backing values. https://3v4l.org/sXIDj Edit: That said, we should definitely continue to avoid E::C == 1 for enums.

*Edit 2: To summarize:

• null has special-handling in zend_compare(), it always works as expected.
• bools are normally handled by zend_std_compare_objects() (and by extension zend_std_cast_object_tostring()). Setting compare to zend_objects_not_comparable() breaks, as reported by this issue.
• int are normally handled by zend_std_compare_objects() and (and by extension zend_std_cast_object_tostring()). However, zend_std_cast_object_tostring() does not have a default-implementation. When there is no custom implementation by an extension, zend_std_compare_objects() produces a warning, and the other value is compared to 1. With zend_objects_not_comparable(), the warning goes away and the result is always false.
• float works the same as int.
• string works the same as int, except that no warning is emitted when the object is not convertible to string.
• array works the same string, except that there is no default implementation ((array) $object has special-handling in the VM).
• object is true if both objects are of the same class, and all properties are equal, by default.
• resource technically works the same as array. That said, there is no (resource) cast so it's unlikely anybody even implements this.

Honestly, I dislike the idea that == can result in warnings at all. Generally, it seems two arbitrary things should be comparable without fearing warnings. The same does not go for < for obvious reasons (because order should be transitive, which is almost impossible to achieve for unrelated, arbitrary values). Anyway, not something to change here.

There are two possible approaches:

• Add special handling for bool, string, and possibly int/float to reintroduce the warning but still returning false, while keeping array and resource unsupported.
• Falling back to zend_std_compare_objects in all cases except object, and int/float to avoid $enum == 1.