False positives

[djibarian]

After upgrading from v2.9.0 to v2.10.2 I got a lot of false positives and I had to downgrade again. For example:

$var = NULL;

function foo(){
    global $var;
    // var used here
}

causes a VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable.

Also

if(($val = func()) !== FALSE){
    $val->foo();
}

causes a VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable.

[sirbrillig]

Thanks for the reports! Let's take them one at a time.

causes a VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable.

This is a tricky one. The issue is that the only way we can know the file-scoped $var = NULL; is used is because there's a global $var; statement in one of the functions. Now we could certainly determine that from scanning for global statements within the current file, but what if the function was in a separate file? In that case, there's no way to tell that the variable is actually used. Dealing with sharing scope at the global level is really better suited for a static analysis tool.

The reason you saw this report as a regression was probably because 2.10 has much better scope detection, and finally supports the file-level scope (as of #190). If you're going to be using a lot of global variables, you might want to enable the allowUndefinedVariablesInFileScope option so those will be ignored (at least, I think they will... I wonder if we need another option to allow unused variables in file scope too).

causes a VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable.

That does sound like a bug, but I've so far been unable to reproduce it. I created a file called test.php that looks like this:

<?php
if(($val = func()) !== FALSE){
    $val->foo();
}

Then I ran phpcs --standard=VariableAnalysis test.php and I got no results. Can you help me figure out what I'm doing wrong?

[sirbrillig]

If you're going to be using a lot of global variables, you might want to enable the allowUndefinedVariablesInFileScope option so those will be ignored (at least, I think they will... I wonder if we need another option to allow unused variables in file scope too).

I realized that there's no complete way to disable scope detection for the global scope. allowUndefinedVariablesInFileScope only applies to undefined variables. Therefore I added a parallel allowUnusedVariablesInFileScope option in #229 which should help with that.

[djibarian]

The exact code is

if(($q = @simplexml_load_string("<?xml version=\"1.0\" encoding=\"UTF-8\"?><t>" . $s . "</t>")) === FALSE || !__cst_check($q, $a, $b))
    $s = NULL;
else
    $s = mb_substr($s = $q->asXML(), $i = mb_strpos($s, "<t>") + 3, mb_strrpos($s, "</t>") - $i);

throwing a Variable $q is undefined..

A bit convoluted I know, it’s old code.

Maybe you can recreate the problem with that one.

[djibarian]

Another example:

$targets = [];
$p = &$targets[];
if($p !== FALSE){
    ...
}

throws an Unused variable $p..

[djibarian]

And another one:

if($c = count($pinsData))
    $p = array();
if($c){
    ...
}

throws Variable $c is undefined..

[sirbrillig]

Thank you for the examples!

With the first one I was able to reproduce the error where $q is marked as undefined. Here's a simpler example:

if($q = getData()) 
    echo $q;
else
    echo $q->asXML(); // $q is reported as undefined

That doesn't happen with curly braces, though:

if($q = getData()) {
    echo $q;
} else {
    echo $q->asXML(); // no warnings
}

So this is a bug that has something to do with scope detection and inline conditional blocks.

I tried the other examples you gave, however, and I was not able to get the same results. Maybe the actual warning was coming from some use that's part of the excluded code? In any case, let me try to fix the regression above and we'll see if it solves the other reports you're seeing.

[sirbrillig]

I found it. Could you give v2.10.3-beta.1 a try to see if it resolves your issues?

https://github.com/sirbrillig/phpcs-variable-analysis/releases/tag/v2.10.3-beta.1

[sirbrillig]

Since I haven't heard anything, I'll assume this issue is resolved. Please let me know if not!