Unused global variable warning when global variable is written to

[aeisenberg]

Say I have this function:

function updateGlobal($newVal) {
  global $myGlobal;
  $myGlobal = $newVal;
}

I will get the following message:

Unused global variable $myGlobal.

It seems like this is an erroneous message. Or at least it should be configurable.

Currently, I'm able to get around the warning this way:

function updateGlobal($newVal) {
  global $myGlobal;
  $myGlobal;
  $myGlobal = $newVal;
}

But this is not ideal.

[sirbrillig]

Wow, that is a bug. I actually don't think we handle the global keyword at all; it should count as a variable definition for the purposes of checking a variable's scope. Thanks for finding it!

[aeisenberg]

I'm not entirely familiar with the code, but it looks like if you added a call to markVariableRead on line 590 after the markVariableDeclaration, that should fix the problem. I can submit a PR if you like.

[aeisenberg]

Nope....that's not gonna work.

[sirbrillig]

Yeah, that's looking for the variable declaration. I was wrong, we are checking for the global keyword, so there's something wrong with that. 🤔

[aeisenberg]

Looks like in checkForAssignment, you need to check if the variable being assigned is a global variable and if so also mark it as read.

How do you check if the variable is global?

[aeisenberg]

Or is this something that isn't kept track of yet?

[sirbrillig]

Global variables are a little complicated (see #37) but I am tracking their declaration with the scopeType property of VariableInfo. In this case, that variable declaration is being found and recorded correctly.

Really we just want to see if the variable has been declared within the scope (processScopeCloseForVariable(), which uses getStackPtrIfVariableIsUnused()). I think that getStackPtrIfVariableIsUnused() might be doing something wrong or I'm misunderstanding what that function was supposed to do. Still looking...

[sirbrillig]

Gah, jetlag is clouding my brain. The issue as I understand it is that for some reason the assignment $myGlobal = $newVal isn't being recorded as a use of $myGlobal, although as far as I can tell myGlobal is being saved in a VariableInfo object when it is declared, and its firstInitialized property is being set correctly when it is being assigned. The warning happens at the end of the scope in processScopeCloseForVariable() so something there must not be operating as it should be. Or maybe there's some confusion about what variables should be reported there.

Possibly helpful note to self: getStackPtrIfVariableIsUnused is first defined here: https://github.com/sirbrillig/phpcs-variable-analysis/pull/32/files

There's a test to recreate the issue here: https://github.com/sirbrillig/phpcs-variable-analysis/pull/83/files

Debug mode can be triggered by doing this: ./vendor/bin/phpcs -vvvv --standard=VariableAnalysis VariableAnalysis/Tests/CodeAnalysis/fixtures/FunctionWithGlobalVarFixture.php

[sirbrillig]

Found it. Global imports just need to be treated like pass-by-reference.

[aeisenberg]

Thanks!

On Mon, Apr 22, 2019 at 14:18 Payton Swick ***@***.***> wrote: Closed #81 <#81> via #83 <#83>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#81 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACYYJ4OJBB6HXH2IUVHFC3PRYTUDANCNFSM4HG75NNQ> .

-- Sent while on the go.