Add vimeo/psalm to the build and fixed detected static analysis issues that were detected by it
#423
Conversation
src/Reflection/ReflectionClass.php
Outdated
| @@ -340,7 +345,7 @@ public function getMethods(?int $filter = null) : array | |||
| array_filter( | |||
| $this->getMethodsIndexedByName(), | |||
| function (ReflectionMethod $method) use ($filter) { | |||
| return $filter & $method->getModifiers(); | |||
| return (bool) ($filter & $method->getModifiers()); | |||
There was a problem hiding this comment.
The closure should have return type hint as well.
| @@ -17,7 +17,7 @@ | |||
| /** @var SourceLocator */ | |||
| private $wrappedSourceLocator; | |||
|
|
|||
| /** @var Reflection[] indexed by reflector key and identifier cache key */ | |||
| /** @var (Reflection|null)[] indexed by reflector key and identifier cache key */ | |||
There was a problem hiding this comment.
This syntax is not supported by other tools (eg. PHPCS), please don't use it.
src/TypesFinder/ResolveTypes.php
Outdated
| @@ -30,6 +30,7 @@ public function __invoke(array $stringTypes, Context $context) : array | |||
| $resolvedTypes[] = $this->typeResolver->resolve($stringType, $context); | |||
| } | |||
|
|
|||
| /** @var Type[] */ | |||
There was a problem hiding this comment.
This doccomment should be on line 27 and contain variable name.
There was a problem hiding this comment.
I did one of those for-the-typechecker things where I told it that the array only gets non-nullable objects in the foreach
| @@ -685,8 +690,8 @@ function (ReflectionClass $trait) use ($filter) { | |||
|
|
|||
| return array_filter( | |||
| $this->cachedProperties, | |||
| function (ReflectionProperty $property) use ($filter) { | |||
| return $filter & $property->getModifiers(); | |||
| function (ReflectionProperty $property) use ($filter) : bool { | |||
There was a problem hiding this comment.
This should be founded with CS :(
There was a problem hiding this comment.
It's difficult. There are no annotations.
There was a problem hiding this comment.
We're basically missing a mixed return type (in the language) in order to have CS here too...
|
Why did you choose Psalm and not phpstan? |
|
@prisis we had |
|
PHPStan will return :) It is disabled only temporarily 31129a7 |
|
Also because of https://twitter.com/Ocramius/status/994684609928540161 But now PHPStan supports 4.0 in |
No, I think this PR is a good fit. This is an extremely clean library (not the best architecture, but we put some effort in it), and it values types over everything else, so having multiple type checkers is beneficial both for the library and for the authors of the type checkers. |
| @@ -156,6 +156,7 @@ public function getClosureNodes() : ?array | |||
| $nodeTraverser->addVisitor($nodeVisitor); | |||
| $nodeTraverser->traverse($ast); | |||
|
|
|||
| /** @var array $closureNodes */ | |||
| @@ -47,6 +47,8 @@ | |||
| * | |||
| * @throws \InvalidArgumentException | |||
| * @throws \ReflectionException | |||
| * | |||
| * @psalm-suppress DocblockTypeContradiction | |||
There was a problem hiding this comment.
Adding custom annotations is a bit risky due to various annotation parsers. @muglug what do you think about these? Is this kind of stuff something that psalm.xml can handle at all?
| @@ -685,8 +690,8 @@ function (ReflectionClass $trait) use ($filter) { | |||
|
|
|||
| return array_filter( | |||
| $this->cachedProperties, | |||
| function (ReflectionProperty $property) use ($filter) { | |||
| return $filter & $property->getModifiers(); | |||
| function (ReflectionProperty $property) use ($filter) : bool { | |||
There was a problem hiding this comment.
We're basically missing a mixed return type (in the language) in order to have CS here too...
|
🚢 |
Ocramius
changed the title
vimeo/psalm to the build and fixed detected static analysis issues that were detected by it
This adds Psalm to BetterReflection's build, fixing issues it found (mainly by adding supplementary docblocks).
If you'd rather those docblocks be
asserts, I'm happy to update.