Psscriptanalyzer: New Rule Suggestion: AvoidReuseOfAutomaticVariables

[Glober777]

I think it would be really helpful, if there was a rule that would point out when there's attempt to reuse (assign something to) an automatic variable, as sometimes, those may get reused by mistake, like $Host, $Home, $Args, $Input, etc. While in some cases the engine will throw out an error during the execution (i.e. $Host = 'Hello!'), there are occasions when no errors will show up, like $Args = 'Hello!'. The inability to retrieve the assigned value back from the $Args may be confusing to those people who don't yet know that $Args is actually an automatic variable.

[mklement0]

For convenience, here's the list of _all_ automatic variables that _shouldn't_ be writable, but currently are (copied from the linked issue):

As @lzybkr has pointed out, this lists needs careful vetting, however, to prevent false positives.

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
ReportErrorShowExceptionClass      False
ReportErrorShowInnerException      False
ReportErrorShowSource              False
ReportErrorShowStackTrace          False
Sender                             False
StackTrace                          True
This                               False

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

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):

$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.

[iSazonov]

We need two rule:

1. $_ /$PSItem and $input should be read only.
   Conclusion @powershell-committee for $_/$PSItem https://github.com/PowerShell/PowerShell/issues/3695#issuecomment-319826645
   Conclusion @powershell-committee for $input https://github.com/PowerShell/PowerShell/issues/3061#issuecomment-319826831
2. For rest automatic variables "It is recommended to be read only (don't reuse)"
   Conclusion @powershell-committee for this. https://github.com/PowerShell/PowerShell/issues/3061#issuecomment-319826831

[mklement0]

Just coming across this again, @iSazonov: The committee ultimately decided _not_ to enforce the read-only status of _any_ automatic variables at the engine level - see https://github.com/PowerShell/PowerShell/issues/3695#issuecomment-324491388.

[bergmeister]

@mklement0 I created the AvoidAssignmentToAutomaticVariable rule a year ago for it and made it lint on read-only variables only so far, more automatic variables could be added.

[mklement0]

^{Sorry, @bergmeister - it took a while for some conceptual fog to clear, so I've deleted the previous comment - hopefully, this one now presents a clear picture.}

Thanks, @bergmeister - didn't actually know about that rule. Since it doesn't cover the truly problematic cases listed above - those that aren't read-only, but _should be_, adding these variables is indeed needed.

I think two actions are needed:

• Activate PSAvoidAssignmentToAutomaticVariable _by default_ in the VSCode PowerShell extension - it currently isn't, which is why I (and probably many others) was unaware of it.

• Add the variables listed above to the existing PSAvoidAssignmentToAutomaticVariable rule, as _warnings_.

  • They should only be warnings, because you _can sometimes get away with_ setting such automatic variables - notably in the case of $_ / $PSItem - and there definitely is code out there that does it.

P.S.: I've just summarized the issue of inadvertently assigning to automatic variables in this Stack Overflow answer, which describes the status quo (this is partially a note to self to remind me of updating that answer once the actions above are implemented).

[bergmeister]

Yes, I forgot to add that rule to the default set of rules uses in the vs code extension, I created a PR for this: https://github.com/PowerShell/PowerShellEditorServices/pull/1101

Yes, there is the backlog of warning also on non-readonly variables, I did not do it straight away because I got push-back from Jason because some of them can be assigned to by design and there was homework to do, which non-read-only variables we should warn about but it seems you've already done that, so all we have to do is add them here in a 2nd list of variables with lower severity. If you could give me just a list of variables to add to this new list, that would be great so that I don't have to go through the effort of reading and understanding your long post :-)

[mklement0]

Great - thanks, @bergmeister, also for opening the PSES PR.

[bergmeister]

@mklement0 You're welcome. Could you please provide me with a simple list of 'naughty but not too naughty' automatic variables (i.e. not read-only but the ones that should still not be assigned to)? It would help me not having to parse your long SO article. :-)

[mklement0]

Your wish is my command; alas, the list is not quite that simple, but it comes down to this:

• Decide whether to include the names of _legacy_ (WinPS-only) variables and pick the appropriate list below.

