search

vimeo / psalm

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

psalm losing track of param & return types after splitting method #1230

Closed SignpostMarv closed 5 years ago

SignpostMarv commented 5 years ago

currently in the process of splitting methods to reduce the per-method complexity of a class, psalm seems to have lost track of the types on a pair of parameters- example to follow.

SignpostMarv commented 5 years ago

similar issues occurred in phpstan, relevant diff is linked in the commit message above, initial failed scrutinizer build is available

SignpostMarv commented 5 years ago

minimal example: https://getpsalm.org/r/a0a8c87c06

muglug commented 5 years ago

It’s about the method_exists call - removing that fixes.

SignpostMarv commented 5 years ago

@muglug ! method_exists(string, '__get') is preferable to (! (interface_exists($type) || class_exists($type)) || ! (new ReflectionClass($type))->hasMethod('__get')): https://3v4l.org/bhsRi/vld#output https://getpsalm.org/r/6656f58fad

weirdan commented 5 years ago

Out of curiosity, can data bits like "has method X" cross the scope boundary? In other words, are they part of a type?

SignpostMarv commented 5 years ago

@weirdan ClassStorage might allow that ?

weirdan commented 5 years ago

ClassStorage, judging by the name, would store data about known classes. But if the class is known (at analysis time) you wouldn't be using method_exists() on it.

I was thinking about something like class-string&class-has-method<X>

muglug commented 5 years ago

has method X doesn't cross any scope boundary - the only thing I track in a scope-like manner is Context::$check_methods (Context is what I'm calling the scope object).

Calling method_exists sets that flag to false which then causes the bug here (because Psalm erroneously exits early when checking static methods with that flag set).

Instead the forthcoming fix will only exit early if the method doesn’t exist.

muglug commented 5 years ago

Fixed in 60e9d4f