sirbrillig / phpcs-variable-analysis

Find undefined and unused variables with the PHP Codesniffer static analysis tool.
Other
136 stars 14 forks source link

List assignment next to array causes unused variable warning #317

Closed djibarian closed 7 months ago

djibarian commented 7 months ago

This bit is throwing a VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable false positive for $notificationId. It seems it has to do with array destructuring the result, because if changed to $bar it disappears.

static function foo(DBC $db, int $notificationId): void {
    [$bar] = $db->queryGetRowIndexed("
        SELECT blah...
    ", [
        "n.notificationId" => $notificationId
    ]);
}

And this one too, in $level, although in this case it looks related to that "self reference assignment":

static function dotToMultiArray(
    array $array
): array {
    $multi = [];
    foreach($array as $key => $value){
        $level = &$multi;
        foreach(explode(".", $key) as $node)
            $level = &$level[$node];
        $level = $value;
    }
    return $multi;
}
sirbrillig commented 7 months ago

Thanks for the report!

It seems it has to do with array destructuring the result, because if changed to $bar it disappears.

I think you're right because this passes:

static function foo(DBC $db, int $notificationId): void {
    list($bar) = $db->queryGetRowIndexed('SELECT blah... ', [ 'n.notificationId' => $notificationId ]);
    echo $bar;
}

Whereas this fails:

static function foo(DBC $db, int $notificationId): void {
    [$bar] = $db->queryGetRowIndexed('SELECT blah... ', [ 'n.notificationId' => $notificationId ]);
    echo $bar;
}

Even though both are destructuring. What's really weird is that the warning is Unused function parameter $notificationId. which has nothing to do with $bar.

Here's a more minimal case with the same bug:

function foo(int $baz) {
    [$bar] = doSomething([$baz]);
    return $bar;
}

Investigating...

sirbrillig commented 7 months ago

https://github.com/sirbrillig/phpcs-variable-analysis/pull/318 should fix the issue you found, although you'll still get an unused variable warning in your first example unless you do something with $bar like print it or return it since otherwise $bar is not used.

As for the second issue,

function dotToMultiArray( array $array ): array {
    $multi = [];
    foreach($array as $key => $value){
        $level = &$multi;                // $level is assigned
        foreach(explode(".", $key) as $node)
            $level = &$level[$node]; // $level is assigned again
        $level = $value;                 // $level is assigned a third time
    }
    return $multi;                           // We ignore $level and return $multi
}

In this case, $level is marked as unused because it is. It's only ever assigned a value but that value is never used for anything. Maybe I'm misunderstanding something?

I think that example is equivalent to:

function dotToMultiArray( array $array ): array {
    $multi = [];
    return $multi;
}
djibarian commented 7 months ago

...although you'll still get an unused variable warning in your first example unless you do something with $bar like print it or return it since otherwise $bar is not used.

Yes, the function wasn't complete and $bar is used afterwards. Thanks for the quick fix!

Re the second case, $level is indeed used, look carefully. It is used as a pointer to traverse a multi-dimensional array (first and second) and set the value of one of its nodes (third). Probably the assignments by reference are confusing the linter.

sirbrillig commented 7 months ago

Interesting... Does that actually work to run $level = $value and assign to a sub element of $multi? I thought that assigning directly to the variable would overwrite the reference itself? Is it because the reference is pointing to an array element and not the array?

sirbrillig commented 7 months ago

Oh! It does. Wild. I made https://github.com/sirbrillig/phpcs-variable-analysis/issues/319 to investigate that one.