• Review the list to make sure there aren't variables on them that _should_ be writeable.

  • The list already excludes the following, which to my knowledge are _all_ that should be excluded: $null, $OFS, $LASTEXITCODE (though it's better to use exit <n> for the latter)
  • Perhaps @lzybkr can take a look too.

As an aside: There are automatic variables whose _value_ (the object they reference) you're allowed to modify (e.g., $Error, $PSBoundParameters), but that is unrelated to trying to redefine the _variable_.

---

Below is the list of technically-read/write-but-should-be-readonly variables, created with the following command (it extracts the list of all automatic variables to examine from the about_Automatic_Variables help topic):

$( 
  foreach ($varName in ((Get-Help about_automatic_variables) -split [environment]::newline -match '^\s*\$\w+\s*$').Trim().Trim('$') | Sort-Object -Unique) {
    $var = Get-Variable -Scope Global $varName -ErrorAction Ignore
    if ($var.Options -match 'readonly|constant') {
      Write-Verbose "$varName`: read-only or constant"
    }
    elseif ($varName -in 'null', 'OFS', 'LastExitCode') {
      # exceptions - variables you *should* be able to assign to.
      Write-Verbose "$varName`: writable by design"
    }
    else { # non-readonly / non-constant 
      # Test to make sure you can actually assign without error.
      Set-Variable -Name $varName -Value $null -ErrorAction SilentlyContinue
      if ($?) {
        $varName
      }
    }
  }
) -replace '(^|$)', '"' -join ', ' 

Note:

• Running the command in _Windows PowerShell_ also covers _legacy_ automatic variables that are are no longer present in PowerShell _Core_ - not sure if that's necessary, though.

  • Specifically, the following among the problematic automatic variables apparently no longer exist in PowerShell Core:

# These were seemingly never implemented  even in WinPS
ReportErrorShowExceptionClass
ReportErrorShowInnerException
ReportErrorShowSource
ReportErrorShowStackTrace

• AllNodes is DSC-related and there are still tab-completion tests for it in the Core repo.

Windows PowerShell List

"_", "AllNodes", "Args", "Event", "EventArgs", "EventSubscriber", "ForEach", "Input", "Matches", "MyInvocation", "NestedPromptLevel", "Profile", "PSBoundParameters", "PsCmdlet", "PSCommandPath", "PSDebugContext", "PSItem", "PSScriptRoot", "PSSenderInfo", "Pwd", "ReportErrorShowExceptionClass", "ReportErrorShowInnerException", "ReportErrorShowSource", "ReportErrorShowStackTrace", "Sender", "StackTrace", "This"

Readable version:

AllNodes
Args
Event
EventArgs
EventSubscriber
ForEach
Input
Matches
MyInvocation
NestedPromptLevel
Profile
PSBoundParameters
PsCmdlet
PSCommandPath
PSDebugContext
PSItem
PSScriptRoot
PSSenderInfo
Pwd
ReportErrorShowExceptionClass
ReportErrorShowInnerException
ReportErrorShowSource
ReportErrorShowStackTrace
Sender
StackTrace
This

PowerShell Core List

"_", "ARGS", "CONSOLEFILENAME", "EVENT", "EVENTARGS", "EVENTSUBSCRIBER", "FOREACH", "INPUT", "MATCHES", "MYINVOCATION", "NESTEDPROMPTLEVEL", "PROFILE", "PSBOUNDPARAMETERS", "PSCMDLET", "PSCOMMANDPATH", "PSDEBUGCONTEXT", "PSITEM", "PSSCRIPTROOT", "PSSENDERINFO", "PWD", "SENDER", "StackTrace", "THIS"

ARGS
CONSOLEFILENAME
EVENT
EVENTARGS
EVENTSUBSCRIBER
FOREACH
INPUT
MATCHES
MYINVOCATION
NESTEDPROMPTLEVEL
PROFILE
PSBOUNDPARAMETERS
PSCMDLET
PSCOMMANDPATH
PSDEBUGCONTEXT
PSITEM
PSSCRIPTROOT
PSSENDERINFO
PWD
SENDER
StackTrace
THIS

[lzybkr]

That list seems good. I do think it's a good rule, but curiously I've intentionally violated the proposed rule in a way that I think makes sense.

I rebind $PSScriptRoot to handle when the script might be invoked as a symbolic link. This is arguably a PowerShell bug, but this is a useful workaround for readability by not introducing a variable that means the same thing.

[lzybkr]

Legacy is probably better - plenty of folks will stay with 5.1 for a long time.

And I do agree PSScriptRoot should be included - I just wanted to provide a scenario where making it a hard error maybe seems unreasonable and I'm certain there will be others.