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

Fix array_key_exists() with all int literal keys #5197

Merged

Conversation

brainlock
Copy link
Contributor

@brainlock brainlock commented Feb 11, 2021

(https://psalm.dev/r/a0e5cf2ff8)

When checking code like the following:

<?php

function checkNegated(string $key): void {
    $arr = [
        0 => "foo",
        1 => "bar",
    ];

    if (!array_key_exists($key, $arr)) {
        printf("not found\n");
    }
}

function check(string $key): void {
    $arr = [
        0 => "foo",
        1 => "bar",
    ];

    if (array_key_exists($key, $arr)) {
        printf("found\n");
    }
}

the if in checkNegated would cause:

ERROR: RedundantCondition - 9:10 - Type string for $key is never =int(0)

This happens when the array keys are all int literals, but the "needle"
is a string.

array_key_exists() uses a loose equality comparison, but the generated
assertions for this specific case
(AssertionFinder::getArrayKeyExistsAssertions) was generating strict
equality clauses. This commit fixes it by changing the generated clause
from = to ~.

@psalm-github-bot
Copy link

I found these snippets:

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

$arr = [
	0 => "foo",
	1 => "bar",
];

$val_found = "1";
$val_not_found = "999";

if (array_key_exists($val_found, $arr)) {
    printf("found\n");
}

if (!array_key_exists($val_not_found, $arr)) {
    printf("not found\n");
}
Psalm output (using commit 257a1ca):

ERROR: RedundantCondition - 15:6 - Type "999" for $val_not_found is never =int(0)

ERROR: RedundantCondition - 15:6 - Type "999" for $val_not_found is never =int(1)

@brainlock
Copy link
Contributor Author

brainlock commented Feb 12, 2021

The psalm.dev snippet I first linked could be confusing, so here's a new one to highlight the actual bug (it cannot know that the key is never in the array):

https://psalm.dev/r/d770e4c1c5

(I'll update the commit message as well)

@psalm-github-bot
Copy link

I found these snippets:

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

function checkNegated(string $key): void {
    $arr = [
	    0 => "foo",
	    1 => "bar",
    ];
    
    if (!array_key_exists($key, $arr)) {
    	printf("not found\n");
	}
}

function check(string $key): void {
    $arr = [
	    0 => "foo",
	    1 => "bar",
    ];
    
    if (array_key_exists($key, $arr)) {
    	printf("found\n");
	}
}
Psalm output (using commit 257a1ca):

ERROR: RedundantCondition - 9:10 - Type string for $key is never =int(0)

ERROR: RedundantCondition - 9:10 - Type string for $key is never =int(1)

@brainlock brainlock force-pushed the fix-negate-array-key-exists-assertions branch from 435c062 to 1b8f3b5 Compare February 12, 2021 08:44
When checking code like the following:

```
<?php

function checkNegated(string $key): void {
    $arr = [
        0 => "foo",
        1 => "bar",
    ];

    if (!array_key_exists($key, $arr)) {
        printf("not found\n");
    }
}

function check(string $key): void {
    $arr = [
        0 => "foo",
        1 => "bar",
    ];

    if (array_key_exists($key, $arr)) {
        printf("found\n");
    }
}
```

the `if` in `checkNegated` would cause:

```
ERROR: RedundantCondition - 9:10 - Type string for $key is never =int(0)
```

This happens when the array keys are all int literals, but the "needle"
is a string.

`array_key_exists()` uses a loose equality comparison, but the generated
assertions for this specific case
(`AssertionFinder::getArrayKeyExistsAssertions`) was generating strict
equality clauses. This commit fixes it by changing the generated clause
from `=` to `~`.
@brainlock brainlock force-pushed the fix-negate-array-key-exists-assertions branch from 1b8f3b5 to 191426b Compare February 12, 2021 08:49
@muglug muglug merged commit 144bb37 into vimeo:master Feb 12, 2021
@muglug
Copy link
Collaborator

muglug commented Feb 12, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants