Weird property purity (?) issue

[danog]

When using a property that is a union, array_key_exists assertions aren't parsed: https://psalm.dev/r/ceab5bdfcd
However, if that same property is first copied into a variable, the assertions are parsed correctly: https://psalm.dev/r/8e55452ecd

This somewhat reminds me of a purity issue, but all non-magic properties should be pure by default.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ceab5bdfcd

<?php

class a {
    /** @var self::STATE_* */
    public int $status = self::STATE_A;
    public const STATE_A = 0;
    public const STATE_B = 1;
    public const STATE_C = 2;
}

$a = new a;

/** @var array<string> */
$arr = [];

if (array_key_exists($a->status, $arr)) {
    echo $arr[$a->status];
}

Psalm output (using commit cb976f8):

INFO: PossiblyUndefinedIntArrayOffset - 17:10 - Possibly undefined array offset '0|1|2' is risky given expected type 'array-key'. Consider using isset beforehand.

https://psalm.dev/r/8e55452ecd

<?php

class a {
    /** @var self::STATE_* */
    public int $status = self::STATE_A;
    public const STATE_A = 0;
    public const STATE_B = 1;
    public const STATE_C = 2;
}

$a = new a;

/** @var array<string> */
$arr = [];

$status = $a->status;
if (array_key_exists($status, $arr)) {
    echo $arr[$status];
}

Psalm output (using commit cb976f8):

No issues!

[orklah]

It doesn't seem related to purity. I think it might be related with this line:

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php

Line 3618 in 5b82082

&& !$expr->getArgs()[0]->value instanceof PhpParser\Node\Expr\ClassConstFetch

I'm not sure why but ClassConst seem to be excluded from array_key_exists assertions

[hirokinoue]

@orklah
Given the sample code of the issue, adding || $expr->getArgs()[0]->value instanceof PhpParser\Node\Expr\PropertyFetch and StaticPropertyFetch after Line 3701 results in pushing [$arr[0] => [['array-key-exists']] ($arr[1], $arr[2] as well) into $if_types, which seems to be reflected in context->vars_in_scope.

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php

Line 3701 in 5b82082

} elseif ($expr->getArgs()[0]->value instanceof PhpParser\Node\Expr\Variable

As the result, the issue is solved. I guess context->vars_in_scope will be more aqurate by the fix.
There might be some side effects but only for analysis of array_key_exists(). I'd like some advice such as:

• I haven't noticed negative side effects yet, is there anything I should look into?
• I think ArrayKeyExistsTest should be added some test cases. Do you have any idea about the other tests that should be fixed?

If you are alright I'll create a PR.
Any advice would be appreciated. Thanks in advance.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ceab5bdfcd

<?php

class a {
    /** @var self::STATE_* */
    public int $status = self::STATE_A;
    public const STATE_A = 0;
    public const STATE_B = 1;
    public const STATE_C = 2;
}

$a = new a;

/** @var array<string> */
$arr = [];

if (array_key_exists($a->status, $arr)) {
    echo $arr[$a->status];
}

Psalm output (using commit afe85fa):

INFO: PossiblyUndefinedIntArrayOffset - 17:10 - Possibly undefined array offset '0|1|2' is risky given expected type 'array-key'. Consider using isset beforehand.

[orklah]

Hey @hirokinoue ! Sorry for the delay of my reply. You're of course welcome to propose a PR, there's a good chance that the test suite will fail at some point if any side effect were to appear so go ahead!