Added fallthrough for == operator with objects for extensions #7973

JordanRL · 2022-01-20T00:25:59Z

This should allow extensions to handle the == operator differently from the <=> operator. ~~It accomplishes this by allowing the do_operation handler to look at ZEND_IS_EQUAL and ZEND_IS_NOT_EQUAL prior to calling zend_compare.~~ It accomplishes this by checking a new equals handler on the object for ZEND_IS_EQUAL and ZEND_IS_NOT_EQUAL prior to calling zend_compare.

cmb69

Thank you for the PR! However, I wonder where handling ==/!= differently from other comparisons would be desireable. Can you give an example use-case?

Zend/zend_operators.c

JordanRL · 2022-01-20T17:52:39Z

Thank you for the PR! However, I wonder where handling ==/!= differently from other comparisons would be desireable. Can you give an example use-case?

In the absence of the operator overloads RFC, I am planning at some point on writing a math extension. This will involve types for matrices and complex numbers, both of which are uncomparable, but can be equivalent.

cmb69 · 2022-01-20T22:46:19Z

Ah, now I see, thanks! But what about introducing a handler which only is called for == and !=? Or maybe a handler, which receives the info whether it is about equality or ordering, that supersedes the compare handler?

JordanRL · 2022-01-21T00:13:01Z

@cmb69 Something like that would have BC implications and require an RFC, wouldn't it? Or am I misunderstanding what you're suggesting?

cmb69 · 2022-01-21T12:41:36Z

@JordanRL, my idea would not break BC; it's basically a new handler which is called, if it is defined; so for classes which don't define the handler, compare is called like before. But in any way, at least some discussion on the internals mailing list appear to be in order.

JordanRL · 2022-01-21T17:44:28Z

@cmb69 alright, I'll switch this to draft.

cmb69 · 2022-01-31T15:30:47Z

@dstogov, @nikic, thoughts about this? Would it require an RFC?

cmb69 · 2022-05-12T17:55:12Z

Anybody interested in this (or has serious objections)?

dstogov · 2022-05-13T09:43:40Z

@dstogov, @nikic, thoughts about this? Would it require an RFC?

This will break ext/gmp and any other extension that implements do_opeartion(). So this definitely needs an RFC.

JordanRL · 2022-05-13T10:24:19Z

This will break ext/gmp and any other extension that implements do_opeartion(). So this definitely needs an RFC.

If a new handler was added instead that would avoid the impact on gmp and other extensions, right? I think that was what @cmb69 was originally suggesting.

JordanRL · 2022-09-14T21:45:36Z

Since the last time this was reviewed, the following changes have been made:

This is no longer accomplished using the do_operation handler. Instead a new (optional) handler has been added: equals
Due to the above change, I don't believe that this should require an RFC any longer, as it should be a completely drop-in replacement that has identical behavior if unused.
It shouldn't reduce performance in any noticeable way for comparisons, as all fast comparisons avoid the added code paths.
A default handler was added. It simply always returns FAILURE and allows the existing zend_compare call to be performed (just with more steps).
Overrides for the compare handler on objects should work as before for == and !=, as this code will fall through to zend_compare which then calls the compare handler if it exists (such as with the DateTime extension).
The useful case is if an extension defines both a compare handler and an equals handler. In that case, the equals handler is called first, and its value for ret is used if the handler also returns SUCCESS. This allows extensions to create objects which are equatable but not comparable (such as complex numbers).

JordanRL · 2022-09-15T20:29:34Z

@dstogov if possible, could you confirm whether the new code for the PR avoids the BC issues you were talking about?

dstogov · 2022-09-19T15:01:23Z

@JordanRL in general the patch is right, but I don't like it.

The new handler solves only specific case - when objects are not ordered, and may be only equal or not equal. However, this is not enough even for comparison of IS_DOUBLE numbers (with NaN). So, later someone will come with new set of object handlers to solve more general problem.

May be it's better to allow zend_compare() to return ZEND_UNCOMPARABLE? Or throw some "UNORDERED" exception from compare handler. The last case doesn't require any engine changes at all.

