New Rule Suggestion: AvoidReuseOfAutomaticVariables #712
Comments
|
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. "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 |
|
We need two rule:
|
ghost
mentioned this issue
|
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 PowerShell/PowerShell#3695 (comment). |
|
@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. |
|
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:
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). |
|
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 :-) |
|
Great - thanks, @bergmeister, also for opening the PSES PR. |
|
@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. :-) |
|
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., 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 $(
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:
Windows PowerShell ListReadable version: PowerShell Core List |
|
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 |
|
Thanks, @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. |
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$Argsmay be confusing to those people who don't yet know that $Args is actually an automatic variable.The text was updated successfully, but these errors were encountered: