Fix #9433

[ptomulik]

A follow up of accidentally closed #9436. As it was said, shepherd tests started reporting errors related to list. This needs closer look.

[orklah]

I'm not familiar with the piece of code you changed. Can you explain what you found during your debug and if you know the reason why it fixes your issue?

It seems to me the new errors raised in Shepherd are linked to another issue in Psalm (that already pops up on other occasions) and your fix just revealed them but I'm not sure of that for now

[ptomulik]

I'm not familiar with the piece of code you changed. Can you explain what you found during your debug and if you know the reason why it fixes your issue?

The very first thing I found, was the line where the outer scope of the if(isset($list[$offset])) { } gets incorrectly updated. This was the following line:

psalm/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php

Line 289 in 381a23b

$outer_context->update(

After that line, the $outer_context->vars_in_scope contained extra variable: $list[$offset]. This only happened for $list declared as list<>. When declared as array<>, the $outer_context was not updated (this is what we expect also for list<>). The difference cause was $pre_assignment_else_redefined_vars provided as argument to IfAnalyzer::analyze(). Searching for place, where that argument is prepared, led me to the piece of code starting at the following line

psalm/src/Psalm/Type/Reconciler.php

Line 710 in 06266c2

if ($existing_key_type_part instanceof TArray) {

What I found was, that for array<> the code was setting a $new_base_type_candidate->possibly_undefined flag, whereas for list<> the flag was not set. For list<> (which is handled by TKeyedArray case here) the $new_base_type_candidate is prepared entirely within the following call

psalm/src/Psalm/Type/Reconciler.php

Line 764 in 06266c2

$new_base_type_candidate = $existing_key_type_part->getGenericValueType();

The method $existing_key_type_part->getGenericValueType() used here accepts boolean argument $possibly_undefined which defaults to false. So, I just passed true here.

[ptomulik]

To be honest, I'm not very sure about that hard-coded true passed to getGenericValueType(). It's just a first try. Perhaps some additional logic should be implemented to chose between true and false. The piece of code handling array<> doesn't seem so straightforward.

[orklah]

Well, that does seem consistent and I'm pretty sure the bug that appeared exists on other places too. I'd advise suppressing the new issues and go with it. If we somehow find an adverse effect, we'll at least add a new test and we'll be able to revert this if needed.

Can you fix the CI? I'll proceed to merge afterwards

[ptomulik]

Well, my nose says me it is the right place, except we shouldn't pass true (possiblyUndefined) unconditionally, the same way as we shouldn't pass false (default value for the parameter). I believe, the solution should be based on what is implemented for TArray:

if (($has_isset || $has_inverted_isset) && isset($new_assertions[$new_base_key])) {
                                if ($has_inverted_isset && $new_base_key === $key) {
                                    $new_base_type_candidate = $new_base_type_candidate->getBuilder();
                                    $new_base_type_candidate->addType(new TNull);
                                    $new_base_type_candidate->possibly_undefined = true;
                                    $new_base_type_candidate = $new_base_type_candidate->freeze();
                                } else {
                                    $new_base_type_candidate = $new_base_type_candidate->setPossiblyUndefined(true);
                                }

[orklah]

Well, the way I see it:
we're in getValueForKey, a method used to get a potential value from an array for a given offset of said array.
We get in this condition:

psalm/src/Psalm/Type/Reconciler.php

Line 759 in 1c19260

} elseif ($array_key[0] === '$' || ($array_key[0] !== '\'' && !is_numeric($array_key[0]))) {

because we have a TKeyedArray (we had a TList but is was converted to TKeyedArray at line 702

This condition in particular checks what the offset looks like. We enter here if we have an offset that is a variable (first operand) or if we have an offset that is not a literal string or a literal number (second operand).

So yeah, we could be more precise in some case. For example, if the offset is a non-numeric string, then it will never be defined because we have a list. If we can get the value for the variable in the offset, we could try to see if it's a known offset for the current array and keep the value defined.

But in other cases, if we have no way to know what the offset is, we're forced to return a possibly undefined type.

The thing is, we never do that kind of checks with TArray. Both condition will return possibly_undefined to true so I may be missing something here

[ptomulik]

Can you fix the CI? I'll proceed to merge afterwards

I see that CI fails same way for this PR and for master? Shouldn't we investigate this?

[weirdan]

I see that CI fails same way for this PR and for master?

Does it? I see one job failed on master, due to the composer timing out, but here you have an actual error reported by Psalm.

[ptomulik]

I see that CI fails same way for this PR and for master?

Does it? I see one job failed on master, due to the composer timing out, but here you have an actual error reported by Psalm.

Seems I was wrong. I've seen shepherd failing on my local working dir with master checked out, but this could be a false positive. Maybe it was psalm caching (I was running shepher test with this PR's branch checked out then switched to master and just rerun the test), or the fact that I haven't reinstalled packages with composer. In the meantime I've noticed shepherd tests failing on this repo on Github actions and this convinced me falsely that something is going on ... :)

Today I see, that tests pass on locally checked-out master, so it's fine. Sorry.

[ptomulik]

I think, I found explanation for the failing shepherd test. It's just revealed another variant of the bug we attempt to fix in this PR. To this end, we were concerned on this variant:

if (isset($list[$i])) { }
$list[$i] = something... // false negative (psalm assumes falsely that $list[$i] exists)

The other variant is (in a simplified form):

$list[$i] = expression (... $list[$i] ?? something... );

namely, it's this fragment of code from src/Psalm/Internal/Type/TypeCombiner.php (starting from line 554)

$combination->array_type_params[$i] = Type::combineUnionTypes(
  $combination->array_type_params[$i] ?? null,
  $type_param,
  $codebase,
  $overwrite_empty_array,
);

I think the best we can do at this moment is to just add @psalm-suppress here. The proper solution is to fix #9484, at least to make this example to not report false positive:

https://psalm.dev/r/8972d76050

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/8972d76050

<?php

class Test {
    /** @var list<int> */
    public array $list = [];
    
    /** @psalm-param list<int> $other */
    public function test(array $other): void
    {
        foreach ($other as $i => $v) {
            $this->list[$i] = $v;
        }
    }
}

Psalm output (using commit 9cf4f60):

ERROR: PropertyTypeCoercion - 11:13 - $this->list expects 'list<int>',  parent type 'non-empty-array<int<0, max>, int>' provided

[orklah]

Thanks!