Fix false positive of function.alreadyNarrowedType (function call variable assignment) #4237
Conversation
|
Looking at issuebot results tells me that I need a different approach |
staabm
changed the title
|
This pull request has been marked as ready for review. |
|
This pull request has been marked as ready for review. |
| continue; | ||
| } | ||
|
|
||
| $assignedInCallVars[] = $arg->value; |
There was a problem hiding this comment.
Can be nested multiple assigns in themselves. Also it might be useful to check and unwrap Expr\AssignOp\Coalesce (??=) along with tests.
There was a problem hiding this comment.
Multiple nested assigns, meaning $foo = $bar = ....
There was a problem hiding this comment.
Can be nested multiple assigns in themselves.
fixed
Also it might be useful to check and unwrap
Expr\AssignOp\Coalesce(??=) along with tests.
added some basic tests which work as expected.
more complicated with a mix of assign and coalesce feel alien to me and is pretty hard to read. I would not add support for such non-readable code if possible.
staabm
force-pushed
the
bug13268
branch
2 times, most recently
from
40a9c18 to
edaae9f
Compare
|
This pull request has been marked as ready for review. |
…iable assignment)
|
coming from this PR here and looking at the issue in phpstan/phpstan#12234 it seems like a better approach would be to have a per argument Scope object. It looks like the scope object passed to the rules is not seeing the assignment early enough and therefore reasons with a not updated scope. If I am right it might mean we need a better fix here and it could also cover phpstan/phpstan#12234 wdyt? |
|
Yeah, this is kind of a problem with how PHPStan is written on principle. We have multiple places like NodeScopeResolver, TypeSpecifier, MutatingScope::resolveType, and rules. All of these places need to use the appropriate Scope for the AST property they're asking about. And sometimes they do duplicate work. I was able to fix some bugs by doing the same thing in TypeSpecifier and MutatingScope that's already been done in NodeScopeResolver. Example: 989dd6f Sometimes I'm able to solve this with virtual nodes. LiteralArrayNode and LiteralArrayItem exists so that a rule can see the correct Scope for each array item. The bug in https://phpstan.org/r/8d3536cc-d647-455c-8919-300cc35d7abf is present because although NodeScopeResolver updates the Scope when processing each argument, and providing the Scope for the next argument, the rules that call FunctionCallParametersCheck simply hook onto FuncCall or MethodCall, and use the Scope before the call to get all argument types. I wonder: If we were to write the analyser from scratch today, how would we go about it for this problem to never happen? I've long thought that NodeScopeResolver/TypeSpecifier/MutatingScope::resolveType should maybe be one thing that just goes through AST once. But I don't know how to tackle that we don't know beforehand what type they will ask for. |
|
Thank you! |
|
In regard to my comment: #4237 (comment) There would of course be a great performance improvement to the fact that |
closes phpstan/phpstan#13268
closes phpstan/phpstan#12087
closes phpstan/phpstan#9666
closes phpstan/phpstan#9445
closes phpstan/phpstan#7773