vimeo / psalm

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

PossiblyNullReference false-positive #5972

Open fluffycondor opened 3 years ago

fluffycondor commented 3 years ago

https://psalm.dev/r/6cfd65652c

Expected: no errors. Got: PossiblyNullReference

Something really unexplainable is going on here 🤔 Psalm correctly deducts that Foo::getDate() returns only DateTimeImmutable in this branch, but on the very next line it threats the result as DateTimeImmutable|null. The class Foo is marked as immutable as well.

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

I found these snippets:

https://psalm.dev/r/6cfd65652c ```php date; } } class Bar { public ?DateTimeImmutable $date; public ?string $timezone; public function __construct(Foo $foo, ?DateTimeImmutable $date) { if (is_null($foo->getDate())) { $this->date = $date; $this->timezone = !is_null($date) ? $date->getTimezone()->getName() : null; } else { $this->date = $foo->getDate(); /** @psalm-trace $this->date */; $this->timezone = $foo->getDate()->getTimezone()->getName(); } } } ``` ``` Psalm output (using commit 6d4d166): INFO: Trace - 25:44 - $this->date: DateTimeImmutable ERROR: PossiblyNullReference - 26:48 - Cannot call method getTimezone on possibly null value ```
weirdan commented 3 years ago

Seems to be constructor-specific: https://psalm.dev/r/5ced18e821

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

I found these snippets:

https://psalm.dev/r/5ced18e821 ```php date; } } class Bar { public DateTimeImmutable $date2; public function __construct(Foo $foo) { if (null !== $foo->getDate()) { $this->date2 = $foo->getDate(); } $this->date2 = new DateTimeImmutable(); } public function foo(Foo $foo): void { if (null !== $foo->getDate()) { $this->date2 = $foo->getDate(); } $this->date2 = new DateTimeImmutable(); } } ``` ``` Psalm output (using commit 6d4d166): ERROR: PossiblyNullPropertyAssignmentValue - 20:28 - $this->date2 with non-nullable declared type 'DateTimeImmutable' cannot be assigned nullable type 'DateTimeImmutable|null' ```
marcrobertscamao commented 3 years ago

@weirdan we have the same problem, $this->logger is a LoggerInterface or null as a property, the constructor then sets it to be an instance of NullLogger from the psr package, however we still get a possibly null value response.

marcrobertscamao commented 3 years ago

I can replicate this simply in this snippet: https://psalm.dev/r/769852b21a

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

I found these snippets:

https://psalm.dev/r/769852b21a ```php logger = $logger; } } class A { use Loggable; public function __constructor() { $this->logger = new NullLogger(); } public function run():void { $this->logger->log(); } } ``` ``` Psalm output (using commit eb973ab): ERROR: PossiblyNullReference - 32:24 - Cannot call method log on possibly null value ```
MisatoTremor commented 2 years ago

I think it is not constructor related as it also happens in other functions: https://psalm.dev/r/db552ea25f

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

I found these snippets:

https://psalm.dev/r/db552ea25f ```php b; } public function bar(): void { if (null !== $this->getB()) { echo $this->getB()->getFoo(); } } } class B { private string $foo = 'foo'; public function getFoo(): string { return $this->foo; } } ``` ``` Psalm output (using commit 10ea05a): ERROR: PossiblyNullReference - 15:33 - Cannot call method getFoo on possibly null value ```
orklah commented 2 years ago

@MisatoTremor your snippet is indeed error prone. Nothing exclude errors if the getter is not pure: https://psalm.dev/r/96c247415c

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

I found these snippets:

https://psalm.dev/r/96c247415c ```php b; } public function bar(): void { if (null !== $this->getB()) { echo $this->getB()->getFoo(); } } } class B { private string $foo = 'foo'; public function getFoo(): string { return $this->foo; } } ``` ``` Psalm output (using commit 10ea05a): ERROR: PossiblyNullReference - 15:33 - Cannot call method getFoo on possibly null value ```
AndrolGenhald commented 2 years ago

This example from the original issue has gotten stranger, the trace is happening twice with different types...

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

I found these snippets:

https://psalm.dev/r/6cfd65652c ```php date; } } class Bar { public ?DateTimeImmutable $date; public ?string $timezone; public function __construct(Foo $foo, ?DateTimeImmutable $date) { if (is_null($foo->getDate())) { $this->date = $date; $this->timezone = !is_null($date) ? $date->getTimezone()->getName() : null; } else { $this->date = $foo->getDate(); /** @psalm-trace $this->date */; $this->timezone = $foo->getDate()->getTimezone()->getName(); } } } ``` ``` Psalm output (using commit 10ea05a): INFO: Trace - 25:44 - $this->date: DateTimeImmutable INFO: Trace - 25:44 - $this->date: DateTimeImmutable|null ERROR: PossiblyNullReference - 26:48 - Cannot call method getTimezone on possibly null value ```
ghost commented 2 years ago

Could this issue be related to my issue?

https://github.com/vimeo/psalm/issues/8548

MisatoTremor commented 2 years ago

@MisatoTremor your snippet is indeed error prone. Nothing exclude errors if the getter is not pure: https://psalm.dev/r/96c247415c

Thanks, now I get it! But that applies to the original issue as well, as it follows the same principle. So this is not a false-positive at all

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

I found these snippets:

https://psalm.dev/r/96c247415c ```php b; } public function bar(): void { if (null !== $this->getB()) { echo $this->getB()->getFoo(); } } } class B { private string $foo = 'foo'; public function getFoo(): string { return $this->foo; } } ``` ``` Psalm output (using commit e7f05c3): ERROR: PossiblyNullReference - 15:33 - Cannot call method getFoo on possibly null value ```