Keep list on unset() with nested dim-fetch #3964
Conversation
|
This pull request has been marked as ready for review. |
|
Looks like this fixes a few more issues :) https://github.com/phpstan/phpstan-src/actions/runs/14669281251 Please add regression tests. |
|
Nice 👍 |
src/Type/IntersectionType.php
Outdated
| @@ -799,6 +799,9 @@ public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $uni | |||
| } | |||
|
|
|||
| $result = $this->intersectTypes(static fn (Type $type): Type => $type->setOffsetValueType($offsetType, $valueType, $unionValues)); | |||
| if ($this->isList()->yes() && $valueType->isArray()->yes()) { | |||
| $result = TypeCombinator::intersect($result, new AccessoryArrayListType()); | |||
There was a problem hiding this comment.
Again, I don't get why does it matter what $valueType is.
There was a problem hiding this comment.
the idea was:
/** @param list<array<int>> $list */
function doFoo($list, int $i) {
$list[$i][3] = 123;
}
when setting offset $i on $list to array(3 => 123), it does not turn $list into array.
(which is only true when $i is a existing offset)
There was a problem hiding this comment.
But that's true for anything, not just arrays. I feel like we should call setExistingOffsetValueType more if we're sure the offset exists.
There was a problem hiding this comment.
This just doesn't make sense. A simple code:
/** @param list<array<int>> $list */
function doFoo($list, int $i) {
$list[$i][3] = 123;
\PHPStan\dumpType($list);
}We don't know whether $list[$i] exists. 2.1.x is correct to dump this as non-empty-array<int, non-empty-array<int>&hasOffsetValue(3, 123)>.
Your branch dumps this as non-empty-list<non-empty-array<int>&hasOffsetValue(3, 123)> which is wrong.
There was a problem hiding this comment.
For example, with code like this:
foreach ($list as $i => $value) {
$list[$i][3] = 123;
}We should preserve $list being a list because $list[$i] exists and this information is already carried in Scope (hasExpressionType).
Right now setExistingOffsetValueType is only called for unset() but we could call it for when we're sure the offset exists.
There was a problem hiding this comment.
I am now using setExistingOffsetValueType more, but in the process somehow broke some tests.
need to investigate further, but as I kind of working thru the fog was thinking it would be worth sharing the progress to verify I am working in the right direction
| assertType("non-empty-array<array<mixed>>&hasOffsetValue('Review', array<mixed~'review', mixed>)&hasOffsetValue('SurveyInvitation', array<mixed~'review', mixed>)", $review); | ||
| assertType("non-empty-array<array<mixed>>&hasOffsetValue('Review', array{id: null, text: null, answer: null})&hasOffsetValue('SurveyInvitation', array<mixed~'review', mixed>)", $review); |
There was a problem hiding this comment.
I think this expectation change is correct.. previously unset($review['SurveyInvitation']['review']); affected the offset Review (uppercase R) which was a bug IMO
| return new self( | ||
| $this->keyType, | ||
| TypeCombinator::union($this->itemType, $valueType), | ||
| if ($offsetType instanceof ConstantStringType || $offsetType instanceof ConstantIntegerType) { |
There was a problem hiding this comment.
The idea is that we're overwriting an existing offset so $this->keyType should stay intact as is in the original version of this method.
Of course setExistingOffsetValueType was used when unsetting an offset so some details somewhere might be wrong, but what you're doing here now is basically the same thing setOffsetValueType is doing so it seems wrong.
closes phpstan/phpstan#12927
closes phpstan/phpstan#12330
closes phpstan/phpstan#11171
closes phpstan/phpstan#8282
closes phpstan/phpstan#12754