use conditional parameter for is_a() via stub#1306
Conversation
There was a problem hiding this comment.
stub taken 1:1 from psalm
There was a problem hiding this comment.
update: psalm was missing the type for $allow_string, which will be added with vimeo/psalm#7951
|
Please read the title of the linked issue: is_a: "if third argument is false then do not allow string in first argument" Which means that there should be a new test in CallToFunctionParametersRuleTest (or ImpossibleCheckTypeFunctionCallRuleTest, that depends) that reports a new error here https://phpstan.org/r/44b40ab5-7c7f-4ced-8048-e084d3d63f1d on line 10. |
|
yeah, it seems the initially reported bug in 4371 is different from the one I had in phpstan/phpstan#4371 (comment) but I am fine, with also covering the original issue here then |
There was a problem hiding this comment.
is no longer considered "fine" because Foo is not a subclass of Throwable
There was a problem hiding this comment.
But also please test Foo::class (which is different thanks to namespace resolution), the class isn't final so a subclass might implement Throwable.
There was a problem hiding this comment.
hm I guess you mean I should test class-string<Foo> as a parameter not Foo::class.. at least thats what I did for now.. see added tests at the end of this file
staabm
left a comment
There was a problem hiding this comment.
this gets a few rough edges, therefore I pushed the current development state and hope for feedback.
There was a problem hiding this comment.
hm I guess you mean I should test class-string<Foo> as a parameter not Foo::class.. at least thats what I did for now.. see added tests at the end of this file
There was a problem hiding this comment.
with 3e955af I dropped the conditional return type stub again, as I am no longer sure its worth to report about string vs. class-string type errors for $class.. might be tedious.
since we already have extensions for is_a in place the stub itself was nevertheless only partly usefull, I think.
There was a problem hiding this comment.
because I dropped this early-return, I had to compensate a few false positives in the IsAFunctionTypeSpecifyingExtension - added a few early returns.
There was a problem hiding this comment.
especially this condition is pretty ugly .. would love feedback on how to improve it.
at first I only had the $classType instanceof ClassStringType part, but later on needed to add !$classType instanceof GenericClassStringType to prevent regressions of existing tests
…ving a early return in ImpossibleCheckTypeFunctionCallRule
|
This is already quite messy again :/ I think the stub solved a problem so I really don't want to have it reverted here 3e955af. But this is truly a minor problem. I'd rather be if you worked on the NamedArgumentsHelper improvements :) Thanks. |
|
I started workin on this topic, because we had a production issue, which would be great if beeing covered by phpstan the next time. it looked easy at first, but it seems I have no luck in picking topics to work on, since most of the time it turns into walking thru rabbit holes, like we can see here :-/. will do NamedArguments stuff, when free time allows again. |
|
You can just send the conditional stub in a separate PR alongside with the tests as you initially did, without marking phpstan/phpstan#4371 as closing :) |
|
Basically this commit 6bbb2a6 :) |
That's normal, I just spent several hours today trying to clean up something about named arguments and I'm not happy with the result either. The combination of php-8-stubs, phpstorm-stubs and php-8-stubs is a mess in some situations. And PHP itself is a mess too :) |
closes phpstan/phpstan#4371
closes #440
see https://www.php.net/is_a