vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

Multiple `psalm-assert-if-true` tags is not recognized correctly #8623

Open andrew-demb opened 2 years ago

andrew-demb commented 2 years ago

Reused example from the documentation: https://psalm.dev/docs/annotating_code/adding_assertions/#asserting-return-values-of-methods

https://psalm.dev/r/66ce8ba63c

Expected result: no errors Actual result: ERROR: [PossiblyNullReference](https://psalm.dev/083) - 32:35 - Cannot call method getMessage on possibly null value

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/66ce8ba63c ```php exception * @psalm-assert-if-true Exception $this->getException() */ public function hasException(): bool { return $this->exception !== null; } public function getException(): ?Exception { return $this->exception; } public function foo(): void { if( $this->hasException() ) { // Psalm now knows that $this->exception is an instance of Exception echo $this->exception->getMessage(); } } } $result = new Result; if( $result->hasException() ) { // Psalm now knows that $result->getException() will return an instance of Exception echo $result->getException()->getMessage(); } ``` ``` Psalm output (using commit 25b3937): ERROR: PossiblyNullReference - 32:35 - Cannot call method getMessage on possibly null value ```
orklah commented 2 years ago

There's a phrase after the example that sets a condition for that to work:

Please note that the example above only works if you enable method call memoization in the config file or annotate the class as immutable.

And indeed, it works with the correct setting: https://psalm.dev/r/3cbab84b31

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/3cbab84b31 ```php exception * @psalm-assert-if-true Exception $this->getException() */ public function hasException(): bool { return $this->exception !== null; } public function getException(): ?Exception { return $this->exception; } public function foo(): void { if( $this->hasException() ) { // Psalm now knows that $this->exception is an instance of Exception echo $this->exception->getMessage(); } } } $result = new Result; if( $result->hasException() ) { // Psalm now knows that $result->getException() will return an instance of Exception echo $result->getException()->getMessage(); } ``` ``` Psalm output (using commit 7c83878): No issues! ```
andrew-demb commented 2 years ago

There's a phrase after the example that sets a condition for that to work: And indeed, it works with the correct setting:

Sorry for this.

My initial problem was too close to the provided example from documentation so for simplicity, I thought it would be easy to use example from docs.

Probably I found the problem - if I have a method with psalm-assert-if-true, check it with false and throw exception - psalm cannot determine that code after throwing an exception will have proper types according to psalm-assert-if-true.

Here's my real code that was not checked as expected for me: https://psalm.dev/r/15061d01fd Expected: no errors

Do you prefer to create a separate issue for this reproducer?

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/15061d01fd ```php isConfigurationValid()) { // dummy code just for example echo \str_contains($this->serviceId, $this->handles); return new \stdClass(); } if (false === $this->isConfigurationValid()) { throw new \BadMethodCallException('Invalid builder configuration'); } // but it this is not find // dummy code just for example echo \str_contains($this->serviceId, $this->handles); return new \stdClass(); } /** * @psalm-assert-if-true string $this->serviceId * @psalm-assert-if-true string $this->handles */ private function isConfigurationValid(): bool { return null !== $this->serviceId && null !== $this->handles; } } ``` ``` Psalm output (using commit 8fc499e): ERROR: PossiblyNullArgument - 25:28 - Argument 1 of str_contains cannot be null, possibly null value provided ERROR: PossiblyNullArgument - 25:46 - Argument 2 of str_contains cannot be null, possibly null value provided ```
orklah commented 2 years ago

I'll have to look at this after I sleep :p

However, I can warn about one possible misconception: It's not because you have a @psalm-assert-if-true string $a that it says anything at all about the state of $a when the method return false

(possibly $a could be null when false but could also be string|null, it's just unknown). Psalm is handling that very strangely though (it sometime tries to invert the assertion on false) and I'm pretty sure there's bugs in there

I also think I remember that @psalm-assert-if-true can't be used at the same time as @psalm-assert-if-false, which is very confusing if you smash all that together