vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

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

Open gready-hub opened 9 months ago

gready-hub commented 9 months ago

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] commented 9 months ago

I found these snippets:

https://psalm.dev/r/a915915af4 ```php 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 commented 9 months ago

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 commented 9 months ago

@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] commented 9 months ago

I found these snippets:

https://psalm.dev/r/88913cdfa4 ```php } */ 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} INFO: Trace - 18:41 - $this->items: array{b?: non-empty-list} ```
SCIF commented 9 months ago

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 commented 9 months ago

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.