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

[calvinalkan]

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

[psalm-github-bot[bot]]

I found these snippets:

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

```php

[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[bot]]

I found these snippets:

https://psalm.dev/r/c07a298ef3

```php $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[bot]]

I found these snippets:

https://psalm.dev/r/db19108369

```php $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 $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 */ 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 */ public array $arr = []; /** * @param array $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()
                );
            }
        }
    }
}