ensureArrayString/Int/Offset seems to be unsound for non-literal array offsets

[calvinalkan]

https://psalm.dev/r/4bae3e6a7b

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/4bae3e6a7b

<?php

class Ensure
{
    
    public static function bool(array $state, string $key) :bool
    {
        $value = $state['this_rightfully_errors'];
		$possible_not_set = $state[$key];

        if(!is_bool($value)){
            throw new \InvalidArgumentException();
        }
        
        return $value;
    }
    
}

Psalm output (using commit 16b24bd):

INFO: PossiblyUndefinedStringArrayOffset - 8:18 - Possibly undefined array offset ''this_rightfully_errors'' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: MixedAssignment - 9:3 - Unable to determine the type that $possible_not_set is being assigned to

INFO: UnusedVariable - 9:3 - $possible_not_set is never referenced or the value is not used

[calvinalkan]

The responsible method seems to be:

ArrayAnalyzer::checkLiteralStringArrayOffset

which only raises this issue inside this top-level if statement:

if ($offset_type->hasLiteralString() && !$expected_offset_type->hasLiteralClassString() ) {
// issue raised in here.
}

I think the issue should also be raised if $offset_type is TString|TNonEmptyString|etc.

[calvinalkan]

Also, if any string literal is known to be in the array, other string literals are assumed to be in the array aswell.

https://psalm.dev/r/c07a298ef3

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/c07a298ef3

<?php

/**
 * @param array<string,string> $input
 * @param "foo"|"bar" $key
 */
function test(array $input, string $key) :void {

    if (array_key_exists("foo", $input)) {
        $safe = $input["foo"];
        echo "$safe";
        
        $oops = $input[$key];
        echo "$oops";
    }
    
}

Psalm output (using commit 16b24bd):

No issues!

[calvinalkan]

A couple more false negatives:

https://psalm.dev/r/db19108369

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/db19108369

<?php

declare(strict_types=1);

const FOO = 'foo';

class BarClass {
    const BAR = 'bar';

    public static function get(): string
    {
        return 'string';
    }
}

/**
 * @psalm-suppress MixedAssignment
 * @psalm-suppress UnusedVariable
 * @psalm-suppress UnusedParam
 *
 * @param array<array-key,mixed> $input
 */
function keyIsString(array $input, string $key, string $key2, string $key3): void {

    // Should error
    $value = $input[FOO];
    
    // Should error
    $value = $input[BarClass::BAR];
    
    // Should error, but does not.
    $value = $input[BarClass::get()];

    // Must be okay.
    $value3 = $input[$key3] ?? 'foo';

    // Should error, but does not.
    $value2 = $input[$key2];

    // Should not error with isset.
    if (!isset($input[$key])){
        return;
    }

    // Should not error after isset.
    $value = $input[$key];
}

/**
 * @param array<string,string> $input
 * @param "foo"|"bar" $key
 */
function test(array $input, string $key) :void {

    if (array_key_exists("foo", $input)) {
        $safe = $input["foo"];
        echo "$safe";

        // This must error - It could actually be null.
        $oops = $input[$key];
        echo "$oops";
    }

}

/**
 * @return array<string,string>
 */
function getArray() :array
{
    return [];
}


$arr = getArray();

$v = str_repeat((string) rand(0, 1), 10);
// Must error
$func_call = $arr[$v];
echo $func_call;

/**
 * @psalm-immutable
 */
class ObjectWithArr
{
    /**
     * @var array<string,string>
     */
    public array $arr = [];
    
    /**
     * @param array<string,string> $arr
     */
    public function __construct(array $arr)
    {
        $this->arr = $arr;
    }
    
}

$object = new ObjectWithArr(getArray());

if (isset($object->arr['foo'])) {
    // Okay, because object is immutable.
    echo $object->arr['foo'];
}

Psalm output (using commit 16b24bd):

INFO: PossiblyUndefinedStringArrayOffset - 26:14 - Possibly undefined array offset ''foo'' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: PossiblyUndefinedStringArrayOffset - 29:14 - Possibly undefined array offset ''bar'' is risky given expected type 'array-key'. Consider using isset beforehand.

[calvinalkan]

I have been testing the following modifications to ArrayFetchAnalyzer, it seems to catch all of the false negatives, without causing new false positives.

But I don't understand all the implications yet - complicated class.

<?php

declare(strict_types=1);

use Psalm\CodeLocation;
use Psalm\Context;
use Psalm\Internal\Analyzer\Statements\Expression\Fetch\ArrayFetchAnalyzer;
use Psalm\Internal\Analyzer\StatementsAnalyzer;
use Psalm\Issue\PossiblyUndefinedIntArrayOffset;
use Psalm\Issue\PossiblyUndefinedStringArrayOffset;
use Psalm\IssueBuffer;
use Psalm\Type\Atomic\TInt;
use Psalm\Type\Atomic\TLiteralInt;
use Psalm\Type\Atomic\TLiteralString;
use Psalm\Type\Atomic\TString;
use Psalm\Type\Union;

