Covariant Returns and Contravariant Parameters #3712
Conversation
| function initialize($properties = array()) | ||
| { | ||
| parent::initialize($properties); | ||
| } |
There was a problem hiding this comment.
I suppose, you removed this, because of some compatibility break, but you didn't reflect this in RFC.
There was a problem hiding this comment.
See this comment on bugs.php.net:
Did the unused initialize methods have some bearing on this bug?
SinceAdoes not have a parent it would fatal if it were ever called. I'm adding compile-time warnings to uses ofparentwhen there isn't one and ran across this bug, but since it's "fixed" I can't tell if removing the unused methods has any bearing.
There was a problem hiding this comment.
Just don't make unrelated "improvements" in PHPT tests, because they look suspicious, and require reviewers to verify them. Better, commit these chunks separately.
There was a problem hiding this comment.
These are not unrelated. The unused method A::initialize has erroneous parent:: usage, which this includes. I know you said you would have preferred that to be a different patch, but the code that deals with parent is needed for resolving variance correctly.
There was a problem hiding this comment.
It would be better to add compile-time warning. This would clearly explain the change in behavior, and its correctness.
| Fatal error: parent::class cannot be used for compile-time class name resolution in %s on line %d | ||
| Deprecated: Cannot use "parent" without a parent class in %s on line %d | ||
|
|
||
| Fatal error: Cannot use "parent" without a parent class in %s on line %d |
There was a problem hiding this comment.
Personally, I would separate parent::class into a different patch.
| } | ||
| /* }}} */ | ||
|
|
||
| static zend_bool zend_do_perform_implementation_check(const zend_function *fe, const zend_function *proto) /* {{{ */ | ||
| static int zend_do_perform_implementation_check(const zend_function *fe, const zend_function *proto) /* {{{ */ |
There was a problem hiding this comment.
See this comment:
This int return is not a boolean, but will often be treated this way.
- 0 means there is definitely an error.
- 1 means there are definitely not any errors.
- -1 means there are no known errors but it is not known to be good either (there is not enough information at the moment).
I am thinking about using an enum here, something like:
typedef enum {
INHERITANCE_INDETERMINANT = -1,
INHERITANCE_ERROR = 0,
INHERITANCE_SUCCESS = 1,
} _inheritance_status;
What doesn't work with opcache? I ran some small tests by hand with opcache enabled and it seemed to work as expected. Edit: I saw your post on the mailing list. I will investigate. |
morrisonlevi
force-pushed
the
variance2
branch
from
695e0ad to
2e2fb44
Compare
todo: add runtime support when ce cannot be found
Still need to collect and then generate ZEND_VERIFY_VARIANCE opcodes
Known issues: - When there is an inheritance warning it gets generated twice. - The VERIFY_VARIANCE opcode is not generated in the correct place
Refactor a bit of variance verification compilation.
Fix bad error message
Fixes this under opcache:
class A {
function m(Traversable $z) {}
}
class B extends A {
function m(iterable $z) {}
}
Also fixes cases where A::m is Iterator, IteratorAggregate, etc.
Fixes this under opcache:
class A {
function m(): iterable {}
}
class B extends A {
function m(): Traversable {}
}
Also fixes cases where B::m is Iterator, IteratorAggregate, etc.
morrisonlevi
force-pushed
the
variance2
branch
from
c351436 to
acd3ec6
Compare
| opcache.enable_cli=1 | ||
| opcache.optimization_level=-1 | ||
| --SKIPIF-- | ||
| <?php if (!extension_loaded('Zend OPcache') || php_sapi_name() != "cli") die("skip CLI only"); ?> |
There was a problem hiding this comment.
Off topic: The second part of this condition, when does it happen? I mean, can we run the tests without been via the CLI?
There was a problem hiding this comment.
@carusogabriel run-tests can actually execute with php-cgi aswell, see: https://github.com/php/php-src/blob/master/run-tests.php#L123 and below and below :)
morrisonlevi
force-pushed
the
variance2
branch
from
8b07b68 to
6be654a
Compare
|
The build is passing now. |
|
Can you please rebase this PR to get rid of the parent related changes? |
|
Closing in favor of PR #3732. |
This is the preliminary implementation for this RFC: Covariant Returns and Contravariant Parameters. Opened per Dmitry's request for inline comments on the code.