Inferred array keys marked as optional when source referenced in try catch

[gready-hub]

Hello Devs.

Anytime we reference a source object-property through a try-catch Psalm makes the conclusion that the resulting array-key is possibly unset, even though the object property has no bearing on the array-key's existence.

Could possibly assist with a PR once this has behaviour has been verified to be invalid, and perhaps I could get a point in the right direction as well.

    /** @return array{ID: string} */
    public function bar(): array {
        $entity = new Entity('1a2b3c');

        try {
            // Simply by referencing the object property within a try-catch causes the
            // inferred array shape to be modified
            $entity->id;
        } catch (Exception $e) {
            // do nothing?
        }
        
        // Key is now optional when it is most certainly not
        // Psalm infers type: array{ID?: string}
        return ['ID' => $entity->id];
    }

https://psalm.dev/r/a915915af4

Thanks!

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/a915915af4

<?php

/**
 * @psalm-immutable
 */
final class Entity {
    public function __construct(public string $id) {}
}

class Foo {
    /** @return array{ID: string} */
    public function bar(): array {
        $entity = new Entity('1a2b3c');

        try {
            $entity->id;
        } catch (Exception $e) {
            // do nothing?
        }
        
        return ['ID' => $entity->id];
    }
}

echo json_encode((new Foo)->bar());

Psalm output (using commit bf57d59):

ERROR: InvalidReturnStatement - 21:16 - The inferred type 'array{ID?: string}' does not match the declared return type 'array{ID: string}' for Foo::bar due to additional array shape fields (ID)

ERROR: InvalidReturnType - 11:17 - The declared return type 'array{ID: string}' for Foo::bar is incorrect, got 'array{ID?: string}' which is different due to additional array shape fields (ID)

[kkmuffme]

I guess the problem here is that referencing it in the try sets possibly_undefined or possibly_undefined_from_try to true which then spills over to the array assignment after it.

I suggest you start looking at TryAnalyzer where setPossiblyUndefined is called or set to true, shouldn't be too hard.

[SCIF]

@kkmuffme , I found similar issue: https://psalm.dev/r/88913cdfa4 Should I file as a new ticket or it's exactly the same function used/buggy?

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/88913cdfa4

<?php

class A {
    /** @var array{b: list<int>} */
	private array $items;
    
    public function __construct() {
        $this->items = ['b' => [3]];
    }
    
    public function some(): void
    {
        $key = 'b';
        /** @psalm-trace $this->items */;
        if ([] === $this->items[$key]) {
            return;
        }
        /** @psalm-trace $this->items */;
    }
}

Psalm output (using commit d916aa6):

INFO: Trace - 14:41 - $this->items: array{b: list<int>}

INFO: Trace - 18:41 - $this->items: array{b?: non-empty-list<int>}

[SCIF]

In my case it's important that array index is passed as a variable. If I would state b then it become working as expected

[gready-hub]

I think there might be (at least) two issues here:

1. Array keys are being marked as potentially undefined even when they, or their variable value, is never mutated
2. The try-catch analyser marks all variables used or referenced within try to be potentially undefined

I did some digging and did manage to resolve one of the bugs when using classes annotated with @psalm-immutable and with properties using @psalm-readonly. I did this by checking the ::has_mutations property in the TryCatchAnalyzer, however the has_mutations trick doesn't work when we're not using those annotations.