Elide ZEND_VERIFY_RETURN_TYPE when returning $this#22335
Conversation
|
|
||
| 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>
| ; (after optimizer) | ||
| ; %s:4-6 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 No newline at end of file |
| ; (after optimizer) | ||
| ; %s:4-6 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 No newline at end of file |
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) No newline at end of file |
| ; (after optimizer) | ||
| ; %s:4-6 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 No newline at end of file |
| ; (after optimizer) | ||
| ; %s:4-6 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 No newline at end of file |
| ; (after optimizer) | ||
| ; %s:6-8 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 No newline at end of file |
| ; (after optimizer) | ||
| ; %s:6-8 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 No newline at end of file |
Supersedes #21948 at @Girgias request.
closes #21948