-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Elide ZEND_VERIFY_RETURN_TYPE when returning $this #22335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
azjezz
wants to merge
7
commits into
php:master
Choose a base branch
from
carthage-software:this-return-type-check
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+842
−7
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ce53220
Add tests prior to optimization
Girgias f6d7bb8
Do not emit ZEND_VERIFY_RETURN_TYPE if we return $this and return val…
Girgias 21ad193
Optimizer: Implement basic unlinked interface traversal in safe_insta…
Girgias 15c01db
Elide return type check for directly provable instances of $this at c…
Girgias 15b87fd
Optimizer: prove return types for unlinked classes via names
azjezz 3ed6985
drop compile-time $this return elision in favour of the optimizer
azjezz c1985cc
EOL
azjezz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| --TEST-- | ||
| Return type of self is allowed in closure but $this return value must be checked as closure might not be bound to a class | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| $c = function(): self { return $this; }; | ||
| try { | ||
| var_dump($c()); | ||
| } catch (Throwable $e) { | ||
| echo $e::class, ': ', $e->getMessage(), PHP_EOL; | ||
| } | ||
| ?> | ||
| --EXPECT-- | ||
| Error: Using $this when not in object context |
59 changes: 59 additions & 0 deletions
59
...s/opt/verify_return_type/direct_extended_interface_verify_return_type_for_this_class.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| --TEST-- | ||
| Return type check elision for direct interface return type and $this in class method when interface extends another one | ||
| --INI-- | ||
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.optimization_level=-1 | ||
| opcache.opt_debug_level=0x30000 | ||
| --EXTENSIONS-- | ||
| opcache | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| interface I1 {} | ||
| interface I2 extends I1 {} | ||
|
|
||
| class C implements I2 { | ||
| public function foo(): I2 { | ||
| return $this; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| $_main: | ||
| ; (lines=3, args=0, vars=0, tmps=0) | ||
| ; (before optimizer) | ||
| ; %s:1-13 | ||
| ; return [] RANGE[0..0] | ||
| 0000 DECLARE_CLASS string("i2") | ||
| 0001 DECLARE_CLASS string("c") | ||
| 0002 RETURN int(1) | ||
|
|
||
| C::foo: | ||
| ; (lines=5, args=0, vars=0, tmps=1) | ||
| ; (before optimizer) | ||
| ; %s:7-9 | ||
| ; return [] RANGE[0..0] | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| 0003 VERIFY_RETURN_TYPE | ||
| 0004 RETURN null | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) | ||
|
|
||
| $_main: | ||
| ; (lines=3, args=0, vars=0, tmps=0) | ||
| ; (after optimizer) | ||
| ; %s:1-13 | ||
| 0000 DECLARE_CLASS string("i2") | ||
| 0001 DECLARE_CLASS string("c") | ||
| 0002 RETURN int(1) | ||
|
|
||
| C::foo: | ||
| ; (lines=2, args=0, vars=0, tmps=1) | ||
| ; (after optimizer) | ||
| ; %s:7-9 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 |
56 changes: 56 additions & 0 deletions
56
...ache/tests/opt/verify_return_type/direct_interface_verify_return_type_for_this_class.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| --TEST-- | ||
| Return type check elision for direct interface return type and $this in class method | ||
| --INI-- | ||
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.optimization_level=-1 | ||
| opcache.opt_debug_level=0x30000 | ||
| --EXTENSIONS-- | ||
| opcache | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| interface I1 {} | ||
|
|
||
| class C implements I1 { | ||
| public function foo(): I1 { | ||
| return $this; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| $_main: | ||
| ; (lines=2, args=0, vars=0, tmps=0) | ||
| ; (before optimizer) | ||
| ; %s:1-12 | ||
| ; return [] RANGE[0..0] | ||
| 0000 DECLARE_CLASS string("c") | ||
| 0001 RETURN int(1) | ||
|
|
||
| C::foo: | ||
| ; (lines=5, args=0, vars=0, tmps=1) | ||
| ; (before optimizer) | ||
| ; %s:6-8 | ||
| ; return [] RANGE[0..0] | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| 0003 VERIFY_RETURN_TYPE | ||
| 0004 RETURN null | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) | ||
|
|
||
| $_main: | ||
| ; (lines=2, args=0, vars=0, tmps=0) | ||
| ; (after optimizer) | ||
| ; %s:1-12 | ||
| 0000 DECLARE_CLASS string("c") | ||
| 0001 RETURN int(1) | ||
|
|
||
| C::foo: | ||
| ; (lines=2, args=0, vars=0, tmps=1) | ||
| ; (after optimizer) | ||
| ; %s:6-8 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 |
61 changes: 61 additions & 0 deletions
61
...e/tests/opt/verify_return_type/inherited_interface_verify_return_type_for_this_class.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| --TEST-- | ||
| Return type check elision for inherited interface return type and $this in class method | ||
| --INI-- | ||
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.optimization_level=-1 | ||
| opcache.opt_debug_level=0x30000 | ||
| --EXTENSIONS-- | ||
| opcache | ||
| --FILE-- | ||
| <?php | ||
| interface I1 {} | ||
| interface I2 extends I1 {} | ||
|
|
||
| class C implements I2 { | ||
| public function foo(): I1 { | ||
| return $this; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| $_main: | ||
| ; (lines=3, args=0, vars=0, tmps=0) | ||
| ; (before optimizer) | ||
| ; %s:1-12 | ||
| ; return [] RANGE[0..0] | ||
| 0000 DECLARE_CLASS string("i2") | ||
| 0001 DECLARE_CLASS string("c") | ||
| 0002 RETURN int(1) | ||
|
|
||
| C::foo: | ||
| ; (lines=5, args=0, vars=0, tmps=1) | ||
| ; (before optimizer) | ||
| ; %s:6-8 | ||
| ; return [] RANGE[0..0] | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| 0003 VERIFY_RETURN_TYPE | ||
| 0004 RETURN null | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) | ||
|
|
||
| $_main: | ||
| ; (lines=3, args=0, vars=0, tmps=0) | ||
| ; (after optimizer) | ||
| ; %s:1-12 | ||
| 0000 DECLARE_CLASS string("i2") | ||
| 0001 DECLARE_CLASS string("c") | ||
| 0002 RETURN int(1) | ||
|
|
||
| C::foo: | ||
| ; (lines=3, args=0, vars=0, tmps=1) | ||
| ; (after optimizer) | ||
| ; %s:6-8 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) |
65 changes: 65 additions & 0 deletions
65
...urn_type/inherited_interface_via_class_inheritance_verify_return_type_for_this_class.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| --TEST-- | ||
| Return type check elision for inherited interface via class extension return type and $this in class method | ||
| --INI-- | ||
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.optimization_level=-1 | ||
| opcache.opt_debug_level=0x30000 | ||
| --EXTENSIONS-- | ||
| opcache | ||
| --FILE-- | ||
| <?php | ||
| interface I1 {} | ||
| interface I2 extends I1 {} | ||
|
|
||
| class C1 implements I2 {} | ||
|
|
||
| class C2 extends C1 { | ||
| public function foo(): I2 { | ||
| return $this; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| $_main: | ||
| ; (lines=4, args=0, vars=0, tmps=0) | ||
| ; (before optimizer) | ||
| ; %s:1-14 | ||
| ; return [] RANGE[0..0] | ||
| 0000 DECLARE_CLASS string("i2") | ||
| 0001 DECLARE_CLASS string("c1") | ||
| 0002 DECLARE_CLASS_DELAYED string("c2") string("c1") | ||
| 0003 RETURN int(1) | ||
|
|
||
| C2::foo: | ||
| ; (lines=5, args=0, vars=0, tmps=1) | ||
| ; (before optimizer) | ||
| ; %s:8-10 | ||
| ; return [] RANGE[0..0] | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| 0003 VERIFY_RETURN_TYPE | ||
| 0004 RETURN null | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) | ||
|
|
||
| $_main: | ||
| ; (lines=4, args=0, vars=0, tmps=0) | ||
| ; (after optimizer) | ||
| ; %s:1-14 | ||
| 0000 DECLARE_CLASS string("i2") | ||
| 0001 DECLARE_CLASS string("c1") | ||
| 0002 DECLARE_CLASS_DELAYED string("c2") string("c1") | ||
| 0003 RETURN int(1) | ||
|
|
||
| C2::foo: | ||
| ; (lines=3, args=0, vars=0, tmps=1) | ||
| ; (after optimizer) | ||
| ; %s:8-10 | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 VERIFY_RETURN_TYPE T0 | ||
| 0002 RETURN T0 | ||
| LIVE RANGES: | ||
| 0: 0001 - 0002 (tmp/var) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status note for reviewers, carrying over @Girgias's "not sure how to test we hit
this case" and @arnaud-lb's
MAY_BE_THISsuggestion from #21948.This generalized
safe_instanceof()is currently not reached forreturn $this.can_elide_return_type_check()only continues whenuse_info->ceis set, and inference assigns noceto$this(there is noMAY_BE_THIS), so forreturn $thiswe bail before ever callingcan_elide_list_type()/safe_instanceof(). That's why theinherited_interface*tests still showVERIFY_RETURN_TYPEafter the optimizer.I also couldn't reach the unlinked branches via a non-
$thisvalue: when inference knows the operand's class it's alreadyZEND_ACC_LINKED(so theinstanceof_function()fast path is taken), and when the class is unlinked/conditionaluse_info->ceis unset and we bail first. So thisRESOLVED_INTERFACESrecursion in particular is currently unexercised.Making the optimizer side actually elide the inherited-interface
$thiscases needs theMAY_BE_THISinference @arnaud-lb suggested, so the$thisSSA var carries the scopece.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm,
MAY_BE_THISis actually already there, the problem seems to be class resolution, not inference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed,
FETCH_THISalready setsuse_info->ce = op_array->scope(withis_instanceof), so we do reachcan_elide_return_type_check()with the scope class forreturn $this. Nothing was elided becausesafe_instanceof()only worked when the target interface/parent resolved viazend_optimizer_get_class_entry(), and for a not-yet-linked class declared in the same file that returnsNULL, 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 ArrayObjectreturningTraversable, andclass D implements IteratorAggregatereturningTraversable. Both showVERIFY_RETURN_TYPEbefore 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
$thisthat doesn't satisfy the return type is still not elided andTypeErrors at runtime. Full Zend + opcache + reflection pass, including under tracing JIT.