vimeo / psalm

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

Instanceof checks aren't properly reconciled in "else" context #9939

Open tscni opened 1 year ago

tscni commented 1 year ago

Some examples with variable re-definitions: https://psalm.dev/r/2cf106db54 In a(), $x should be 'x' In b(), $x should be X In c(), the instanceof check should not be redundant d() works correctly

I mainly include the class-string cases because them working (in general) is the cause of the issue. This relates to https://github.com/vimeo/psalm/pull/6739 and https://github.com/vimeo/psalm/pull/6746 Specifically (in the negated case) this part of the code: https://github.com/vimeo/psalm/blob/56d7b3793eca1e61036af64fe00d49ad69e3935b/src/Psalm/Internal/Type/NegatedAssertionReconciler.php#L198-L204

I'm currently trying to figure out how I can differentiate class-string<X> from X or 'X', which would be necessary to fix the issue. Maybe I could implement something similar to TNamedObject::$definite_class, e.g. a $definite_type which would be true if it's definitely X / 'X', but not if it's class-string<X>

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

I found these snippets:

https://psalm.dev/r/2cf106db54 ```php $xClass */ function c(X $x, $xClass): void { if ($x instanceof $xClass) { $x = 'x'; } /** @psalm-trace $x */ } /** @param class-string $xClass */ function d(X $x, $xClass): void { if (!$x instanceof $xClass) { $x = 'x'; } /** @psalm-trace $x */ } ``` ``` Psalm output (using commit 653ad66): INFO: Trace - 10:0 - $x: 'x'|X INFO: Trace - 18:0 - $x: 'x'|X ERROR: RedundantCondition - 23:9 - Type X for $x is always X INFO: Trace - 26:0 - $x: 'x'|X INFO: Trace - 34:0 - $x: 'x'|X ```
orklah commented 1 year ago

Yeah, I remember banging my head on that multiple times :p

the issue with $definite_type is that unless a class is final, it's pretty much impossible to know. (Even if there is no child known for this type, it may be a library that expect people to implement their own)

There's definitely room for improvement anyway!

kkmuffme commented 1 year ago

At least for cases where the type after the "instanceof" is a literal (and not a variable) this is relatively simple to fix via a plugin (via AfterExpressionAnalysisEvent for Instanceof_) so that the negation at least reports a redundant condition (https://psalm.dev/r/4ca7336f15) I guess that can be done similarly in psalm itself directly, just not with the assertion system, since we don't know at that stage if it's a literal or not.

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

I found these snippets:

https://psalm.dev/r/4ca7336f15 ```php