Throw a warning when I redefine a global variable in a local scope

[joeyaiello]

Like the title says, it would be useful if I was warned whenever I try to reuse a global variable's name in a more local scope.

$a = 'Unchanged'
function foo {
    Write-Host "foo can read `$a : `$a = $a"
    $a = 'Changed from foo' # this should be warning!!
    Write-Host "foo can change it's own copy of `$a: `$a = $a"
}
foo
Write-Host "After foo call: `$a = $a"

Thanks to @vors for the example.

[KirkMunro]

Can you please clarify exactly what you are trying to accomplish here? Maybe I'm missing something, but I don't think it makes sense to warn on that. If you have specific use cases where this should be a warning, perhaps the proposed rule isn't focused enough on those use cases.

The fact that I can assign a value to a local variable using the same name as a global variable is both useful and necessary to accomplish certain things in PowerShell. Modifying one of the $Preference variables in a specific scope. Setting one of the $Maximum variables in a specific scope to affect the behaviour of something, only in that scope. Assigning a different $ErrorView, but only in a specific scope. Changing the default $OutputEncoding. Or I may write a command that uses a variable name that is the same as one that is global. There is nothing wrong with that in general, unless it were to cause breakage.

What would happen if I wrote a module that used an internal, local variable name that later is added as a global variable, by PowerShell itself or some extension to PowerShell or tool that leverages PowerShell that I happen to be using? Should my module start generating these warnings at that point when it is run through PSScriptAnalyzer? I don't think so.

If there is anything of value to be added here, I think the rule needs to identify explicit variable names that it should not overwrite. But, without looking too hard, it seems that those variables I shouldn't change are read-only or constant already, in which case I don't think there's anything additional that should be done by this tool.

[JennDahm]

It's absolutely normal to define a local variable with the same name as a global variable, but I also imagine it's fairly easy to accidentally create a local variable when intending to use the global variable of the same name, especially if you're new to PowerShell.

On the other hand, I certainly don't remember C, C++, C#, or Java compilers (the languages I have the most experience with) warning me when I redefine a global/object scope variable in a local scope, but I generally avoid doing so anyway.

Perhaps we could label it as "Info," rather than "Warning," as a compromise. I can definitely see the value of this rule, particularly for new users. If you break it regularly, intentionally, you can always disable the rule.

[KirkMunro]

How about narrowing down the use case then, by never generating a warning or informational message for any built-in global variables? Those variables that should be read-only or constant are, and changes should not be discouraged for the rest because it is useful to do so. Or if you don't want to eliminate all built-in global variables, then at least ignore the most common ones that are designed with scoped use in mind (Preference variables, Maximum variables, OutputEncoding, ErrorView, etc.

Regardless of whether or not you take that approach, this proposal still doesn't feel right to me as is. I understand what you are trying to help users avoid though, but in my mind it needs to be broken out into two different use cases:

1. Assignment of a value to a local variable with the same name as a global variable when that variable is not used for anything else in that function. I think the second part of that use case is most important, because if the variable is being used in that function (named or unnamed), in the current scope or in multiple child scopes, then the assignment should be intepreted by the linting tool as a variable declaration with initialization. In this use case, anything more than an informational message would be too much. A warning would be a bad thing for this scenario in my opinion.
2. Modification of a global variable in a function where that modification will not work as expected. For example:

$x = 2
$y = 4
function doubleX {
    $x *= 2
}
doubleX
$x
function addY {
    $x = $x + $y
}
addY
$x

Of course any of the arithmetic assignment operators or the increment/decrement operators would fit into this category. PSScriptAnalyzer should also check for replacement with modification (like the addY function shows), where an assignment is made that includes an operation of any kind (-shl, -shr, -bnot, or any other arithmetic or bitwise operator), and when that operation includes the variable that is being reassigned, where that variable is not a parameter to the function/script block, and where the function or script block were not dot-sourced. The above code runs, and even strict mode does not result in an issue (although it definitely should, and I consider that a bug in strict mode). I think this is the more common mistake you want to avoid that someone might make, and this should absolutely be a warning.

[rkeithhill]

Unfortunately those built-in variables are often not marked as read-only / constant e.g.:

function foo { $args = 1..3; "`$args are $args" }
foo a b c

