Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PossiblyInvalidArrayOffset - Psalm wrongly expects string array keys when only int is possible #4479

Closed
BenMorel opened this issue Nov 4, 2020 · 5 comments
Labels

Comments

@BenMorel
Copy link
Contributor

BenMorel commented Nov 4, 2020

A snippet is worth a thousand words, so here we go:

https://psalm.dev/r/62dbf6620f

Bug 🐛

As you can see, the OWNERSHIP_STATUS_TRANSLATION_KEYS constant can only have OwnershipStatus::* keys, so int only.

But Psalm complains:

ERROR: PossiblyInvalidArrayOffset - 34:16 - Cannot access value on variable Translator::OWNERSHIP_STATUS_TRANSLATION_KEYS using offset value of 2, expecting '1' or '2'

Not sure where the string type '1' or '2' comes from?

Feature request

I'll happily create another issue for this if you wish, but as you can see, Psalm cannot infer the return type of getTranslationKey(), even though it looks pretty obvious.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/62dbf6620f
<?php

class OwnershipStatus
{
    public const NOT_OWNED = 0;
    public const OWNED     = 1;
    public const WISHED    = 2;
}

class BookActivity {
    /**
     * @psalm-var OwnershipStatus::*
     */
    private int $ownershipStatus = OwnershipStatus::NOT_OWNED;
    
    /**
     * @psalm-return OwnershipStatus::*
     */
    public function getOwnershipStatus(): int
    {
        return $this->ownershipStatus;
    }
}

class Translator
{
    private const OWNERSHIP_STATUS_TRANSLATION_KEYS = [
        OwnershipStatus::OWNED  => 'Activity:OwnBook:OWNED',
        OwnershipStatus::WISHED => 'Activity:OwnBook:WISHED'
    ];

    public function getTranslationKey(BookActivity $activity): string
    {
        return self::OWNERSHIP_STATUS_TRANSLATION_KEYS[$activity->getOwnershipStatus()];
    }
}
Psalm output (using commit b5a3f45):

ERROR: PossiblyInvalidArrayOffset - 34:16 - Cannot access value on variable Translator::OWNERSHIP_STATUS_TRANSLATION_KEYS using offset value of 2, expecting '1' or '2'

INFO: MixedReturnStatement - 34:16 - Could not infer a return type

INFO: MixedInferredReturnType - 32:64 - Could not verify return type 'string' for Translator::getTranslationKey

@weirdan
Copy link
Collaborator

weirdan commented Nov 4, 2020

The message is obviously misleading, but the code is not safe either. You're not handling OwnershipStatus::NOT_OWNED, and that's why Psalm complains. See https://psalm.dev/r/ff55667bb8 (if you comment out the assert, you get the original issue reported).

The message should instead read: Cannot access value on variable Translator::OWNERSHIP_STATUS_TRANSLATION_KEYS using offset value of 0, expecting 1 or 2

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ff55667bb8
<?php

class OwnershipStatus
{
    public const NOT_OWNED = 0;
    public const OWNED     = 1;
    public const WISHED    = 2;
}

class BookActivity {
    /**
     * @psalm-var OwnershipStatus::*
     */
    private int $ownershipStatus = OwnershipStatus::NOT_OWNED;
    
    /**
     * @psalm-return OwnershipStatus::*
     */
    public function getOwnershipStatus(): int
    {
        return $this->ownershipStatus;
    }
}

class Translator
{
    private const OWNERSHIP_STATUS_TRANSLATION_KEYS = [
        OwnershipStatus::OWNED  => 'Activity:OwnBook:OWNED',
        OwnershipStatus::WISHED => 'Activity:OwnBook:WISHED'
    ];

    public function getTranslationKey(BookActivity $activity): string
    {
        /** @psalm-trace $status */
        $status = $activity->getOwnershipStatus();
        assert($status !== 0);
        /** @psalm-trace $status */;
        return self::OWNERSHIP_STATUS_TRANSLATION_KEYS[$status];
    }
}
Psalm output (using commit 3e9c5d3):

INFO: Trace - 35:9 - $status: int(0)|int(1)|int(2)

INFO: Trace - 37:36 - $status: int(1)|int(2)

@weirdan weirdan added the bug label Nov 4, 2020
@BenMorel
Copy link
Contributor Author

BenMorel commented Nov 5, 2020

@weirdan Indeed, good point! This is definitely a cosmetic issue then. The exception message is very misleading. It should indeed be "using offset value of 0, expecting 1 or 2".

Leaving this open as a request to fix the exception message then.

By the way, is there a way to declare that a method can accept/return any of these constants, but NOT_OWNED? Or do I have to list all possible values?

@muglug muglug closed this as completed in d47d817 Nov 5, 2020
@BenMorel
Copy link
Contributor Author

BenMorel commented Nov 5, 2020

Thanks a lot, @muglug !

danog pushed a commit to danog/psalm that referenced this issue Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants