Don't lose known offset-types in array_replace()#4826
Don't lose known offset-types in array_replace()#4826staabm wants to merge 7 commits intophpstan:2.1.xfrom
Conversation
| assertType("non-empty-array<'bar'|'foo'|int, string>", array_replace($array1, $array3)); | ||
| assertType("non-empty-array<'bar'|'foo'|int, string>", array_replace($array3, $array1)); | ||
| assertType("non-empty-array<'bar'|'foo'|int, string>&hasOffset('bar')&hasOffset('foo')", array_replace($array1, $array3)); | ||
| assertType("non-empty-array<'bar'|'foo'|int, string>&hasOffsetValue('bar', '2')&hasOffsetValue('foo', '1')", array_replace($array3, $array1)); |
There was a problem hiding this comment.
this kind of overlaps. should we do something about it, or is it fine?
|
@claudepache @VincentLanglet you might be interessted in reviewing this one |
| $hasOffsetValue = TrinaryLogic::createFromBoolean($argType->hasOffsetValueType($keyType)->yes()); | ||
| $offsetTypes[$keyType->getValue()] = [ |
There was a problem hiding this comment.
If multiple constantArray has the same Key, like array{1: int, 2: string}|array{1: int, 3: string} you will call hasOffsetValueType and getOffsetValueType two times to set the same value ; not sure if we could avoid the extra calls.
There was a problem hiding this comment.
as long as this is not identified as a bottleneck I don't want to complicate it further
| foreach ($offsetTypes as $key => [$hasOffsetValue, $offsetValueType]) { | ||
| $offsetTypes[$key] = [ | ||
| $hasOffsetValue->and(TrinaryLogic::createMaybe()), | ||
| new MixedType(), |
There was a problem hiding this comment.
Technically we could provide better than Mixed here.
Maybe a comment to explain there is no need to provide Union($offsetValueType, $argType->getArrayValues) since the value won't be used since we're only instantiating HasOffsetType
| if ($constArrays !== []) { | ||
| foreach ($constArrays as $constantArray) { | ||
| foreach ($constantArray->getKeyTypes() as $keyType) { | ||
| $hasOffsetValue = TrinaryLogic::createFromBoolean($argType->hasOffsetValueType($keyType)->yes()); |
There was a problem hiding this comment.
It's unclear to me reading this why you're doing
TrinaryLogic::createFromBoolean($argType->hasOffsetValueType($keyType)->yes())
and not directly
$argType->hasOffsetValueType($keyType)
You're transforming the Maybe into a No ?
There was a problem hiding this comment.
correct. I need the trinary, as I later on combine it with $hasOffsetValue->and(TrinaryLogic::createMaybe()) for non constant arrays (else case)
| } | ||
| if ($offsetTypes !== []) { | ||
| $knownOffsetValues = []; | ||
| foreach ($offsetTypes as $key => [$hasOffsetValue, $offsetType]) { |
There was a problem hiding this comment.
Eventually, looking at the usage you could have use array<Type|null> as offsetTypes:
- a Type means
HasOffsetValueType(the yes case) - null means
HasOffsetType(the maybe case)
There was a problem hiding this comment.
agree we could transform it. not sure this will lead to logic which is easier to follow?
2 participants
analog #4554