GeezerVison is showing github.com — New Rule Suggestion: AvoidReuseOfAutomaticVariables · Issue #712 · PowerShell/PSScriptAnalyzer
New Rule Suggestion: AvoidReuseOfAutomaticVariables #712DescriptionI 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. Reacted by Michael Klement and iRon7 Activitymklement0 commented on Aug 4, 2017 For convenience, here's the list of all automatic variables that shouldn't be writable, but currently are (copied from the linked issue):
As Jason Shirk (@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 commented on Aug 7, 2017 mklement0 commented on Nov 12, 2019 bergmeister commented on Nov 12, 2019 Reacted by Michael Klement mklement0 commented on Nov 12, 2019 Sorry, Christoph Bergmeister (@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, Christoph Bergmeister (@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 commented on Nov 12, 2019 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: PowerShell/PowerShellEditorServices#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 :-) Reacted by Michael Klement mklement0 commented on Nov 12, 2019 bergmeister commented on Nov 12, 2019 Michael Klement (@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. :-) Reacted by Michael Klement mklement0 commented on Nov 12, 2019 Your wish is my command; alas, the list is not quite that simple, but it comes down to this:
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:
ConsoleFileName
# 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 commented on Nov 12, 2019 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. mklement0 commented on Nov 12, 2019 Thanks, Jason Shirk (@lzybkr).
-
Do you suggest going with the list with the legacy variables or the one without? (I have no strong feelings either way).
-
Re $PSScriptRoot: Interesting (just to spell it out, which I assume you agree with): it's still worth warning about assigning to this variable.
lzybkr commented on Nov 12, 2019 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. Reacted by Michael Klement
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|