Ouptuts:

$args are 1 2 3

I would like to see this rule split in two. First rule: for variables defined by PowerShell that should have been created as read-only / constant, I would like to see a warning that I'm about to write-over a built-in variable. Two obvious candidates that come to mind are $args and $input but I would include $_ and $PSItem as well.

The second rule would be for user-defined global, script and module scope variables. If there is a function assigning to a variable with the same name but isn't scope qualified I would emit either an info or warning stating that if the intent was to modify the higher-level scoped variable that they need to use a scope qualifier. Generally this is an indication of an issue - i.e. the user doesn't know about PowerShell's copy-on-write behavior.

Regarding C#, it will not allow you to re-declare the same name in a child scope. While I'm sensitive to folks reusing variable names like Path, Filename, i, j, ndx, etc I think it unlikely that these variables would be defined by the user at module, script or global scope. Parameters on the other hand, yeah I've been known to overwrite a parameter's value.

[Jaykul]

So basically, we have this feature of the language that lets me inherit variables from outer scopes ... and you want to warn me every time I take advantage of it?

[rkeithhill]

@Jaykul Uh no, please do take advantage of it. That's what it's there for. ;-) The "potential" issue is when assigning to (not reading from) a variable in a nested scope with the same name as a global/script scope but not using a scope qualifier.

If I see the following in a script, it is usually indicative of a problem. And yeah, I have helped folks with this issue more times than I can count:

$LogPath = 'c:\temp\foo.log'
function InitLogging {
    $LogPath = '\\server\share\foo.log'  # User probably needs to change to $script:LogPath = ...
}

That said, I don't feel as strongly about this second scenario as I do about assigning to PowerShell built-ins that should have been made read-only.

[JamesWTruher]

@Jaykul @rkeithhill @KirkMunro @JennDahm - this is a great discussion, but I'm not sure where we should go from here. Should we track this and build a rule, or is it not important enough and we should close this as "no thanks", just let it ride, or split it into two (built-ins vs assignment in child scope)

FWIW, I'm slowly reviewing all of the open issues in this repo, so I'm going to have a bunch of similar questions as I go.

[rkeithhill]

I'd like to see a rule that fires when folks assign to built-in variables like $args, $input, $_ and $PSItem. For the other use cases (users overwriting their own variables), I think no rule is necessary.

[JamesWTruher]

thanks for the response - I've created #858

[vors]

Hey guys, sorry for chiming in so late too. Wow, was few years ago, so I may not recall correctly, but here is my 2 cents

@KirkMunro wrote

Can you please clarify exactly what you are trying to accomplish here? Maybe I'm missing something, but I don't think it makes sense to warn on that. If you have specific use cases where this should be a warning, perhaps the proposed rule isn't focused enough on those use cases.

The specific case that would be good to avoid is essentially what's posted in the description.

$a = 1
function foo {
   $a = 2
}

foo  # $a didn't change and this is very counterintuitive!!

I think you absolutely correctly argue that this is how language designed. My point here is that this is very very steep learning curve and I'm sure a lot of people struggled to figure that out that behavior, when they learned powershell.

If we accept that rationality, there is whole other question of "should PSScriptAnalyzer be a learning tool for people". I think that it should, similarly like pylint is an excellent tool to learn about python quirks.

[Jaykul]

If you're going to start warning people when they're doing things that may not act quite the way they expect, that's a very long list, which depends very much upon what their previous experience is.

But in theory, I don't mind a set of rules like that, if they're in a set, and if they're low-importance (i.e. "info" vs. "warning")...

But it's going to be noisy -- and it's going to be hard to write a rule that catches every case it should catch, without warning improperly. Even without worrying about tracking all the variables across all the scopes, how do you handle this?

$user = @{ id = "Jaykul" }

function Set-Name {
    param($user, $name)
    $user += @{ Name = $Name }
}

Set-Name $user "Joel"

[ili101]

I would like to +1 this. A low-importance alert or something you can turn on or off.
The commune scenario for me is:

1. You write a flat script.
2. You refactor it into functions (and miss adding some variables to param but use them from the parent scope by mistake).
3. You get unexpected results and/or you move a function or use it in a different place and it mysteriously stops working.
4. You find out that you were using parameters out of the function scope by mistake since the refactor.