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 build 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. The remaining 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

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!--
-    Microsoft ResX Schema
-
+  <!-- 
+    Microsoft ResX Schema 
+    
     Version 2.0
-
-    The primary goals of this format is to allow a simple XML format
-    that is mostly human readable. The generation and parsing of the
-    various data types are done through the TypeConverter classes
+    
+    The primary goals of this format is to allow a simple XML format 
+    that is mostly human readable. The generation and parsing of the 
+    various data types are done through the TypeConverter classes 
     associated with the data types.
-
+    
     Example:
-
+    
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-
-    There are any number of "resheader" rows that contain simple
+                
+    There are any number of "resheader" rows that contain simple 
     name/value pairs.
-
-    Each data row contains a name, and value. The row also contains a
-    type or mimetype. Type corresponds to a .NET class that support
-    text/value conversion through the TypeConverter architecture.
-    Classes that don't support this are serialized and stored with the
+    
+    Each data row contains a name, and value. The row also contains a 
+    type or mimetype. Type corresponds to a .NET class that support 
+    text/value conversion through the TypeConverter architecture. 
+    Classes that don't support this are serialized and stored with the 
     mimetype set.
-
-    The mimetype is used for serialized objects, and tells the
-    ResXResourceReader how to depersist the object. This is currently not
+    
+    The mimetype is used for serialized objects, and tells the 
+    ResXResourceReader how to depersist the object. This is currently not 
     extensible. For a given mimetype the value must be set accordingly:
-
-    Note - application/x-microsoft.net.object.binary.base64 is the format
-    that the ResXResourceWriter will generate, however the reader can
+    
+    Note - application/x-microsoft.net.object.binary.base64 is the format 
+    that the ResXResourceWriter will generate, however the reader can 
     read any of the formats listed below.
-
+    
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-
+    
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array
+    value   : The object must be serialized into a byte array 
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -957,7 +957,6 @@
   <data name="UseConsistentWhitespaceErrorSeparatorSemi" xml:space="preserve">
     <value>Use space after a semicolon.</value>
   </data>
-
   <data name="UseSupportsShouldProcessName" xml:space="preserve">
     <value>UseSupportsShouldProcess</value>
   </data>
@@ -982,4 +981,19 @@
   <data name="AlignAssignmentStatementError" xml:space="preserve">
     <value>Assignment statements are not aligned</value>
   </data>
-</root>
+  <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>Variable {0} should use a different name since it is a readonly automatic variable.</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 = 52
+            $expectedNumRules = 53
             if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4))
             {
                 # for PSv3 PSAvoidGlobalAliases is not shipped because