Warn against assignment to read-only automatic variables

Engine/Generic/DiagnosticRecordHelper.cs

@@ -0,0 +1,13 @@
+using System;
+using System.Globalization;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
+{
+    public static class DiagnosticRecordHelper
+    {
+        public static string FormatError(string format, params object[] args)
+        {
+            return String.Format(CultureInfo.CurrentCulture, format, args);
+        }
+    }
+}

RuleDocumentation/AvoidAssignmentToAutomaticVariable.md

@@ -0,0 +1,33 @@
+# AvoidAssignmentToAutomaticVariable
+
+**Severity Level: Warning**
+
+## Description
+
+`PowerShell` exposes some of its built-in variables that are known as automatic variables. Many of them are read-only and PowerShell would throw an error when trying to assign an value on those. Other automatic variables should only be assigned to in certain special cases to achieve a certain effect as a special technique.
+
+To understand more about automatic variables, see ```Get-Help about_Automatic_Variables```.
+
+## How
+
+Use variable names in functions or their parameters that do not conflict with automatic variables.
+
+## Example
+
+### Wrong
+
+The variable `$Error` is an automatic variables that exists in the global scope and should therefore never be used as a variable or parameter name.
+
+``` PowerShell
+function foo($Error){ }
+```
+
+``` PowerShell
+function Get-CustomErrorMessage($ErrorMessage){ $Error = "Error occurred: $ErrorMessage" }
+```
+
+### Correct
+
+``` PowerShell
+function Get-CustomErrorMessage($ErrorMessage){ $FinalErrorMessage = "Error occurred: $ErrorMessage" }
+```

Rules/AvoidAssignmentToAutomaticVariable.cs

@@ -0,0 +1,128 @@
+//
+// Copyright (c) Microsoft Corporation.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+//
+
+using System;
+using System.Linq;
+using System.Collections.Generic;
+using System.Management.Automation.Language;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+using System.Globalization;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// AvoidAssignmentToAutomaticVariable: 
+    /// </summary>
+#if !CORECLR
+[Export(typeof(IScriptRule))]
+#endif
+    public class AvoidAssignmentToAutomaticVariable : IScriptRule
+    {
+        private static readonly IList<string> _readOnlyAutomaticVariables = new List<string>()
+            {
+                // Attempting to assign to any of those read-only variable would result in an error at runtime
+                "?", "true", "false", "Host", "PSCulture", "Error", "ExecutionContext", "Home", "PID", "PSEdition", "PSHome", "PSUICulture", "PSVersionTable", "ShellId"
+            };
+
+        /// <summary>
+        /// Checks for assignment to automatic variables.
+        /// <param name="ast">The script's ast</param>
+        /// <param name="fileName">The script's file name</param>
+        /// <returns>The diagnostic results of this rule</returns>
+        /// </summary>
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
+            IEnumerable<Ast> assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true);
+            foreach (var assignmentStatementAst in assignmentStatementAsts)
+            {
+                var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
+                var variableName = variableExpressionAst.VariablePath.UserPath;
+                if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase))
+                {
+                    yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName),
+                                                      variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName);
+                }
+            }
+
+            IEnumerable<Ast> parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true);
+            foreach (ParameterAst parameterAst in parameterAsts)
+            {
+                var variableExpressionAst = parameterAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
+                var variableName = variableExpressionAst.VariablePath.UserPath;
+                // also check the parent to exclude parameter attributes such as '[Parameter(Mandatory=$true)]' where the read-only variable $true appears.
+                if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase) && !(variableExpressionAst.Parent is NamedAttributeArgumentAst))
+                {
+                    yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName),
+                                                      variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName);
+                }
+            }
+        }
+
+        /// <summary>
+        /// GetName: Retrieves the name of this rule.
+        /// </summary>
+        /// <returns>The name of this rule</returns>
+        public string GetName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidAssignmentToAutomaticVariableName);
+        }
+
+        /// <summary>
+        /// GetCommonName: Retrieves the common name of this rule.
+        /// </summary>
+        /// <returns>The common name of this rule</returns>
+        public string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableCommonName);
+        }
+
+        /// <summary>
+        /// GetDescription: Retrieves the description of this rule.
+        /// </summary>
+        /// <returns>The description of this rule</returns>
+        public string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableDescription);
+        }
+
+        /// <summary>
+        /// GetSourceType: Retrieves the type of the rule: builtin, managed or module.
+        /// </summary>
+        public SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        /// <summary>
+        /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
+        /// </summary>
+        /// <returns></returns>
+        public RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        /// <summary>
+        /// GetSourceName: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+
+}

Rules/Strings.resx

