Elide ZEND_VERIFY_RETURN_TYPE when returning $this#22335
Conversation
github-actions
Bot
added
Category: Engine
Extension: opcache
Category: Optimizer
labels
|
|
||
| if (ce1->num_interfaces) { | ||
| uint32_t i; | ||
| if (ce1->ce_flags & ZEND_ACC_RESOLVED_INTERFACES) { |
There was a problem hiding this comment.
Status note for reviewers, carrying over @Girgias's "not sure how to test we hit
this case" and @arnaud-lb's MAY_BE_THIS suggestion from #21948.
This generalized safe_instanceof() is currently not reached for return $this.
can_elide_return_type_check() only continues when use_info->ce is set, and inference assigns no ce to $this (there is no MAY_BE_THIS), so for return $this we bail before ever calling can_elide_list_type() / safe_instanceof(). That's why the inherited_interface* tests still show VERIFY_RETURN_TYPE after the optimizer.
I also couldn't reach the unlinked branches via a non-$this value: when inference knows the operand's class it's already ZEND_ACC_LINKED (so the instanceof_function() fast path is taken), and when the class is unlinked/conditional use_info->ce is unset and we bail first. So this RESOLVED_INTERFACES recursion in particular is currently unexercised.
Making the optimizer side actually elide the inherited-interface $this cases needs the MAY_BE_THIS inference @arnaud-lb suggested, so the $this SSA var carries the scope ce.
There was a problem hiding this comment.
Hm, MAY_BE_THIS is actually already there, the problem seems to be class resolution, not inference.
There was a problem hiding this comment.
Confirmed, FETCH_THIS already sets use_info->ce = op_array->scope (with is_instanceof), so we do reach can_elide_return_type_check() with the scope class for return $this. Nothing was elided because safe_instanceof() only worked when the target interface/parent resolved via zend_optimizer_get_class_entry(), and for a not-yet-linked class declared in the same file that returns NULL, so the recursion had nothing to walk.
The fix: for an unlinked ce1, match the target against the class's own name / interface_names / parent_name directly (no resolution needed, same idea as the compile-time helper), and walk the parent chain, recursing through any link that does resolve (internal / preloaded / linked classes). That makes the unlinked path reachable, and lets the optimizer prove transitive interface cases the compile-time pass can't.
That also answers the "how to test this": the new tests use a conditionally-declared class (kept unlinked) whose parent/interface is an internal class (so it resolves), class C extends ArrayObject returning Traversable, and class D implements IteratorAggregate returning Traversable. Both show VERIFY_RETURN_TYPE before the optimizer and elided after.
Two latent bugs turned up once the parent walk reached real classes:
ce1->parent/ce1->parent_nameare a union; for an unlinked class the field holds the name string, so readingce1->parentpassed azend_string*tosafe_instanceof()as azend_class_entry*, causing a segfault. Resolved via parent_name instead (this path is unlinked-only).ce1->interfaces[i]can beNULLin theRESOLVED_INTERFACES-but-unlinked state (the existing comment warns about it); guarded it.
Soundness is unchanged: a $this that doesn't satisfy the return type is still not elided and TypeErrors at runtime. Full Zend + opcache + reflection pass, including under tracing JIT.
Signed-off-by: azjezz <azjezz@protonmail.com>
| static bool zend_is_this_instance_of_name(const zend_string *type_name) | ||
| { | ||
| if (zend_string_equals_ci(CG(active_class_entry)->name, type_name)) { | ||
| return true; | ||
| } | ||
| if (zend_string_equals_ci(type_name, ZSTR_KNOWN(ZEND_STR_SELF))) { | ||
| return true; | ||
| } | ||
| if (zend_string_equals_ci(type_name, ZSTR_KNOWN(ZEND_STR_PARENT))) { | ||
| return true; | ||
| } | ||
|
|
||
| ZEND_ASSERT((CG(active_class_entry)->ce_flags & ZEND_ACC_LINKED) == 0); | ||
| if (CG(active_class_entry)->num_interfaces) { | ||
| for (uint32_t i = 0; i < CG(active_class_entry)->num_interfaces; i++) { | ||
| if (zend_string_equals_ci(CG(active_class_entry)->interface_names[i].lc_name, type_name)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| const zend_string *parent_name = CG(active_class_entry)->parent_name; | ||
| if (parent_name && zend_string_equals_ci(parent_name, type_name)) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| static bool zend_is_this_valid_for_return_type(zend_type type) | ||
| { | ||
| /* Closures can be bound to a class scope, however it might not and this must type error */ | ||
| if (CG(active_op_array)->fn_flags & ZEND_ACC_CLOSURE) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Not sure if we should keep the compiler pass.. since it's redundant when the optimiser is enabled..
There was a problem hiding this comment.
Go for less complexity, php is overcomplicated as-is. If the optimizer can handle all cases this should be dropped
3 participants
Supersedes #21948 at @Girgias request.
closes #21948