Fix MutatingScope->compareVariableTypeHolders() #4641
Conversation
|
This pull request has been marked as ready for review. |
src/Analyser/MutatingScope.php
Outdated
| unset($otherVariableTypeHolders[$variableExprString]); | ||
| } | ||
|
|
||
| if (count($otherVariableTypeHolders) > 0) { |
There was a problem hiding this comment.
in the loop this array is reduced one offset at a time, which only makes sense if we check afterwards whether $otherVariableTypeHolders contained elements which were not contained in $variableTypeHolders
There was a problem hiding this comment.
Can you write a failing test against this? Or is it just about performance?
There was a problem hiding this comment.
yeah, I think I need to come up with a test. looks like a bug to me.
not perf related, just stumbled over it, while looking into #4640 (comment)
There was a problem hiding this comment.
Can you write a failing test against this? Or is it just about performance?
I looked into it and tried. I think a case where $other has more types/conditions than $this cannot happen, because in all cases where we call MutatingScope->equals() we do it like $bodyScope->equals($prevScope), meaning we compare a generalized scope (one with reduced information) with one with more information.
this means the scope we get as parameter will always have less information than $this so the theoretical case this PR tried to fix can never happen. but this also means there is dead code in the method, which I now have deleted.
src/Analyser/MutatingScope.php
Outdated
| unset($otherVariableTypeHolders[$variableExprString]); | ||
| } | ||
|
|
||
| if (count($otherVariableTypeHolders) > 0) { |
There was a problem hiding this comment.
Can you write a failing test against this? Or is it just about performance?
|
Thank you! |
3 participants
No description provided.