scope issue

[yonderblue]

So i think the sniff should consider the "first" time it sees the variable (to store the scope) as whenever it sees the variable assigned.

[guywithnose]

I'll have to think on this. It's not simple to see if the variable is being assigned. Although this does make more sense.

[yonderblue]

Can we not check the token to the left after whitespace is an = operator?

[yonderblue]

Just like chad does in his recent pull with the static scope operator.

[guywithnose]

That's the simple case, and if we're ok with just that then no problem. Two, more complicated cases, I'm thinking of are pass by reference and
$foo[$bar] = 3 does not initialize $bar

[yonderblue]

true those are two valid cases but i think having it in there with at least the main case would be very useful and would catch a good bit. we can improve later..

[nubs]

Can we get a concrete test case that we want for this?

[nubs]

Still wanting an exact example of the bug that we want fixed here. A very simple php snippet that the code sniffer gives a warning about that it shouldn't.

[yonderblue]

try {
} catch (Exception $e) {
}

try {
} catch (Exception $e) {
}

will complain about $e whereas it should only in cases like

if () {
    $boo = 4;
}

$boo = 5;

[nubs]

Your first example does not generate any warnings in the current standard.

[yonderblue]

it did before, did you change the way the sniff worked when you revamped this stuff?

[nubs]

Nope. Try adding something to the end of tests/DWS/Sniffs/Scope/VariableScopeSniffTest.inc that will make the tests fail incorrectly and let me know if you can find something.

[yonderblue]

k i will when i get a chance

[nubs]

no hurry :)

[yonderblue]

seems that

echo $do;
try {
    $do = 'something';
} catch(Exception $e) {
    var_dump($e);
}
echo $do;

passes when it should fail the same as

try {
    $do = 'something';
} catch(Exception $e) {
    var_dump($e);
}
echo $do;

since the first echo wasn't an assignment.

[yonderblue]

also, all the cases where its just

function f()
{
    if ($boo == 1) { }
}

should probably fail since boo is being read before it is assigned anywhere in the real php scope.

[nubs]

Okay, so the main issue is that we want to ensure that a variable is "assigned" somewhere on a statement before where it is "read". Is that correct?

Cases that come off the top of my head:

1

Does this pass our standard? In my opinion, it should not because it is immediately reading from the result of the assignment rather than separating those 2 tasks.

if ($foo = doSomething()) {
    var_dump($foo);
}

2

Does this pass our standard? I think it does, because they are in seperate statements.

for ($i = 0; $i < 5; $i++){
}

3

Does this pass our standard? I would guess not, because it's expecting $foo to already have a value:

$foo .= 'bar';

4

Does this pass our standard? I would guess not, mainly because it's part of a boolean expression and therefore its value is being read from on the same statement.

isset($foo) || $foo = 5;

5

This should definitely pass our standard, but helps exhibit the difficulty of this problem:

$foo = 1;
$bar = function() use($foo) {
    echo $foo;
};

6

How about this, does it pass our standard? I don't know what to think about this one. Notice the last of break's.

$foo = getData();
switch ($foo) {
    case 'bar':
        $baz = 1;
    case 'bar2':
        if (!isset($baz)) {
            $baz = 2;
        }
    default:
        if (isset($baz)) {
            echo "Baz! {$baz}";
        }
}

7

Here's another example to exhibit the difficulty of this:

list($foo, $bar) = array(1, 2);
if ($foo == 1) {
    echo 'yay';
}

8

Here's another example that shows the difficulty of this:

$type = getType();
TOL_Util::ensureTrue(in_array($type, ['foo', 'bar']));
$$type = "Hello";
if (isset($foo)) {
    echo "{$foo}, Foo!";
} else {
    echo "{$bar}, Bar!";
}

9

Or this:

$foo = ['bar' => 1, 'baz' => 2];
extract($foo);
echo $bar;

I'm sure there are many more, but these are some that come to mind. Comments?

[yonderblue]

1 - 4 I agree.

5 & 7. I agree and will be a special case.
6. Since there are paths through that dont have the var assigned before its used it should fail. of course difficult.
8 & 9. These are so dynamic and if are too difficult its better to leave off the sniff.

So in summary I think it is clear what should fail and pass, and the ones that are too difficult we let through. At least the ones that are coded for though will be errors and correct.

[nubs]

6 does not have paths through it that assume the var is assigned. All read accesses to it are wrapped inside an isset, which brings up this:

10

I would say this passes coding standard.

if (isset($foo)) {
    echo 'Hi';
}

if (!empty($bar)) {
    echo 'Howdy';
}

[nubs]

As for your comments about 8 and 9, what should the sniff do when it hits that code? It seems to me that it would throw errors. Or do we not do the scoping sniff inside of a function that has a variable variable or the extract method?

[yonderblue]

In the ones that are too hard it doesnt really matter where we get out of dodge as long as it doesnt cause false positives. Sorry about number 6 I didn't look close enough.
We can make special concessions to the functions like isset that we know are safe.

[nubs]

What do you mean by false positive? falsely throwing an error on it?

[yonderblue]

yup

[nubs]

That's super tricky then. It means that when looking at a variable, we have to look backwards through the scope for any variable variables being assigned to. If there are any, then we don't throw an error. Possible, but tricky. Just letting it throw an error and requiring that someone does:

$type = getType();
$foo = null;
$bar = null;
$$type = 'hello';

would be much easier.

[nubs]

11

I know we currently have something about this up for vote, but the strict definition I gave above would mean this is not allowed because it is reading from $bar where it is assigning to it.

$foo = $bar = '5';

[yonderblue]

For $$ thing ya as long as there is a way to get it past the error then thats fine, its the cases where its a false positive and there is no way to fix it that puts a big wrench in stuff.

for 11 I wouldn't think the language reads from $bar until after it sets it. So $bar = '5', and then $foo = $bar. So it doesnt violate anything.

[nubs]

so what about:

12

This is just like the multiple assignment in 11, but it does an operation rather than a straight assignment. php handles it without problem, setting the variables as expected.

$foo = 'Hello, ' . $bar = 'Bar';

Basically, if we allow 11, it would be as a special case due to its simplicity, because it is still doing a write and a read on $bar in one statement. For other cases that you thought should fail the standard but are the same basic situation, see 1 and 4.

[yonderblue]

multiple assignment is pretty standard, but despite the fact that this is doing a write and then a read, so not violating the scoping concepts it still looks like we are assigning a var within an expression which violates our standard in general.

[nubs]

So what are you saying? Can you reiterate whether you think examples 1, 4, 11, and 12 should pass or fail the standard?

[yonderblue]

1 fail
4 fail
11 pass
12 fail

[nubs]

Okay, so you are saying we should have 11 be a special case. There's going to be some funky coding going on in here.

[yonderblue]

Ideally I would say yes its a special case, but like I said before if its too difficult, a sniff that covers half of what we want reliably giving correct errors and letting through the things that are too hard is better than trying to do it all with false positives. :)

[nubs]

Without handling the special case of 11, it would throw errors on multiple assignments in a line. Take a look at https://github.com/nubs/dws-coding-standard/compare/scope-fix?expand=1 for my current plans on how to fix this sniff.

[yonderblue]

the doc in that comparison seems pretty logical to me