vimeo / psalm

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

Treat DocBlock types as uncertain #9688

Open janopae opened 1 year ago

janopae commented 1 year ago

When a type is declared as PHPDoc type and doesn't get checked at runtime, it is on Psalm to verify its correctness. Often, Psalm does this based on other PHPDoc types. As a result, wrong PHPDoc type declarations can cascade through the whole application – similar to an application with type checks completely missing, where the error occurs much later than the where the actual error source is.

The moment an application contains at least

PHPDoc types can't be trusted.

This is usually the case for both, applications that contain some baselined legacy code and applications that use non-psalm libraries.

However, not only does Psalm allow to use variables as if their PHPDoc type was safe. It even disallows performing assertions that check the type at runtime (like if (is_empty($array)) for a non-empty-array according to DocBlock), preventing type-safety in these situations.

Psalm has the option to disable xGivenDockblockType errors to allow runtime type checking despite having the DockBlock already saying a type. But that requires you to omit PHP type declarations completely – which makes your code even more unsafe.

PHPStan has an option called "Treat PHPDoc types as certain", which can be disabled. When disabled, it internally stores 2 types (the type according to PHPDoc and the type according to actual type checks) and at least allows additional type checks for variables that have a more specific PHPDoc type than their PHP type, only emitting errors in the vein of TypeDoesNotContainType when redundantly checking for the PHP type.

I'd argue that Psalm should behave this way by default. There is no reason to raise an issue on an additional type check, especially if you can't be sure about the type without the check.

Maybe Psalm should even provide an option to go further than that: It could require additional runtime checks instead of just allowing them before using a variable with its PHPDoc-decklared type. This essentially means ignoring PHPDoc for inferring types, but still reading PHPDoc when checking correctness of assignments (still raising issues when someone tries to pass a less specific type into a function constrained by PHPDoc types).

According to https://github.com/vimeo/psalm/issues/9578#issuecomment-1487395873, Psalm currently only stores one type, and only has a flag whether whether the type has been inferred from the DocBlock (which doesn't work correctly currently, as it is false when the type has been inferred form DocBlock but there is also a PHP type declaration). Properly distinguishing between both types would only be possible if Psalm saved 2 types when a DocBlock type is given.

This issue can be improved on in the following ways:

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/5270ce3533 ```php $list */ function doSomething(array $list): void { $i = getFirstListElement($list); echo $i + 2; } /** * @template T * @param non-empty-list $list * @return T */ function getFirstListElement(array $list) { return reset($list); } /** * @return non-empty-list */ function getIntegerList(): array { /** @var non-empty-list */ return ['Ha, fooled you!']; } $list = getIntegerList(); doSomething($list); ``` ``` Psalm output (using commit 5efddb4): No issues! ```
https://psalm.dev/r/e483f9ce0d ```php $list */ function doSomething(array $list): void { $i = getFirstListElement($list); if (!is_int($i)) { throw new LogicException(); } echo $i + 2; } /** * @template T * @param non-empty-list $list * @return T */ function getFirstListElement(array $list) { if (empty($list)) { throw new LogicException(); } return reset($list); } /** * @return non-empty-list */ function getIntegerList(): array { /** @var non-empty-list */ return ['Ha, fooled you!']; } $list = getIntegerList(); doSomething($list); ``` ``` Psalm output (using commit 5efddb4): ERROR: DocblockTypeContradiction - 10:10 - Docblock-defined type int for $i is always int ERROR: DocblockTypeContradiction - 24:9 - Docblock-defined type non-empty-list for $list is never falsy ```
https://psalm.dev/r/836af92173 ```php for $a is always !array ```