Address flaw in type reconciliation on array references #8290
Conversation
| @@ -124,7 +124,7 @@ public static function reconcileKeyedTypes( | |||
|
|
|||
| $cloned_referenceds = []; | |||
| foreach ($existing_references as $reference => $referenced) { | |||
| if (!isset($cloned_referenceds[$referenced])) { | |||
| if (!isset($cloned_referenceds[$referenced]) && isset($old_existing_types[$referenced])) { | |||
There was a problem hiding this comment.
@AndrolGenhald I'm not sure whether this is enough, or
- whether there should be a fallback union type assigned to
$existing_types[$referenced], or - whether the
$referencedvalue (e.g.'$doesNotMatter[$a]') should to be split to corresponding types of$doesNotMatter+$aand traversed
Thanks in advance for any hint.
There was a problem hiding this comment.
I'll step through it with your test tonight or tomorrow and see what's going on. Thanks for the reproduction and possible fix!
There was a problem hiding this comment.
Seems like it needs to have a type, I just ran into another error in a project having this patch applied:
Uncaught Error: Call to a member function equals() on null in vendor/vimeo/psalm/src/Psalm/Context.php:524
Line 521 in a32e63f
There was a problem hiding this comment.
Oh, yeah, having nulls in there is not a good idea. I think I know what's going on though; when we do the check against $a it potentially changes $a's type, which means we remove everything dependent on $a from Context::$vars_in_scope (somewhere, can't remember where right now) because $doesNotMatter[$a] doesn't have the same type if $a has changed. When that happens, we also need to do something to any references that exist to $doesNotMatter[$a], probably by changing whatever code removes $doesNotMatter[$a] to use the normal process by calling Context::remove() instead of whatever it's doing now.
Of course this means the fact that it's a reference will be lost, but like I said before, combining references with arrays and objects is just asking for trouble, and there's not much we can do about it.
Edit: oh, this is actually because it's removed from $existing_types in the same method further down, before it even gets to the context. Line 339 (and possibly 724) are where the issue is happening. I think it needs to do something sort of like Context:569-588, but probably a bit different using $reference_map.
|
I think I have a fix that should work, but I need to find a bit more time to finish it since it requires modifying callers of |
Related: #8289