/**
 * @internal
 *
 * @see ArrayFetchAnalyzer::checkLiteralIntArrayOffset()
 * @see ArrayFetchAnalyzer::checkLiteralStringArrayOffset()
 * @see https://github.com/vimeo/psalm/issues/10992
 */
final class ArrayFetchAnalyzerPatch
{
    private static function checkLiteralIntArrayOffset(
        Union $offset_type,
        Union $expected_offset_type,
        ?string $array_var_id,
        PhpParser\Node\Expr\ArrayDimFetch $stmt,
        Context $context,
        StatementsAnalyzer $statements_analyzer
    ) :void {
        if ($context->inside_isset || $context->inside_unset) {
            return;
        }
        
        foreach ($offset_type->getAtomicTypes() as $offset_type_part) {
            if ( ! $offset_type_part instanceof TInt) {
                // This method always seems to be called, regardless of whether the array offset is an int.
                continue;
            }
            
            if ( ! $array_var_id) {
                // This could happen if we try to access an offset on the result of a function call,
                // or any other scenario where we don't know the array variable.
                IssueBuffer::maybeAdd(
                    new PossiblyUndefinedIntArrayOffset(
                        'Possibly undefined array offset: $array_var_id not known.',
                        new CodeLocation($statements_analyzer->getSource(), $stmt)
                    ),
                    $statements_analyzer->getSuppressedIssues()
                );
                return;
            }
            
            if (
                ! $offset_type_part instanceof TLiteralInt
                || ! isset($context->vars_in_scope[$index = $array_var_id.'['.$offset_type_part->value.']'])
                || $context->vars_in_scope[$index]->possibly_undefined
            ) {
                $prefix = $stmt->dim instanceof PhpParser\Node\Expr\FuncCall
                    ? 'Possibly undefined array offset from function call return type'
                    : 'Possibly undefined array offset';
                
                IssueBuffer::maybeAdd(
                    new PossiblyUndefinedIntArrayOffset(
                        $prefix.' \''
                        .$offset_type_part->getId().'\' '
                        .'is risky given expected type \''
                        .$expected_offset_type->getId().'\'.'
                        .' Consider using isset beforehand.',
                        new CodeLocation($statements_analyzer->getSource(), $stmt)
                    ),
                    $statements_analyzer->getSuppressedIssues()
                );
            }
        }
    }
    
    private static function checkLiteralStringArrayOffset(
        Union $offset_type,
        Union $expected_offset_type,
        ?string $array_var_id,
        PhpParser\Node\Expr\ArrayDimFetch $stmt,
        Context $context,
        StatementsAnalyzer $statements_analyzer
    ) :void {
        if ($context->inside_isset || $context->inside_unset) {
            return;
        }
        
        // This method seems only to be called the first time we try to fetch this particular
        // array index.
        // Unless we know that this exact value in scope, it's risky to fetch the array key.
        foreach ($offset_type->getAtomicTypes() as $offset_type_part) {
            if (!$offset_type_part instanceof TString){
                continue;
            }
            
            if ( ! $array_var_id) {
                // This could happen if we try to access an offset on the result of a function call,
                // or any other scenario where we don't know the array variable.
                IssueBuffer::maybeAdd(
                    new PossiblyUndefinedStringArrayOffset(
                        'Possibly undefined array offset: $array_var_id not known.',
                        new CodeLocation($statements_analyzer->getSource(), $stmt)
                    ),
                    $statements_analyzer->getSuppressedIssues()
                );
                return;
            }
            
            if (
                ! $offset_type_part instanceof TLiteralString
                || ! isset($context->vars_in_scope[$index = $array_var_id.'[\''.$offset_type_part->value.'\']'])
                || $context->vars_in_scope[$index]->possibly_undefined
            ) {
                $prefix = $stmt->dim instanceof PhpParser\Node\Expr\FuncCall
                    ? 'Possibly undefined array offset from function call return type'
                    : 'Possibly undefined array offset';
                
                IssueBuffer::maybeAdd(
                    new PossiblyUndefinedStringArrayOffset(
                        $prefix.' \''
                        .$offset_type_part->getId().'\' '
                        .'is risky given expected type \''
                        .$expected_offset_type->getId().'\'.'
                        .' Consider using isset beforehand.',
                        new CodeLocation($statements_analyzer->getSource(), $stmt)
                    ),
                    $statements_analyzer->getSuppressedIssues()
                );
            }
        }
    }
}