Better support for compact

[bugreportuser]

Psalm gives an error when compact() is used on an undefined variable. However, it still infers the type as array<array-key, mixed>.

Example (https://psalm.dev/r/bfab55228d):

<?php
$x = 'str';
$y = 0;
$example = compact('x', 'y');
echo ord($example['x']);
echo chr($example['y']);

Example output:

Psalm output (using commit 1f0aca0):

INFO: MixedArgument - 5:10 - Argument 1 of ord cannot be mixed, expecting string

INFO: MixedArgument - 6:10 - Argument 1 of chr cannot be mixed, expecting int

It should be able to treat that example as:

<?php
$x = 'str';
$y = 0;
$example = ['x' => $x, 'y' => $y];
echo ord($example['x']);
echo chr($example['y']);

[muglug]

Do people use compact? I'd rather not support this IMO bad function.

[bugreportuser]

It's debatable. I think extract is actually the problem, but they're usually grouped together.

compact is basically syntactic sugar. A parser can transform compact('x', 'y') into ['x' => $x, 'y' => $y] as easily as $x ?? 10 into isset($x) ? $x : 10. If you pass variable strings to compact, that's when it becomes a bad function.

[muglug]

yeah, but it's not particularly sugary because it comes in the form of a function that takes a bunch of strings.

[bugreportuser]

That's pretty common in PHP:

constant('constant');
array_map('function', []);
get_parent_class('class');

[muglug]

🙈

[still-dreaming-1]

compact is a decent way of removing duplication and possible typos from your code if you want to build an array of names of variables and their values. To build such an array manually could result in misspelling the variable name in the string, or renaming the variable name at some point without updating the string name. If you use compact, it is at least possible for a static analysis tool to let you know when you are referencing a non-existent variable, and phpmd actually does this. I do wish PHP allowed you to just pass the actual variables to compact though, instead of the variable names, and that it would extract the name from the variable somehow. Then Psalm would already know if you are referencing a non-existent variable.

Psalm would have slightly better support for compact if it at least understood the returned array key type was a string instead of array-key, although that would not exactly fix the issues of the OP.

[still-dreaming-1]

Actually I was interested in making that change myself (improving key type), but I couldn't find anywhere in Psalm's code where return type of compact was specified. It seemed likely to me this would be hard coded somewhere, but I couldn't find it. I did look through the contributions guide, but I still couldn't figure it out.. There is another similar contribution I would like to attempt as well.

[bugreportuser]

@still-dreaming-1 It's in the callmap.

'compact' => ['array', 'var_name'=>'string|array', '...var_names='=>'string|array'],

The first element is the return type, which can be replaced with array<string,mixed>.

[muglug]

This is fixed

[bugreportuser]

Thanks!