Warn when using FileRedirection operator inside if/while statements and improve new PossibleIncorrectUsageOfAssignmentOperator rule

RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md

@@ -0,0 +1,7 @@
+# PossibleIncorrectUsageOfComparisonOperator
+
+**Severity Level: Information**
+
+## Description
+
+In many programming languages, the comparison operator for greater is `>` but `PowerShell` uses `-gt` for it and `-ge` for `>=`. Similarly the equality operator is denoted as `==` or `=` in many programming languages, but `PowerShell` uses `-eq`. Since using as the FileRedirection operator `>` or the assignment operator are rarely needed inside if statements, this rule wants to call out this case because it was probably unintentional.

Rules/PossibleIncorrectUsageOfAssignmentOperator.cs

@@ -22,12 +22,13 @@
 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.
+    /// PossibleIncorrectUsageOfAssignmentOperator: Warn if someone uses '>', '=' or '==' operators inside an if or elseif statement because in most cases that is not the intention.
+    /// The origin of this rule is that people often forget that operators change when switching between different languages such as C# and PowerShell.
     /// </summary>
 #if !CORECLR
 [Export(typeof(IScriptRule))]
 #endif
-    public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule
+    public class PossibleIncorrectUsageOfComparisonOperator : 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.
@@ -49,7 +50,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                         if (assignmentStatementAst.Right.Extent.Text.StartsWith("="))
                         {
                             yield return new DiagnosticRecord(
-                                Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent,
+                                Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.Extent,
                                 GetName(), DiagnosticSeverity.Warning, fileName);
                         }
                         else
@@ -60,11 +61,19 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                             if (commandAst == null)
                             {
                                 yield return new DiagnosticRecord(
-                                   Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent,
+                                   Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.Extent,
                                    GetName(), DiagnosticSeverity.Information, fileName);
                             }
                         }
                     }
+
+                    var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst;
+                    if (fileRedirectionAst != null)
+                    {
+                        yield return new DiagnosticRecord(
+                            Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent,
+                            GetName(), DiagnosticSeverity.Warning, fileName);
+                    }
                 }
             }
         }
@@ -75,7 +84,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         /// <returns>The name of this rule</returns>
         public string GetName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfAssignmentOperatorName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfComparisonOperatorName);
         }
 
         /// <summary>
@@ -84,7 +93,7 @@ public string GetName()
         /// <returns>The common name of this rule</returns>
         public string GetCommonName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorCommonName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorCommonName);
         }
 
         /// <summary>
@@ -93,7 +102,7 @@ public string GetCommonName()
         /// <returns>The description of this rule</returns>
         public string GetDescription()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingWriteHostDescription);
+            return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorDescription);
         }
 
         /// <summary>

Rules/Strings.resx

@@ -981,14 +981,14 @@
   <data name="AlignAssignmentStatementError" xml:space="preserve">
     <value>Assignment statements are not aligned</value>
   </data>
-  <data name="PossibleIncorrectUsageOfAssignmentOperatorCommonName" xml:space="preserve">
+  <data name="PossibleIncorrectUsageOfComparisonOperatorCommonName" 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 name="PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError" xml:space="preserve">
+    <value>Did you really mean to use the assignment operator '=' 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 name="PossibleIncorrectUsageOfComparisonOperatorName" xml:space="preserve">
+    <value>PossibleIncorrectUsageOfComparisonOperator</value>
   </data>
   <data name="AvoidAssignmentToReadOnlyAutomaticVariable" xml:space="preserve">
     <value>Use a different variable name</value>
@@ -1005,4 +1005,10 @@
   <data name="AvoidAssignmentToAutomaticVariableName" xml:space="preserve">
     <value>AvoidAssignmentToAutomaticVariable</value>
   </data>
+  <data name="PossibleIncorrectUsageOfComparisonOperatorDescription" xml:space="preserve">
+    <value>'&gt;', '=' or '==' are not comparison operators in the PowerShell language and rarely needed inside if statements.</value>
+  </data>
+  <data name="PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError" xml:space="preserve">
+    <value>Did you really mean to use the redirection operator '&gt;'? If you wanted to use an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators.</value>
+  </data>
 </root>

Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 → Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1

@@ -1,7 +1,7 @@
-Import-Module PSScriptAnalyzer
-$ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator"
+#Import-Module PSScriptAnalyzer
+$ruleName = "PSPossibleIncorrectUsageOfComparisonOperator"
 
-Describe "PossibleIncorrectUsageOfAssignmentOperator" {
+Describe "PossibleIncorrectUsageOfComparisonOperator" {
     Context "When there are violations" {
         It "assignment inside if statement causes warning" {
             $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName}
@@ -37,6 +37,26 @@ Describe "PossibleIncorrectUsageOfAssignmentOperator" {
             $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == "$b"){}' | Where-Object {$_.RuleName -eq $ruleName}
             $warnings.Count | Should Be 1
         }
+
+        It "File redirection operator inside if statement causes warning" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a > $b){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "File redirection operator inside if statement causes warning when wrapped in command expression" {
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a > ($b)){}' | Where-Object {$_.RuleName -eq $ruleName}
+            $warnings.Count | Should Be 1
+        }
+
+        It "File redirection operator 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 "File redirection operator 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
+        }
     }
 
     Context "When there are no violations" {