Powershell: Set-StrictMode should detect use of automatic variables as parameters or the target of an assignment

[briantist]

It's common for people to inadvertently to use an automatic variable as a parameter name or a variable name, and then get confused when it doesn't contain the value they expect.

This is just one example but it comes up fairly often, and unless you already know the name is reserved, PowerShell doesn't offer you any clues as to what's going on.

Because the various automatic variables get overwritten based on different conditions, it can be even more confusing when it seems to work in one context but not another.

Given Set-StrictMode's other roles, this seems like a good fit.

One intentional use is for nullification, as in:

$null = Invoke-CommandWithUnwantedOutput

So that could be an exception (because it's unlikely to be done unintentionally), but my thoughts are that there are other ways to achieve this, and strict mode is not enabled by default, so in my opinion it should not be an exception.

[iSazonov]

It is rather the PSScriptAnalizer area.

[lzybkr]

I would be open to discussing this as a breaking change - assuming we can get the data to justify that the impact is minimal.

[briantist]

@lzybkr how would this be a breaking change? An update to Set-StrictMode would likely just mean supporting the next version number. Right now only 1 and 2 are defined; in theory we'd set 6 (or whatever version of PowerShell it ends up being), or 3 if version is not supposed to correspond to PS version.

Or are you suggesting making this behavior default, without strict mode?

[lzybkr]

Right - parser error - not tied to Set-StrictMode.

[briantist]

@lzybkr ah ok got it.

It looks like at least some of them are already not writable. I tried to set $Host and it gave me this:

Cannot overwrite variable Host because it is read-only or constant.

Presumably, variables that do change during the course of the script like $Input and $args can't be set that way unless the internal code handles undoing that or removing and recreating the variable?

And of course $OFS has to remain writable.

[mklement0]

The same issue just came up in #3695, with respect to $_ / $PSItem being writable.

Here's a list of all automatic variables that _shouldn't_ be writable, but currently are:

Name                          Predefined
----                          ----------
_                                  False
AllNodes                           False
Args                                True
Event                              False
EventArgs                          False
EventSubscriber                    False
ForEach                             True
Input                               True
Matches                             True
MyInvocation                        True
NestedPromptLevel                   True
Profile                             True
PSBoundParameters                   True
PsCmdlet                           False
PSCommandPath                       True
PSDebugContext                     False
PSItem                             False
PSScriptRoot                        True
PSSenderInfo                       False
Pwd                                 True
Sender                             False
StackTrace                          True
This                               False

"Predefined" refers to whether they exist in the global scope.

Some of them are only defined in certain contexts, but, arguably, to prevent confusion, preventing their custom use categorically is the right approach.

The following code was used to detect them (note that it has to rely on parsing the _help topic_ to identify all automatic variables - I'm not aware of a better way; the obsolete ReportErrorShow* variables were manually removed after the fact
):

$autoVarNames = ((get-help about_automatic_variables) -split [environment]::newline -match '^\s*\$\w+\s*$').Trim().Trim('$') | Sort-Object -Unique

foreach ($varName in $autoVarNames) {
  $var = Get-Variable $varName -ErrorAction 'SilentlyContinue'
  $exists = $?
  if ($exists -and $var.Options -match 'readonly|constant') {
    Write-Verbose "$varName`: read-only or constant"
  } elseif ($varName -in 'NULL', 'OFS', 'LastExitCode') { # exceptions
    Write-Verbose "$varName`: writable by design"
  } else {
    Set-Variable -Name $varName -Value $null -ErrorAction SilentlyContinue
    if ($?) {
      [pscustomobject] @{ Name = $varName; Predefined = $exists }
    }
  }
}

Note that the code has a fixed list of exceptions so as not to report automatic variables that should indeed be writable, such as $OFS, and $LastExitCode, or assignable as a quiet no-op, such as $NULL - do tell us if other exceptions are missing.

[briantist]

Excellent work @mklement0 !

[iSazonov]

Related comment https://github.com/PowerShell/PowerShell/issues/2000#issuecomment-241473824

[lzybkr]

We try to use this file to make it easier to know what variables PowerShell cares about, automatic or otherwise.

I'll also point out the need to be careful here, e.g. disallowing anyone to set $LastExitCode would break posh-git which saves and restores the value in the prompt function so that it can call git and not clobber the real $LastExitCode from your last command.

[mklement0]

@lzybkr:

Thanks for the link, that's handy. I've noticed that not all variables listed in the automatic-variables help topic are defined in that file, however, such as AllNodes, Event, EventSubscriber

And thanks for the correction re $LastExitCode - I've removed it from the list above and adapted the code that generates it (it has a hard-coded list of exceptions).

The list definitely needs careful reviewing - do let me know if there are others that should be excluded.

I think it's also important going forward to think about the proper read-only status at the time of introducing an automatic variable.

Longer-term, it would be helpful if automatic and preference variables could be discovered through _reflection_, in user code.
At least for preference variables this has been suggested before on uservoice.com

[briantist]

Definitely seconding (and voted on) the suggestion for discoverability.

[SteveL-MSFT]

@PowerShell/powershell-committee recommends we have psscriptanalyzer rules for all the automatic variables with enforcement in parser for $_/$psitem and $input

[mklement0]

I'm glad to hear that writing to $_/ $psitem and $input will be prevented, @SteveL-MSFT.

Given that we know that this will be a _breaking_ change, it seems to me that this is an opportunity to get it _fundamentally_ right, by making _all_ automatic variables that are _conceptually_ read-only _actually_ read-only - without having to rely on a code-analysis tool (PSScriptAnalyzer) that may or may not be used by users.

Similarly, PSScriptAnalyzer can't protect you from trying things like $PWD = '/' from the command line.

This ties in with making automatic variables discoverable as such.

[SteveL-MSFT]

We discussed possibly making all automatic variables functionally read only, however, agreed that in many cases like $PWD someone could be using it already without problem as they didn't Set-Location so $PWD isn't overwritten. So other than $_, $PSItem, and $Input, we decided to error on the side of limiting the breaking change to the most common cases.

[mklement0]

See this documentation issue for the necessary attendant change to the documentation.

[mklement0]

Now that I think about it: We've lost sight of @briantist's original idea: that a _new_ Set-StrictMode version level could solve the problem _comprehensively_ at the _engine_ level, without breaking backward compatibility.

[iSazonov]

If we can avoid this, we'd better avoid it.

[nightroman]

It is very good that writing to $input will be prevented. Such a code did not work anyway and users only discovered this on troubleshooting. This fixes the problem and does not break anything.

But I afraid preventing writing to $_ breaks things and make coding harder. See these two examples:

• https://github.com/PowerShell/PowerShell/issues/3695#issuecomment-315672717
• https://github.com/PowerShell/PowerShell/issues/3695#issuecomment-320151872

[iSazonov]

I believe that we want just exclude such practise and recommend use custom variables - it makes scritps more clear and reliable.

[nightroman]

I am not sure about more clear for $_. Using $_ as a custom variable in proper contexts may be more clear and more reusable than something else. It is well known as "the current target object" variable, either in pipelines/loops or in custom expressions defined as script blocks.

[mklement0]

@iSazonov:

If we can avoid this, we'd better avoid it.

It seems to me that a Set-StrictMode -Version 6 would generally be - not just in this case - a great opportunity not to burden users with historical warts that are only retained to avoid breaking old code.

[mklement0]

@nightroman:

In the case of cmdlets, an automatically defined $_ is a _necessity_, because the pipeline object being processed has _no other name_.
The same applies analogously to $_ in switch statements (with array-valued input) and the catch handlers of try / catch statements.

In your example, there is no need for an implicit variable: you can simply pass argument(s) to the script-block invocation, and either define parameters inside the script block or use $Args to reference them:

$foo = ...  # value to pass to the script block below
if (& { ... ) $foo) {  # inside the script block, use $Args or define parameters with param(...) 
        ...
}

Similarly, in a foreach loop, you _must_ specify an _explicit_ iteration variable that names each item being enumerated; choosing a variable name that the engine _implicitly_ defines in other contexts is problematic.

That said, in the case of a _loop_ with _item-by-item_ processing, there is a stronger case for the $_ paradigm, but as I've suggested, the right way to resolve this is to make $_ _implicit_ as well, by allowing a foreach syntax variation without an explicit iteration variable.

[nightroman]

@mklement0 Thank you. This is not what I worry about. Here are my concerns:

• Read only $_ will break existing code for sure.
• Replacement expressions with arguments and parameters will work but they will not be universal, e.g. I will not be able to use them as is in my code and in built-in cmdlets that require expressions with $_.
• On moving code between ForEach-Object pipelines and foreach loops I will have to rename variables.

[iSazonov]

@mklement0 The main drawback of Set-StrictMode is that it is realtime. Before we dicussed that we could be parse-time with using. That's why I think we should avoid the cmdlet.

[mklement0]

@iSazonov: Good point about Set-StrictMode - I assume you're referring to @lzybkr's RFC for a _lexical_ strict mode with something like using strict.

If we had something like that, I think it would be a good general way to get rid of historical warts on an opt-in basis while not breaking old code.

[briantist]

@mklement0 I haven't followed this in a while, but as I think about it more I do actually think that $_ should remain writable.

One reason for this is that it's common to modify the object in $_, using Add-Member for instance. Because the object is probably a reference, it could be that this would still work, if the only thing being prevented is overwriting the variable itself (with a value type or a new reference).

But the other use I have in mind is, I think, legitimate.

If you want to have a function that takes a scriptblock parameter and then executes it against a series of items within your function, you have only a few choices:

1. Don't support $_. This sucks for the caller. In PowerShell, you're used to writing such scriptblocks where $_ represents the current item. Not being able to do also means you can't easily store and re-use such a scriptblock. So this ends up with "custom" code being non-idiomatic.
2. Set the variable in your function before executing the scriptblock (that is do something like $_ = $CurrentItem ; & $UserSuppliedScriptBlock). This wouldn't work anymore.
3. "Fake it" with ForEach-Object. I've done this before, mostly for prototyping, but it feels wrong. Basically, in your own function, you do something like this: $CurrentItem | % -Process $UserSuppliedScriptBlock. This does work... until $CurrentItem is an array and needs to be treated as such. There are other situations I don't remember offhand where this is problematic. I'd prefer that we didn't have to resort to this to support the same workflow that lots of other things in PowerShell do.

[mklement0]

@briantist: You should read the thread at #3695 - it's been decided to leave $_ writable, mostly so as not to break existing code.

That said, the solution to passing a collection to ForEach-Object as-is is to use -InputObject rather than the pipeline:

> ForEach-Object { $_.GetType().Name } -InputObject 1, 2
Object[]

[bergmeister]

@lzybkr I am currently implemting the rule in PSSA to warn against assignment of automatic variables, could we consider making the SpecialVariables class public to avoid having a copy of those magic strings in PSSA or is there a better way to determine whether a variable is automatic?

[lzybkr]

It's a good idea, but it would take some work to make it useful.

For your use case - it's insufficient because some of those variables are, by design, assignable.

In other cases, there is insufficient metadata to say why the variable is special. Many variables are read but not normally assigned by PowerShell, some are assigned by PowerShell in very specific (and uncommon) circumstances, etc.

Some historical background - that file didn't exist until V3. Before that, there was no good way to find all references to variables that the PowerShell engine cared about in some way. It's also probably missing some that I missed or were added when I wasn't looking.

[bergmeister]

@lzybkr I was also thinking about splitting the warnings into 2 categories:

• definitely avoid/cannot assign for variables such as e.g. $false where PowerShell itself would throw an error -> severity error
• re-consider assignment for variables such as $PSItem -> severity warning

Thanks for the info. Version should be more than enough since v2 is already deprecated and PSSA requires at least v3 as far as I know. For starters, I could start at least with the official list here, which should be the most useful anyway. Some of them like $IsWindows cannot be put into it yet since PSSA is not aware of PowerShell Core yet as far as I know.

[lzybkr]

That list is good, but review it closely. After my quick scan, I think warning on $sender would be very noisy and I think the doc is outdated regarding the $REPORTERRORSHOW* variables - those are not defined anymore, I don't recall when they were removed though.