IfAnalyzer incorrectly updates outer scope

[ptomulik]

Consider this example, which correctly reports ReferenceConstraintViolation (dumb inserts into list may introduce discontinuites in indexint turning list into array):

https://psalm.dev/r/461cbad1b2

Now, adding dummy if(isset(...)){}, which has no chance to change flow of control, results with false negative (error disappears):

https://psalm.dev/r/dd0484a239

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/461cbad1b2

<?php

/**
 * @param list<string> $list
 */
function modify(array &$list, int $offset, string $val): void
{
    $list[$offset] = $val;
}

Psalm output (using commit 381a23b):

ERROR: ReferenceConstraintViolation - 6:24 - Variable $list is limited to values of type list<string> because it is passed by reference, non-empty-array<int, string> type found. Use @param-out to specify a different output type

https://psalm.dev/r/dd0484a239

<?php

/**
 * @param list<string> $list
 */
function modify(array &$list, int $offset, string $val): void
{
    if(isset($list[$offset])) { }
    $list[$offset] = $val;
}

Psalm output (using commit 381a23b):

No issues!

[ptomulik]

Handling list assignments is more interesting, just compare these two examples (and the very first example without if(isset(...)):

https://psalm.dev/r/7310523657

https://psalm.dev/r/14d1091480

The difference between above two is isset() vs !isset().

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/7310523657

<?php

/**
 * @param list<string> $list
 */
function modify(array &$list, int $offset, string $val): void
{
    if(isset($list[$offset])) {
        $list[$offset] = $val;
    }   
}

Psalm output (using commit 381a23b):

No issues!

https://psalm.dev/r/14d1091480

<?php

/**
 * @param list<string> $list
 */
function modify(array &$list, int $offset, string $val): void
{
    if(!isset($list[$offset])) {
        $list[$offset] = $val;
    }   
}

Psalm output (using commit 381a23b):

No issues!

[orklah]

Note: you can use @psalm-trace to see what Psalm think of the types at different places:
https://psalm.dev/r/c3879d8df6
https://psalm.dev/r/f7570574b5

So the difference there is Psalm seems to be convinced that the type is still a list but only after isset() is checked. According to your other snippets, it seems it doesn't even matter how isset was checked (checking the existence or absence do the same)

My guess is that there's a flaw in the way Psalm adds a dynamic offset to an existing list. It may be hard to solve, sometimes, Psalm makes some simplifications in order to allow a specific code construct but this creates edge cases like this

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/c3879d8df6

<?php

/**
 * @param list<string> $list
 */
function modify(array &$list, int $offset, string $val): void
{
    if(isset($list[$offset])) { 
    	/** @psalm-trace $list */;
    }
    /** @psalm-trace $list */;
    $list[$offset] = $val;
    /** @psalm-trace $list */;
}

Psalm output (using commit 381a23b):

INFO: Trace - 9:31 - $list: list<string>

INFO: Trace - 11:30 - $list: list<string>

INFO: Trace - 13:30 - $list: non-empty-list<string>

https://psalm.dev/r/f7570574b5

<?php

/**
 * @param list<string> $list
 */
function modify(array &$list, int $offset, string $val): void
{
    /** @psalm-trace $list */;
    $list[$offset] = $val;
    
    /** @psalm-trace $list */;
}

Psalm output (using commit 381a23b):

INFO: Trace - 8:30 - $list: list<string>

INFO: Trace - 11:30 - $list: non-empty-array<int, string>

ERROR: ReferenceConstraintViolation - 6:24 - Variable $list is limited to values of type list<string> because it is passed by reference, non-empty-array<int, string> type found. Use @param-out to specify a different output type

[ptomulik]

For the example with if(isset(...)) { }, the contents of outer_context changes here:

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

Line 289 in 381a23b

$outer_context->update(

[orklah]

yeah but potentially, you want this context to change (for example if you change the variable inside of a branch of the if or if one of the branch returns or something like that)

[ptomulik]

yeah but potentially, you want this context to change (for example if you change the variable inside of a branch of the if or if one of the branch returns or something like that)

sure, but it should be done correctly, currently it's buggy, the problem appears to be in another place, so that 289 line is only a starting point for deeper search. Anyway, the fix is on my way.

[ygottschalk]

possibly related or same issue: #9412

[ptomulik]

possibly related or same issue: #9412

Yes, it's related.