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

Constant as separator in explode() produces false-positives #7831

Closed
gndk opened this issue Mar 30, 2022 · 1 comment · Fixed by #9016
Closed

Constant as separator in explode() produces false-positives #7831

gndk opened this issue Mar 30, 2022 · 1 comment · Fixed by #9016

Comments

@gndk
Copy link
Contributor

gndk commented Mar 30, 2022

Errors with const: https://psalm.dev/r/d5a6c69fdc
No errors without const: https://psalm.dev/r/76b2904ea2

Some relevant comments from Slack:

Error comes from here:

if ($call_args[0]->value instanceof PhpParser\Node\Scalar\String_) {

this condition is suboptimal because it checks that the parameter is a literal string, not a variable nor a a constant

Yes with SPLIT_SEPARATOR psalm thinks, the separator could be empty. which would result in returning false before php 8.0.
So I think there are two bugs. psalm should detect that the constant is not empty. and psalm should know, that explode can not return false anymore since php 8.0.

@psalm-github-bot
Copy link

I found these snippets:

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

declare(strict_types=1);

namespace App\Domain\Addressing;

use RuntimeException;

use function array_key_exists;
use function explode;
use function sprintf;

/**
 * @psalm-immutable
 */
final class Name
{
    private const SPLIT_SEPARATOR             = ' ';
    private const SPLIT_ARRAY_KEY_GIVEN_NAME  = 'givenName';
    private const SPLIT_ARRAY_KEY_FAMILY_NAME = 'familyName';
    
    /**
     * @return array{givenName: string, familyName: string}
     */
    private function split(string $name): array
    {
        $parts = explode(self::SPLIT_SEPARATOR, $name, 2);

        if (! array_key_exists(0, $parts) || ! array_key_exists(1, $parts)) {
            throw new RuntimeException();
        }

        return [
            self::SPLIT_ARRAY_KEY_GIVEN_NAME => $parts[0],
            self::SPLIT_ARRAY_KEY_FAMILY_NAME => $parts[1],
        ];
    }
}
Psalm output (using commit 5baf85e):

INFO: LessSpecificReturnStatement - 33:16 - The type 'array{familyName: null|string, givenName: null|string}' is more general than the declared return type 'array{familyName: string, givenName: string}' for App\Domain\Addressing\Name::split

INFO: MoreSpecificReturnType - 23:16 - The declared return type 'array{familyName: string, givenName: string}' for App\Domain\Addressing\Name::split is more specific than the inferred return type 'array{familyName: null|string, givenName: null|string}'
https://psalm.dev/r/76b2904ea2
<?php

declare(strict_types=1);

namespace App\Domain\Addressing;

use RuntimeException;

use function array_key_exists;
use function explode;
use function sprintf;

/**
 * @psalm-immutable
 */
final class Name
{
    private const SPLIT_SEPARATOR             = ' ';
    private const SPLIT_ARRAY_KEY_GIVEN_NAME  = 'givenName';
    private const SPLIT_ARRAY_KEY_FAMILY_NAME = 'familyName';
    
    /**
     * @return array{givenName: string, familyName: string}
     */
    private function split(string $name): array
    {
        $parts = explode(' ', $name, 2);

        if (! array_key_exists(0, $parts) || ! array_key_exists(1, $parts)) {
            throw new RuntimeException();
        }

        return [
            self::SPLIT_ARRAY_KEY_GIVEN_NAME => $parts[0],
            self::SPLIT_ARRAY_KEY_FAMILY_NAME => $parts[1],
        ];
    }
}
Psalm output (using commit 5baf85e):

No issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants