Operand of type false is always falsy

[tdclaritum]

The following code

<?php

/** @var list<array{id: int, template_id: int}> $records */
$records = json_decode(file_get_contents('test.json'), true);
$processed = [];
foreach($records as $record) {
    if (empty($processed[$record['template_id']])) {
        $processed[$record['template_id']] = [];
    }
    $processed[$record['template_id']][] = $record['id'];
}

produces the following error

ERROR: TypeDoesNotContainType - a.php:7:9 - Operand of type false is always falsy (see https://psalm.dev/056)
    if (empty($processed[$record['template_id']])) {

but it shouldn't. It seems the issue is that psalm doesn't know the data inside $records. If $records is defined like

$records = [
    ['id' => 1, 'template_id' => 1],
    ['id' => 2, 'template_id' => 1],
    ['id' => 3, 'template_id' => 2],
];

there is no error reported.

Reproduce on psalm.dev: https://psalm.dev/r/5921391c4a

Workaround: replace empty() with ! isset() - if (empty($processed[$record['template_id']])) { => if (! isset($processed[$record['template_id']])) {

Edit: syntax highlighting

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/5921391c4a

<?php

/** @var list<array{id: int, template_id: int}> $records */
$records = json_decode(file_get_contents('test.json'), true);
$processed = [];
foreach($records as $record) {
    if (empty($processed[$record['template_id']])) {
        $processed[$record['template_id']] = [];
    }
    $processed[$record['template_id']][] = $record['id'];
}

Psalm output (using commit 3c90054):

ERROR: TypeDoesNotContainType - 7:9 - Operand of type false is always falsy

[edsrzf]

It looks like this behavior was introduced in 5.20 with #10502. I ran into it when trying to update from 5.18 to 5.22.

[edsrzf]

A smaller repro that's essentially the same thing, though it results in a different error: https://psalm.dev/r/2e5b073a15

It seems like this is may be an intentional change from #10502?

In my repro:

• Psalm types $a['a'] as true, even though the key 'a' might not exist in the array.
• Due to the changes in the PR, empty(true) now type checks as the type false, where previously it had the less specific type bool.
• Psalm reports this as a redundant condition.

This all makes sense from Psalm's point of view. I can't see an easy way to fix it except to revert that part of #10502 and go back to typing the expression as bool.

The workaround using isset works because isset doesn't have this behavior. It always type checks to bool, no matter what.

You could maybe make the case that isset should be preferred over empty for these types of checks, but it feels wrong to me to report this as a redundant or invalid condition when it's not.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/2e5b073a15

<?php

/** @var array<true> $a */
if (empty($a['a'])) {}

Psalm output (using commit c488d40):

ERROR: DocblockTypeContradiction - 4:5 - Operand of type false is always falsy

[weirdan]

This all makes sense from Psalm's point of view. I can't see an easy way to fix it except to revert that part of #10502 and go back to typing the expression as bool.

Another approach could be to type $a['a'] as null|true when in isset/empty context for direct dimfetch descendants of isset/empty (unless we previously established the key exists). It'll correctly trigger contradiction for empty(array<false>[key]) (false|null is always empty) but won't for empty(array<true>[key]) (true|null can be either empty or not). Kinda the opposite of PossiblyUndefinedStringArrayOffset.

[edsrzf]

That sounds like it could be a good approach. I'll try that.

[mingalevme]

Psalm 5.22.2

https://psalm.dev/r/7bb11edb34

<?php

/**
 * @param list<ArrayIterator> $list
 * @return ArrayIterator
 */
function get(array $list, int $index): ArrayIterator
{
    if (empty($list[$index])) {
        throw new InvalidArgumentException('Index is out of range');
    }

    return $list[$index];
}

Psalm output (using commit [b940c7e](https://github.com/vimeo/psalm/commit/b940c7e)): 

ERROR: [DocblockTypeContradiction](https://psalm.dev/155) - 9:9 - Operand of type false is always falsy

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/7bb11edb34

<?php

/**
 * @param list<ArrayIterator> $list
 * @return ArrayIterator
 */
function get(array $list, int $index): ArrayIterator
{
    if (empty($list[$index])) {
        throw new InvalidArgumentException('Index is out of range');
    }

    return $list[$index];
}

Psalm output (using commit b940c7e):

ERROR: DocblockTypeContradiction - 9:9 - Operand of type false is always falsy

[edsrzf]

I'm playing with a fix using context's inside_isset, as suggested above. I'm running into a lot of flow-on effects from this, though, because the inside_isset flag isn't cleared in lots of contexts. #10752 fixes one specific example of this, but almost every expression type needs to ensure that it sets inside_isset = false before analyzing sub-nodes.

For example, one of the expressions that causes problems in the Psalm code base is:

$source->node_data->getType($call_args[0]->value) ?? Type::getMixed();

The whole left side of the ?? has inside_isset = true. However that flag should be cleared as soon as we descend into the call arguments.

This has repercussions beyond this specific bug since it means that some types of issues inside isset, empty, or on the left side of ?? (probably the most common) could go undetected.

It's a larger issue, but context management overall feels pretty problematic in the Psalm code base. I have to think there'd be many other issues like this.

[ArtemGoutsoul]

Possibly related issues or duplicates:

• Regression with RedundantConditionGivenDocblockType and DocblockTypeContradiction #10715
• Regression: incorrectly reported TypeDoesNotContainType in a loop #10716