PSAvoidAssignmentToAutomaticVariable generates false positives during script analysis

[KirkMunro]

Steps to reproduce

Run this function through PSSA.

function Write-Log {
    [CmdletBinding()]
    param
    (
        [Parameter(Position = 0, Mandatory)]
        [ValidateNotNullOrEmpty()]
        [string]
        $Message,

        [switch]
        $Error
    )
    $callerEAP = $ErrorActionPreference
    try {
        $messageColor = if ($PSCmdlet.MyInvocation.BoundParameters.ContainsKey('Error') -and $Error) {
            [ConsoleColor]::Red
        } else {
            [ConsoleColor]::Cyan
        }

        Write-Host -Foreground $messageColor -Object $Message

        # Reset colors to return to default after an error message on a compilation error
        [System.Console]::ResetColor()
    } catch {
        Write-Error -ErrorRecord $_ -ErrorAction $callerEAP
    }
}

Expected behavior

It passes.

Actual behavior

PSSA reports the following:

The Variable 'Error' cannot be assigned since it is a readonly automatic variable that is built into PowerShell, please use a different name.

There are a number of problems with this:

1. The comma should be a period for it to read properly.
2. The error text is incorrect. $Error can be assigned in child scopes without issue. $Error is read-only only in the global scope, and errors are automatically written into that globally-scoped collection that is read-only from PowerShell. In functions, or child scopes, you can use a $Error variable to your hearts content.
3. Most importantly: the issue that is being called out by PSSA here should not be called out at all.

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      7.0.1
PSEdition                      Core
GitCommitId                    7.0.1
OS                             Microsoft Windows 10.0.18363
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.19.0

[bergmeister]

1: Feel free to open a PR regarding the error message here: https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/Strings.resx#L1039

2 &3: It seems $Error is special compared to other read-only automatic variables, the same doesn't apply to e.g. $PID. One would have to add a lot of code to detect your specific usage of $Error where it is ok and therefore I think the benefit is not paying off the cost. One could also argue that despite PowerShell not having a problem in this instance, it is still not a good practice to use such a variable name and therefore PSSA should still warn. Generally speaking, for special cases where the user knows what they are doing and have reviewed the warning, PSSA offers local suppression, which seems to be the right thing to do here but I'd still argue that a better name like e.g. UseRedAsConsoleColor would help and improve readability. Let's assume the PowerShell team would be OK to accept a PR for points 2/3, would you be willing to invest your time/effort in a PR? If not, then I'd just address the error message and close the issue otherwise as 'wont fix'.

[KirkMunro]

Just circling around to some threads I started but got distracted away from.

For 2 & 3, keep this in mind: the example that I gave is an advanced function definition. If I had defined that command as a binary cmdlet, there would be no error. That is why I feel the PSSA guidance is misguided in this case. It feels wrong that PSSA would report something that is just wrong (it states that "The Variable 'Error' cannot be assigned since it is a readonly automatic variable...", which is incorrect), and that someone should use suppression to silence what is ultimately a bug in PSSA.

As far as whether or not I plan to open a PR, I suppose that depends...Hacktoberfest starts in a little over a month, so maybe.