JordanRL · 2022-09-19T17:55:06Z

@dstogov Fixing ZEND_UNCOMPARABLE is on my list, yeah. The reason I didn't opt for something like what you're talking about to solve this, is that the compare handler is not given an opcode, because it is called from zend_compare which is also not given an opcode.

This makes sense, as zend_compare is called from many locations that don't really have an opcode to pass (such as from the defs for some functions in core).

zend_compare can return ZEND_UNCOMPARABLE. But currently, ZEND_UNCOMPARABLE is defined as 1, which means the arguments must be provided such that zend_compare is evaluated as <=. I think to check for the ZEND_UNCOMPARABLE case, you need to call it twice with reversed arguments, or change the logic so that <= is sufficient.

This presented a large problem for the RFC I worked on for operator overloads, since the order of the operands in the original PHP code needed to be preserved so that the order of competing handlers could be determined accurately, which was the original reason I dived into adding ZEND_IS_GREATER and ZEND_IS_GREATER_OR_EQUAL to the ZEND_AST_BINARY_OP which of course resulted in needing updates to JIT and the OpCache (which I was not qualified to do).

So, I can certainly look at whether I could modify something through that path, but my previous experience made me worried about handling it that way.

dstogov · 2022-09-19T20:20:02Z

Oh. right! My previous post doesn't make a lot of sense :( We can't return ZEND_UNCOMPARABLE or throw UNORDERD exception without knowledge about comparison operators.

JordanRL · 2022-09-19T20:53:49Z

Oh. right! My previous post doesn't make a lot of sense :( We can't return ZEND_UNCOMPARABLE or throw UNORDERD exception without knowledge about comparison operators.

Well, I think that you could do something like throw an exception, but it would involve lots of special conditions to check values and types probably. That would maybe be enough for something like complex numbers or matrices, but I saw value in contributing a more general solution for this sort of thing.

For instance, once this handler exists, maybe it could be used for some more intelligent handling of the $obj == true cases and such. It could make RFCs from others to do something along those lines much easier to accomplish, outside of my goal of allowing uncomparable objects to be equatable.

nikic · 2022-09-19T21:07:14Z

Zend/zend_object_handlers.c

@@ -1941,6 +1941,10 @@ ZEND_API HashTable *zend_get_properties_for(zval *obj, zend_prop_purpose purpose
 	return zend_std_get_properties_for(zobj, purpose);
 }

+ZEND_API zend_result zend_std_equals_objects(zend_uchar opcode, zval *result, zval *op1, zval *op2) {
+	return FAILURE;


Can the fallback to the compare handler be in here?

Do we really need to pass the opcode here? I think it would be better for the return value to indicate whether it equals or not (possibly with an equals/not_equals/uncomparable enum). This would prevent inconsistent == and != implementations by design.

The reason I didn't put the fallback here is that extensions that used this handler would have to manually implement the fallback again (assuming they wanted to use the compare handler). But yeah, if this function returned a result enum instead of zend_result I think the fallback would have to go in here.

That's just very different from how the rest of the handlers are done (from what I've seen), and I wanted this code to be similar to other code in the engine.

The reason I didn't put the fallback here is that extensions that used this handler would have to manually implement the fallback again (assuming they wanted to use the compare handler).

Not sure I get what you mean here -- if they want to use some custom logic and then fall back to the default, they can call zend_std_equals_objects to do so.

Not sure I get what you mean here -- if they want to use some custom logic and then fall back to the default, they can call zend_std_equals_objects to do so.

Or yeah... they could do the obvious thing you just suggested...

Actually though, what about the case where op1 falls back but op2 has custom logic? How would the default handler for op1 handle that?

nikic · 2022-09-19T21:08:23Z

Zend/zend_vm_def.h

@@ -501,7 +501,11 @@ ZEND_VM_HELPER(zend_is_equal_helper, ANY, ANY, zval *op_1, zval *op_2)
 	if (UNEXPECTED(Z_TYPE_INFO_P(op_2) == IS_UNDEF)) {
 		op_2 = ZVAL_UNDEFINED_OP2();
 	}
