Make ParametersAcceptorSelector::selectSingle() return union-ed conditional types#1159
Conversation
…tional types See phpstan/phpstan/issues/3853#issuecomment-1082351763
There was a problem hiding this comment.
Maybe cache the result here in a private property? :)
rvanvelzen
force-pushed
the
select-single-conditional
branch
from
42e5efc to
a71c977
Compare
| $parametersAcceptor = $parametersAcceptors[0]; | ||
| if (TypeUtils::containsConditional($parametersAcceptor->getReturnType())) { | ||
| return new SingleParametersAcceptor($parametersAcceptor); | ||
| } |
There was a problem hiding this comment.
I couldn't immediately get tests to pass without guarding here. I could whip up a helper class that can convert a given ParametersAcceptor into the same type but with the return type adjusted? That way the contract still holds.
| public function getReturnType(): Type | ||
| { | ||
| if ($this->returnType === null) { | ||
| return $this->returnType = TypeTraverser::map($this->acceptor->getReturnType(), static function (Type $type, callable $traverse) { |
There was a problem hiding this comment.
style-nit - the line is already long enough, and will return on the outer block immediately with the same value
| return $this->returnType = TypeTraverser::map($this->acceptor->getReturnType(), static function (Type $type, callable $traverse) { | |
| $this->returnType = TypeTraverser::map($this->acceptor->getReturnType(), static function (Type $type, callable $traverse) { |
| assertType('bool', $this->get(0)); | ||
| assertType('bool', $this->get(1)); | ||
| assertType('bool', $this->get(2)); |
There was a problem hiding this comment.
do we expect a more precise type here?
| assertType('bool', $this->get(0)); | |
| assertType('bool', $this->get(1)); | |
| assertType('bool', $this->get(2)); | |
| assertType('false', $this->get(0)); | |
| assertType('true', $this->get(1)); | |
| assertType('false', $this->get(2)); |
There was a problem hiding this comment.
No - there is return type extension that just returns ParametersAcceptorSelector::selectSingle() to verify that it provides the resulting union type instead of the conditional type.
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
I really like this solution, it's better than what I'd come up with :)
src/Reflection/FunctionVariant.php
Outdated
| */ | ||
| public function flattenConditionalsInReturnType(): self | ||
| { | ||
| $result = clone $this; |
There was a problem hiding this comment.
I dislike clone, please create new instance explicitly with new self.
There was a problem hiding this comment.
Hmm, that's disappointing. Given that these classes are marked @api and the constructor isn't final, constructing an instance of static seems impossible.
Or would you accept a version that returns $this if self::class !== static::class?
There was a problem hiding this comment.
Give me a moment, I'll think of something 😊
There was a problem hiding this comment.
Yeah, these classes should ideally be final, there's no reason to extend them. I've marked a new todo for 2.0 when the time comes - class has either to be @api or final (it can be both).
There was a problem hiding this comment.
Maybe we can mark them @final in the meantime?
| { | ||
| $returnType = $this->getReturnType(); | ||
|
|
||
| $result = clone $this; |
There was a problem hiding this comment.
I dislike clone, please create new instance explicitly with new self.
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
This is good enough, I'm happy to merge it :)
|
Thank you! |
3 participants
See phpstan/phpstan#3853 (comment)