Warn when an if statement contains an assignment

Engine/Generic/IScriptRule.cs

@@ -10,11 +10,7 @@
 // THE SOFTWARE.
 //
 
-using System;
 using System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
 using System.Management.Automation.Language;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic

RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md

@@ -0,0 +1,7 @@
+# PossibleIncorrectUsageOfAssignmentOperator
+
+**Severity Level: Information**
+
+## Description
+
+In many programming languages, the equality operator is denoted as `==` or `=`, but `PowerShell` uses `-eq`. Since assignment inside if statements are very rare, this rule wants to call out this case because it might have been unintentional.

Rules/PossibleIncorrectUsageOfAssignmentOperator.cs

@@ -0,0 +1,124 @@
+//
+// 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 Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+using System;
+using System.Collections.Generic;
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+using System.Management.Automation.Language;
+using System.Globalization;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// PossibleIncorrectUsageOfAssignmentOperator: Warn if someone uses the '=' or '==' by accident in an if statement because in most cases that is not the intention.
+    /// </summary>
+#if !CORECLR
+[Export(typeof(IScriptRule))]
+#endif
+    public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule
+    {
+        /// <summary>
+        /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements.
+        /// </summary>
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
+            var ifStatementAsts = ast.FindAll(testAst => testAst is IfStatementAst, searchNestedScriptBlocks: true);
+            foreach (IfStatementAst ifStatementAst in ifStatementAsts)
+            {
+                foreach (var clause in ifStatementAst.Clauses)
+                {
+                    var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
+                    if (assignmentStatementAst != null)
+                    {
+                        // Check if someone used '==', which can easily happen when the person is used to coding a lot in C#.
+                        // In most cases, this will be a runtime error because PowerShell will look for a cmdlet name starting with '=', which is technically possible to define
+                        if (assignmentStatementAst.Right.Extent.Text.StartsWith("="))
+                        {
+                            yield return new DiagnosticRecord(
+                                Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent,
+                                GetName(), DiagnosticSeverity.Warning, fileName);
+                        }
+                        else
+                        {
+                            // If the right hand side contains a CommandAst at some point, then we do not want to warn
+                            // because this could be intentional in cases like 'if ($a = Get-ChildItem){ }'
+                            var commandAst = assignmentStatementAst.Right.Find(testAst => testAst is CommandAst, searchNestedScriptBlocks: true) as CommandAst;
+                            if (commandAst == null)
+                            {
+                                yield return new DiagnosticRecord(
+                                   Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent,
+                                   GetName(), DiagnosticSeverity.Information, 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.PossibleIncorrectUsageOfAssignmentOperatorName);
+        }
+
+        /// <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.PossibleIncorrectUsageOfAssignmentOperatorCommonName);
+        }
+
+        /// <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.AvoidUsingWriteHostDescription);
+        }
+
+        /// <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.Information;
+        }
+
+        /// <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,13 @@
   <data name="AlignAssignmentStatementError" xml:space="preserve">
     <value>Assignment statements are not aligned</value>
   </data>
-</root>
+  <data name="PossibleIncorrectUsageOfAssignmentOperatorCommonName" xml:space="preserve">
+    <value>'=' operator means assignment. Did you mean the equal operator '-eq'?</value>
+  </data>
+  <data name="PossibleIncorrectUsageOfAssignmentOperatorError" xml:space="preserve">
+    <value>Did you really mean to make an assignment inside an if statement? If you rather meant to check for equality, use the '-eq'  operator.</value>
+  </data>
+  <data name="PossibleIncorrectUsageOfAssignmentOperatorName" xml:space="preserve">
+    <value>PossibleIncorrectUsageOfAssignmentOperator</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
@@ -159,7 +159,7 @@ Describe "TestSeverity" {
 
     It "filters rules based on multiple severity inputs"{
         $rules = Get-ScriptAnalyzerRule -Severity Error,Information
-        $rules.Count | Should be 14
+        $rules.Count | Should be 15
     }
 
         It "takes lower case inputs" {

Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1

@@ -0,0 +1,63 @@
+Import-Module PSScriptAnalyzer
+$ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator"
+
+Describe "PossibleIncorrectUsageOfAssignmentOperator" {
+    Context "When there are violations" {
+        It "assignment inside if statement causes warning" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "assignment inside if statement causes warning when when wrapped in command expression" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=($b)){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "assignment inside if statement causes warning when wrapped in expression" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a="$b"){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "assignment inside elseif statement causes warning" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){}elseif($a = $b){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "double equals inside if statement causes warning" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == $b){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "double equals inside if statement causes warning when wrapping it in command expresion" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == ($b)){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "double equals inside if statement causes warning when wrapped in expression" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == "$b"){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+    }
+
+    Context "When there are no violations" {
+        It "returns no violations when there is no equality operator" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){$a=$b}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 0
+        }
+
+        It "returns no violations when there is an evaluation on the RHS" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = Get-ChildItem){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 0
+        }
+
+        It "returns no violations when there is an evaluation on the RHS wrapped in an expression" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 0
+        }
+
+        It "returns no violations when there is an evaluation on the RHS wrapped in an expression and also includes a variable" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem $b)){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 0
+        }
+    }
+}