@@ -990,4 +990,19 @@
   <data name="PossibleIncorrectUsageOfAssignmentOperatorName" xml:space="preserve">
     <value>PossibleIncorrectUsageOfAssignmentOperator</value>
   </data>
+  <data name="AvoidAssignmentToReadOnlyAutomaticVariable" xml:space="preserve">
+    <value>Use a different variable name</value>
+  </data>
+  <data name="AvoidAssignmentToReadOnlyAutomaticVariableCommonName" xml:space="preserve">
+    <value>Changing automtic variables might have undesired side effects</value>
+  </data>
+  <data name="AvoidAssignmentToReadOnlyAutomaticVariableDescription" xml:space="preserve">
+    <value>This automatic variables is built into PowerShell and readonly.</value>
+  </data>
+  <data name="AvoidAssignmentToReadOnlyAutomaticVariableError" xml:space="preserve">
+    <value>The Variable '{0}' cannot be assigned since it is a readonly automatic variable that is built into PowerShell, please use a different name.</value>
+  </data>
+  <data name="AvoidAssignmentToAutomaticVariableName" xml:space="preserve">
+    <value>AvoidAssignmentToAutomaticVariable</value>
+  </data>
 </root>

Tests/Engine/GetScriptAnalyzerRule.tests.ps1

@@ -61,7 +61,7 @@ Describe "Test Name parameters" {
 
         It "get Rules with no parameters supplied" {
 			$defaultRules = Get-ScriptAnalyzerRule
-            $expectedNumRules = 53
+            $expectedNumRules = 54
             if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4))
             {
                 # for PSv3 PSAvoidGlobalAliases is not shipped because

Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1

@@ -0,0 +1,74 @@
+Import-Module PSScriptAnalyzer
+$ruleName = "PSAvoidAssignmentToAutomaticVariable"
+
+Describe "AvoidAssignmentToAutomaticVariables" {
+    Context "ReadOnly Variables" {
+
+        $readOnlyVariableSeverity = "Error"
+        $testCases_ReadOnlyVariables = @(
+            @{ VariableName = '?' }
+            @{ VariableName = 'Error' }
+            @{ VariableName = 'ExecutionContext' }
+            @{ VariableName = 'false' }
+            @{ VariableName = 'Home' }
+            @{ VariableName = 'Host' }
+            @{ VariableName = 'PID' }
+            @{ VariableName = 'PSCulture' }
+            @{ VariableName = 'PSEdition' }
+            @{ VariableName = 'PSHome' }
+            @{ VariableName = 'PSUICulture' }
+            @{ VariableName = 'PSVersionTable' }
+            @{ VariableName = 'ShellId' }
+            @{ VariableName = 'true' }
+        )
+
+        It "Variable '<VariableName>' produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
+            param ($VariableName)
+
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$${VariableName} = 'foo'" | Where-Object { $_.RuleName -eq $ruleName }
+            $warnings.Count | Should Be 1
+            $warnings.Severity | Should Be $readOnlyVariableSeverity
+        }
+
+        It "Using Variable '<VariableName>' as parameter name produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
+            param ($VariableName)
+
+            [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" | Where-Object {$_.RuleName -eq $ruleName }
+            $warnings.Count | Should Be 1
+            $warnings.Severity | Should Be $readOnlyVariableSeverity
+        }
+
+        It "Using Variable '<VariableName>' as parameter name in param block produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
+            param ($VariableName)
+
+            [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" | Where-Object {$_.RuleName -eq $ruleName }
+            $warnings.Count | Should Be 1
+            $warnings.Severity | Should Be $readOnlyVariableSeverity
+        }
+
+        It "Does not flag parameter attributes" {
+            [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'function foo{Param([Parameter(Mandatory=$true)]$param1)}' | Where-Object { $_.RuleName -eq $ruleName }
+            $warnings.Count | Should Be 0
+        }
+
+        It "Setting Variable '<VariableName>' throws exception to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables {
+            param ($VariableName)
+
+            # Setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string, therefore exclude this case.
+            # For the library test in WMF 4, assigning a value $PSEdition does not seem to throw an error, therefore this special case is excluded as well.
+            if ($VariableName -ne 'Error' -and ($VariableName -ne 'PSEdition' -and $PSVersionTable.PSVersion.Major -ne 4))
+            {
+                try
+                {
+                    Set-Variable -Name $VariableName -Value 'foo' -ErrorVariable errorVariable -ErrorAction Stop
+                    throw "Expected exception did not occur when assigning value to read-only variable '$VariableName'"
+                }
+                catch
+                {
+                    $_.FullyQualifiedErrorId | Should Be 'VariableNotWritable,Microsoft.PowerShell.Commands.SetVariableCommand'
+                }
+            }
+        }
+
+    }
+}