Skip to content

Comments

Unsetting optional offset might still give a list.#5029

Merged
ondrejmirtes merged 10 commits intophpstan:2.1.xfrom
VincentLanglet:fix14177
Feb 23, 2026
Merged

Unsetting optional offset might still give a list.#5029
ondrejmirtes merged 10 commits intophpstan:2.1.xfrom
VincentLanglet:fix14177

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Feb 22, 2026

When doing array_key_exists on list{0: string, 1: string, 2?: string, 3?: string},
the ArrayKeyExistsFunctionTypeSpecifyingExtension is specify HasOffsetType and in the else condition we're calling tryRemove on this HasOffsetType.

ConstantArrayType::tryRemove do

		if ($typeToRemove instanceof HasOffsetType) {
			return $this->unsetOffset($typeToRemove->getOffsetType());
		}

but the new array created is always with $this->isList = Ternary::no.

The issue is that the intersection with List gives Never ; see
https://phpstan.org/r/c8177c3c-a643-43dc-b62f-b207740d75f8

Technically,

  • removing an optional key could still be a list
  • removing the last non optional key (which is not the easiest check since they might not be ordered) could still be a list

This is fully related to phpstan/phpstan#11628
see https://phpstan.org/r/fb18be32-9fba-42ba-8815-b767734cac6a

This is kinda tricky as changing $newIsList = TrinaryLogic::createMaybe() to maybe or true will then creates an issue if I append an element later (cause PHPStan won't detect it's not a list anymore).

Also, some extra thought: when we're inside the else part of an array_key_exists call we're not really calling unset on the array. So maybe a simpler way to solve this, is to

  • Ignore the issue with unset
  • Solving the array_key_exists issue by
    • Adding an optional param to ConstantArrayType::unsetOffset like something $newIsList or preserve list
    • Changing the call inside ConstantArrayType::tryRemove to override the behavior of isList when trying to remove HasOffsetType and HasOffsetValueType.

Closes phpstan/phpstan#14177

ondrejmirtes and others added 2 commits February 22, 2026 18:23
…ing keys

When `ConstantArrayType::unsetOffset()` removed an optional trailing key
from a list type, it unconditionally set `isList` to `No`. This caused
`TypeCombinator::remove()` to produce `NeverType` when re-intersecting
with `AccessoryArrayListType`, since the list accessor rejects non-list
arrays. The `NeverType` falsy branch then collapsed scope merging after
conditionals like ternaries, propagating the narrowed (truthy) type
beyond the conditional and triggering false "always true" reports on
subsequent `array_key_exists()` calls.

The fix preserves the original `isList` value when unsetting an optional
key whose removal leaves the remaining keys as consecutive integers
starting from 0.

Fixes phpstan/phpstan#14177
return new self($newKeyTypes, $newValueTypes, $this->nextAutoIndexes, $newOptionalKeys, TrinaryLogic::createNo());
$newIsList = TrinaryLogic::createNo();
if (!$this->isList->no() && in_array($i, $this->optionalKeys, true)) {
$newIsList = TrinaryLogic::createMaybe();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not always, right? The new type is a list only if we're removing an optional key and:

  • The key is the last one
  • Or all of the keys after it are also optional

If you're going to implement this logic, add failing tests first 😊

Copy link
Contributor

@staabm staabm Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per phpstan/phpstan#12768 even unsetting the last item makes it no longer a list because of internal pointer and append with [] will create a hole

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but the point is that we're unsetting something that might not be on the array. So it might still be a list.

@VincentLanglet VincentLanglet changed the title Fix14177 Unsetting optional offset might still give a list. Feb 22, 2026
@VincentLanglet VincentLanglet marked this pull request as ready for review February 22, 2026 22:59
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

* So the nextAutoIndexes won't change, and the array might still be a list even with PHPStan definition.
*
* @param list<ConstantIntegerType|ConstantStringType> $newKeyTypes
* @param int[] $newOptionalKeys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like these alignments. There should be just a single space between the type and variable.

{
assertType('true', array_is_list($b));
unset($b[1]);
assertType('false', array_is_list($b)); // Could be true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could not. When you're unsetting an existing key, adding a new one will create a gap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf https://3v4l.org/4GBOm#v8.3.30

It's still a list, and it change when we add back a new element
So technically it could be true, and PHPStan should be aware it's false as soon as we add back an element.

But I agree it's harder.

I removed the comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the logic is unsound when passing the array around to function arguments.

When we accept a list, it must mean that after appending a new item, it's still a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Maybe the right fix will be to introduce a DynamicReturnTypeExtension for array_is_list to handle this special case.

Anyway, this PR should be ok to fix the issue 14177 so far.

* @param int[] $newOptionalKeys
* @param int[] $newOptionalKeys
*/
private static function isListAfterUnset(array $newKeyTypes, array $newOptionalKeys, TrinaryLogic $arrayIsList, bool $unsetOptionalKey): TrinaryLogic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might be able to kill the mutation with somethig like

https://phpstan.org/r/53bdb42a-d790-4576-aa47-62e33eb40865

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, it will be an union of two ConstantArrayType ; and each one will be handled separatly by this method.

Also, the tests are failing locally with the mutation ; dunno why it's reported as not caught then.

@ondrejmirtes ondrejmirtes merged commit ae822a8 into phpstan:2.1.x Feb 23, 2026
654 of 665 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants