Add @phpstan-self-out support #1799
Conversation
|
Initially I used |
|
This also needs a rule that checks if the out type is a subtype of the class. The type is currently unrestricted (just like in psalm) so generating an error for incompatible types is useful. |
|
I don't understand why CI is failing :/ |
|
Looks like Rector fails and doesn't transform the code at all... |
|
I had the exact same thing in a PR a couple of days ago where I modified MutatintgScope and its factories |
|
One theory I have is that Rector parallelism isn't really compatible with our caching, so I tried this: c5d186b Hopefully the build passes now. |
|
So the problem that's happening now is that Rector loads the wrong version of phpdoc-parser, and fails to do its job, because it's calling to an undefined method. Rector should really preload its own version of phpdoc-parser, like PHPStan does with nikic/php-parser... |
|
Oh it actually does, so the problem is something else still. |
|
Alright, so the problem is that Rector preloads its own version of phpdoc-parser, but it actually uses PHPStan from our current |
|
Ah OK, looks like a different error that I had with MutatingScope in #1783 then.. |
|
That's gonna be the same category of problem... |
|
Thanks for looking into the rector issue! |
src/PhpDoc/ResolvedPhpDocBlock.php
Outdated
| @@ -216,6 +220,7 @@ public function merge(array $parents, array $parentPhpDocBlocks): self | |||
| $result->typeAliasTags = $this->getTypeAliasTags(); | |||
| $result->typeAliasImportTags = $this->getTypeAliasImportTags(); | |||
| $result->assertTags = self::mergeAssertTags($this->getAssertTags(), $parents, $parentPhpDocBlocks); | |||
| $result->selfOutTypeTag = $this->getThisOutTag(); | |||
There was a problem hiding this comment.
If the method overrides a parent method, it should use the this-out tag from the parent? That's what all the other tags do here.
Also - selfOutTypeTag vs. getThisOutTag, consistency please :) I like self more.
src/PhpDoc/ResolvedPhpDocBlock.php
Outdated
| @@ -297,6 +302,7 @@ public function changeParameterNamesByMapping(array $parameterNameMapping): self | |||
| $self->typeAliasTags = $this->typeAliasTags; | |||
| $self->typeAliasImportTags = $this->typeAliasImportTags; | |||
| $self->assertTags = $assertTags; | |||
| $self->selfOutTypeTag = $this->getThisOutTag(); | |||
There was a problem hiding this comment.
All the other properties here assign properties, so it should be consistent. That's because we don't need to resolve the tag at this point, let false be false.
| $classReflection = $method->getDeclaringClass(); | ||
| $classType = new ObjectType($classReflection->getName(), null, $classReflection); | ||
|
|
||
| if (!$classType->isSuperTypeOf($selfOutType)->no()) { |
There was a problem hiding this comment.
The problem here is that it allows $selfOutType to be wider than $classType. Means it can do @phpstan-self-out self|null for example.
I'd change this condition to if ($classType->isSuperTypeOf($selfOutType)->yes()) {.
|
|
||
| return [ | ||
| RuleErrorBuilder::message(sprintf( | ||
| 'Out type %s is not compatible with %s.', |
There was a problem hiding this comment.
Would be nice to have more details here. Like Self-out type %s of method %s::%s() is not subtype of %s. That matches a similar message about PHPDoc vs. native type...
ondrejmirtes
force-pushed
the
self-out
branch
from
1c58df1 to
ad444fc
Compare
|
Thank you very much! |
This is a "missing" feature from the assert family which allows changing the
$thistype after a method call, which is particularly useful for setters.See https://psalm.dev/docs/annotating_code/adding_assertions/#asserting-return-values-of-methods for the docs for psalm.
Syntax implementation in phpstan/phpdoc-parser#149