Psalm: Overriding list items by existing key makes it not a list anymore

[BenMorel]

When overwriting list keys this way:

$list = ['a', 'b', 'c'];

foreach ($list as $key => $value) {
    $list[$key] = $value . '!';   
}

The list becomes an array<int, string>, even though it should stay a list<string>.

https://psalm.dev/r/5e22300c5c

Note: I do know that I'd better use array_map() in such a case!

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/5e22300c5c

<?php

/**
 * @return list<string>
 */
function getList(): array
{
    return ['a', 'b', 'c'];
}

/**
 * @param list<string> $list
 * @psalm-suppress UnusedParam
 */
function takesList(array $list): void
{
}

$list = getList();

foreach ($list as $key => $value) {
    $list[$key] = $value . '!';   
}

takesList($list);

Psalm output (using commit ea314cc):

ERROR: ArgumentTypeCoercion - 25:11 - Argument 1 of takesList expects list<string>, parent type array<int, string> provided

[muglug]

Yeah, this is a known bug (as in, I know about it because I've had to suppress the issue in Psalm’s own codebase).

It's definitely a tricky one though. For this to work, Psalm would need to be sure $key was a member of $list's keys.

So something like this:

$list = ['a', 'b', 'c'];

foreach ($list as $key => $value) {
    $list[$key + 1] = $value . '!';   
}

Would still invalidate the list type.

As would

$list = ['a', 'b', 'c'];

foreach ($list as $key => $value) {
    if (rand(0, 1)) {
        array_pop($list);
    }
    $list[$key] = $value . '!';   
}

[muglug]

It's so hard, in fact, that I'm tempted to close this 😕

[alexanderpas]

Some form of array taint analysis might be the solution here.

If we have a list, there are only a few actions we can do, without breaking the list data type.

• Writing to any key between the start (0) and the end of the list.
• Removing the last item from the list.
• Writing to the position which would be the next item in the list.

While any of the following actions would break the list datatype automatically:

• Writing to any key which is beyond the next item on the list.
• Removing any key which is not the last item on the list.

[BenMorel]

• Removing the last item from the list.

Actually, all array_push()/pop()/shift()/unshift() should be allowed?

[weirdan]

Actually, all array_push()/pop()/shift()/unshift() should be allowed?

They are allowed: https://psalm.dev/r/cc9f4c576b

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/cc9f4c576b

<?php

/**
 * @param list<mixed> $l
 * @return list<mixed>
 */
function pop(array $l): array {
    array_pop($l);
    return $l;
}

/**
 * @param list<mixed> $l
 * @return list<mixed>
 */
function push(array $l): array {
    array_push($l, 1);
    return $l;
}

/**
 * @param list<mixed> $l
 * @return list<mixed>
 */
function push2(array $l): array {
    $l[] = 1;
    return $l;
}

/**
 * @param list<mixed> $l
 * @return list<mixed>
 */
function shift(array $l): array {
    array_shift($l);
    return $l;
}

/**
 * @param list<mixed> $l
 * @return list<mixed>
 */
function unshift(array $l): array {
    array_unshift($l, 1);
    return $l;
}

Psalm output (using commit cec8d71):

No issues!

[muglug]

This is currently blocked by #4794, but once that's fixed it should be _relatively_ easy

[muglug]

Still quite hard

[muglug]

I've fixed the examples given in the issue description, but there are still some false-positives I'm seeing in Psalm's own codebase that I would have hoped this had fixed.

[muglug]

For reference: https://github.com/vimeo/psalm/blob/a4ac52aea467d2e53f75db17c913195348dc200a/src/Psalm/Internal/Type/AssertionReconciler.php#L745

I think it’s due to nested loops

[muglug]

ACTUALLY never mind, they were all special cases and this _is_ fixed.

[BenMorel]

This is awesome, thanks! 🎉