-	ret = zend_compare(op_1, op_2);
+	if (Z_TYPE_P(op_1) == IS_OBJECT || Z_TYPE_P(op_2) == IS_OBJECT) {
+		ret = zend_equals_object(op_1, op_2, ZEND_IS_EQUAL);


I believe you need a separate zend_equals that is pushed through the entire hierarchy. Otherwise objects nested in arrays will not use the correct handler, for example.

Ah, similar to zend_compare that handles all types and type combinations? I hadn't considered the case of objects in arrays.

Ah, similar to zend_compare that handles all types and type combinations?

Yeah, exactly.

JordanRL added 2 commits January 19, 2022 16:22

Added fallthrough for == operator with objects for extensions

348feb7

Added fallthrough for == operator with objects for extensions

2fa6155

cmb69 reviewed Jan 20, 2022

View reviewed changes

Zend/zend_operators.c Outdated Show resolved Hide resolved

Fixed looping bug in zend_equals_object

2be2077

JordanRL marked this pull request as draft January 21, 2022 17:44

Girgias self-requested a review May 12, 2022 20:08

cmb69 added the RFC label May 13, 2022

JordanRL added 3 commits May 13, 2022 07:04

Merge branch 'php:master' into equals-fallthrough-vm

de984eb

Switching to a new handler based implementation

c6440fa

Fixing broken compile

8a2b6cd

ramsey added Status: Requires RFC Category: Engine labels May 27, 2022

JordanRL marked this pull request as ready for review September 14, 2022 21:45

nikic reviewed Sep 19, 2022

View reviewed changes

JordanRL requested review from dstogov and iluuu1994 as code owners October 7, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added fallthrough for == operator with objects for extensions #7973

Added fallthrough for == operator with objects for extensions #7973

JordanRL commented Jan 20, 2022 •

edited

Loading

cmb69 left a comment

JordanRL commented Jan 20, 2022

cmb69 commented Jan 20, 2022

JordanRL commented Jan 21, 2022

cmb69 commented Jan 21, 2022

JordanRL commented Jan 21, 2022

cmb69 commented Jan 31, 2022

cmb69 commented May 12, 2022

dstogov commented May 13, 2022

JordanRL commented May 13, 2022

JordanRL commented Sep 14, 2022 •

edited

Loading

JordanRL commented Sep 15, 2022

dstogov commented Sep 19, 2022

JordanRL commented Sep 19, 2022

dstogov commented Sep 19, 2022

JordanRL commented Sep 19, 2022 •

edited

Loading

nikic Sep 19, 2022

JordanRL Sep 19, 2022

nikic Sep 19, 2022

JordanRL Sep 19, 2022

JordanRL Sep 19, 2022

nikic Sep 19, 2022

JordanRL Sep 19, 2022

nikic Sep 19, 2022

Added fallthrough for == operator with objects for extensions #7973

Are you sure you want to change the base?

Added fallthrough for == operator with objects for extensions #7973

Conversation

JordanRL commented Jan 20, 2022 • edited Loading

cmb69 left a comment

Choose a reason for hiding this comment

JordanRL commented Jan 20, 2022

cmb69 commented Jan 20, 2022

JordanRL commented Jan 21, 2022

cmb69 commented Jan 21, 2022

JordanRL commented Jan 21, 2022

cmb69 commented Jan 31, 2022

cmb69 commented May 12, 2022

dstogov commented May 13, 2022

JordanRL commented May 13, 2022

JordanRL commented Sep 14, 2022 • edited Loading

JordanRL commented Sep 15, 2022

dstogov commented Sep 19, 2022

JordanRL commented Sep 19, 2022

dstogov commented Sep 19, 2022

JordanRL commented Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordanRL commented Jan 20, 2022 •

edited

Loading

JordanRL commented Sep 14, 2022 •

edited

Loading

JordanRL commented Sep 19, 2022 •

edited

Loading