-
Notifications
You must be signed in to change notification settings - Fork 8k
Elide ZEND_VERIFY_RETURN_TYPE when returning $this if possible #21948
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
Girgias
wants to merge
4
commits into
php:master
Choose a base branch
from
Girgias:elide-return-type-check-for-this-with-static
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.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
c27e7fc
Add tests prior to optimization
Girgias aaaafd8
Do not emit ZEND_VERIFY_RETURN_TYPE if we return $this and return val…
Girgias dd69c23
Optimizer: Implement basic unlinked interface traversal in safe_insta…
Girgias 7a27ef4
Elide return type check for directly provable instances of $this at c…
Girgias 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 |
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 |
|---|---|---|
|
|
@@ -2642,6 +2642,58 @@ static void zend_compile_memoized_expr(znode *result, zend_ast *expr, uint32_t t | |
| } | ||
| /* }}} */ | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| if (ZEND_TYPE_FULL_MASK(type) & (MAY_BE_OBJECT|MAY_BE_STATIC)) { | ||
| return true; | ||
| } | ||
|
|
||
| const zend_type *single_type; | ||
| ZEND_TYPE_FOREACH(type, single_type) { | ||
| if (ZEND_TYPE_HAS_NAME(*single_type)) { | ||
| const zend_string *name = ZEND_TYPE_NAME(*single_type); | ||
| if (zend_is_this_instance_of_name(name)) { | ||
| return true; | ||
| } | ||
| } | ||
| } ZEND_TYPE_FOREACH_END(); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| static void zend_emit_return_type_check( | ||
| znode *expr, const zend_arg_info *return_info, bool implicit) /* {{{ */ | ||
| { | ||
|
|
@@ -2697,6 +2749,17 @@ static void zend_emit_return_type_check( | |
| return; | ||
| } | ||
|
|
||
| /* If return type contains static and we are returning $this | ||
| * (determined by checking if the previous opcode is ZEND_FETCH_THIS) | ||
| * then we don't need to check the return type */ | ||
| const zend_op_array *op_array = CG(active_op_array); | ||
| if (expr && op_array->last >= 1 | ||
| && op_array->opcodes[op_array->last-1].opcode == ZEND_FETCH_THIS | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should probably check that the last opline is the right one, i.e. compare vars of |
||
| && zend_is_this_valid_for_return_type(type)) { | ||
| ZEND_ASSERT((expr->op_type & (IS_VAR|IS_TMP_VAR)) && expr->u.op.var == op_array->opcodes[op_array->last-1].result.var); | ||
| return; | ||
| } | ||
|
|
||
| opline = zend_emit_op(NULL, ZEND_VERIFY_RETURN_TYPE, expr, NULL); | ||
| if (expr && expr->op_type == IS_CONST) { | ||
| opline->result_type = expr->op_type = IS_TMP_VAR; | ||
|
|
||
56 changes: 56 additions & 0 deletions
56
...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,56 @@ | ||
| --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=4, args=0, vars=0, tmps=1) | ||
| ; (before optimizer) | ||
| ; %s:7-9 | ||
| ; return [] RANGE[0..0] | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 | ||
| 0002 VERIFY_RETURN_TYPE | ||
| 0003 RETURN null | ||
|
|
||
| $_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 |
53 changes: 53 additions & 0 deletions
53
...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,53 @@ | ||
| --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=4, args=0, vars=0, tmps=1) | ||
| ; (before optimizer) | ||
| ; %s:6-8 | ||
| ; return [] RANGE[0..0] | ||
| 0000 T0 = FETCH_THIS | ||
| 0001 RETURN T0 | ||
| 0002 VERIFY_RETURN_TYPE | ||
| 0003 RETURN null | ||
|
|
||
| $_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.
Not sure how to test we hit this case? As the tests I added don't.