Warn when using FileRedirection operator inside if/while statements and improve new PossibleIncorrectUsageOfAssignmentOperator rule (#881)

* first working prototype that warns against usage of the redirection operator inside if statements.
TODO: rename rule, adapt documentation and error message strings

* adapt error message strings and add tests

* remove old todo comment

* remove duplicated test case

* syntax fix from last cleanup commits

* tweak extents in error message: for equal sign warnings, it will point to the equal sign (instead of the RHS only) and fore file redirections it will be the redirection ast (e.g. '> $b')

* Enhance check for assignment by rather checking if assigned variable is used in assignment block

to avoid false positives for assignments: check if variable on LHS is used in statement block
update documentation and change level to warning since we are now much more certain that a violation. is happening

Commits:

* prototype of detecting variable usage on LHS to avoid  false positives

* simplify and reduce nesting

* fix regression by continue statements

* fix last regression

* add simple test case for new improvemeent

* finish it off and adapt tests

* update docs

* minor style update

* fix typo

* fix test due to warning level change

* tweak messages and readme

* update to pester v4 syntax

* Revert to not check assigned variable usage of RHS but add optional clang suppression, split rule and enhance assignment operator rule to not warn for more special cases on the RHS

* Minor fix resource variable naming

* uncommented accidental comment out of ipmo pssa in tests

* do not exclude BinaryExpressionAst on RHS because this case is the chance that it is by design is much smaller, therefore rather having some false positives is preferred

* update test to test against clang suppression

* make clang suppression work again

* reword warning text to use 'equality operator' instead of 'equals operator'

* Always warn when LHS is $null

* Enhance PossibleIncorrectUsageOfAssignmentOperator rule to do the same analysis for while and do-while statements as well, which is the exact same use case as if statements

* tweak message to be more generic in terms of the conditional statement

* Address PR comments

Author: bergmeister

RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md

@@ -0,0 +1,54 @@
+# PossibleIncorrectUsageOfAssignmentOperator
+
+**Severity Level: Information**
+
+## Description
+
+In many programming languages, the equality operator is denoted as `==` or `=` in many programming languages, but `PowerShell` uses `-eq`. Therefore it can easily happen that the wrong operator is used unintentionally and this rule catches a few special cases where the likelihood of that is quite high.
+
+The rule looks for usages of `==` and `=` operators inside `if`, `else if`, `while` and `do-while` statements but it will not warn if any kind of command or expression is used at the right hand side as this is probably by design.
+
+## Example
+
+### Wrong
+
+```` PowerShell
+if ($a = $b)
+{
+    ...
+}
+````
+
+```` PowerShell
+if ($a == $b)
+{
+
+}
+````
+
+### Correct
+
+```` PowerShell
+if ($a -eq $b) # Compare $a with $b
+{
+    ...
+}
+````
+
+```` PowerShell
+if ($a = Get-Something) # Only execute action if command returns something and assign result to variable
+{
+    Do-SomethingWith $a
+}
+````
+
+## Implicit suppresion using Clang style
+
+There are some rare cases where assignment of variable inside an if statement is by design. Instead of suppression the rule, one can also signal that assignment was intentional by wrapping the expression in extra parenthesis. An exception for this is when `$null` is used on the LHS is used because there is no use case for this.
+
+```` powershell
+if (($shortVariableName = $SuperLongVariableName['SpecialItem']['AnotherItem']))
+{
+    ...
+}
+````

RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md

@@ -0,0 +1,29 @@
+# PossibleIncorrectUsageOfRedirectionOperator
+
+**Severity Level: Information**
+
+## Description
+
+In many programming languages, the comparison operator for 'greater than' is `>` but `PowerShell` uses `-gt` for it and `-ge` (greater or equal) for `>=`. Therefore it can easily happen that the wrong operator is used unintentionally and this rule catches a few special cases where the likelihood of that is quite high.
+
+The rule looks for usages of `>` or `>=` operators inside if, elseif, while and do-while statements because this is likely going to be unintentional usage.
+
+## Example
+
+### Wrong
+
+```` PowerShell
+if ($a > $b)
+{
+    ...
+}
+````
+
+### Correct
+
+```` PowerShell
+if ($a -gt $b)
+{
+    ...
+}
+````

RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md

@@ -0,0 +1,24 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System.Management.Automation.Language;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
+{
+    /// <summary>
+    /// The idea behind clang suppresion style is to wrap a statement in extra parenthesis to implicitly suppress warnings of its content to signal that the offending operation was deliberate.
+    /// </summary>
+    internal static class ClangSuppresion
+    {
+        /// <summary>
+        /// The community requested an implicit suppression mechanism that follows the clang style where warnings are not issued if the expression is wrapped in extra parenthesis.
+        /// See here for details: https://github.com/Microsoft/clang/blob/349091162fcf2211a2e55cf81db934978e1c4f0c/test/SemaCXX/warn-assignment-condition.cpp#L15-L18
+        /// </summary>
+        /// <param name="scriptExtent"></param>
+        /// <returns></returns>
+        internal static bool ScriptExtendIsWrappedInParenthesis(IScriptExtent scriptExtent)
+        {
+            return scriptExtent.Text.StartsWith("(") && scriptExtent.Text.EndsWith(")");
+        }
+    }
+}

Rules/ClangSuppresion.cs

@@ -13,53 +13,89 @@
 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, elseif, while and do-while 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
     {
         /// <summary>
-        /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements.
+        /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst/WhileStatementAst/DoWhileStatementAst,
+        /// which includes if, elseif, while and do-while statements.
         /// </summary>
         public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
+            var whileStatementAsts = ast.FindAll(testAst => testAst is WhileStatementAst || testAst is DoWhileStatementAst, searchNestedScriptBlocks: true);
+            foreach (LoopStatementAst whileStatementAst in whileStatementAsts)
+            {
+                var diagnosticRecord = AnalyzePipelineBaseAst(whileStatementAst.Condition, fileName);
+                if (diagnosticRecord != null)
+                {
+                    yield return diagnosticRecord;
+                }
+            }
+
             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)
+                    var diagnosticRecord = AnalyzePipelineBaseAst(clause.Item1, fileName);
+                    if (diagnosticRecord != 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);
-                            }
-                        }
+                        yield return diagnosticRecord;
                     }
                 }
             }
         }
 
+        private DiagnosticRecord AnalyzePipelineBaseAst(PipelineBaseAst pipelineBaseAst, string fileName)
+        {
+            var assignmentStatementAst = pipelineBaseAst.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
+            if (assignmentStatementAst == null)
+            {
+                return 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("="))
+            {
+                return new DiagnosticRecord(
+                    Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
+                    GetName(), DiagnosticSeverity.Warning, fileName);
+            }
+
+            // Check if LHS is $null and then always warn
+            if (assignmentStatementAst.Left is VariableExpressionAst variableExpressionAst)
+            {
+                if (variableExpressionAst.VariablePath.UserPath.Equals("null", StringComparison.OrdinalIgnoreCase))
+                {
+                    return new DiagnosticRecord(
+                        Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
+                        GetName(), DiagnosticSeverity.Warning, fileName);
+                }
+            }
+
+            // If the RHS 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 the RHS contains an InvokeMemberExpressionAst, then we also do not want to warn because this could e.g. be 'if ($f = [System.IO.Path]::GetTempFileName()){ }'
+            var invokeMemberExpressionAst = assignmentStatementAst.Right.Find(testAst => testAst is ExpressionAst, searchNestedScriptBlocks: true) as InvokeMemberExpressionAst;
+            var doNotWarnBecauseImplicitClangStyleSuppressionWasUsed = ClangSuppresion.ScriptExtendIsWrappedInParenthesis(pipelineBaseAst.Extent);
+            if (commandAst == null && invokeMemberExpressionAst == null && !doNotWarnBecauseImplicitClangStyleSuppressionWasUsed)
+            {
+                return new DiagnosticRecord(
+                   Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
+                   GetName(), DiagnosticSeverity.Information, fileName);
+            }
+
+            return null;
+        }
+
         /// <summary>
         /// GetName: Retrieves the name of this rule.
         /// </summary>
@@ -84,7 +120,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.PossibleIncorrectUsageOfAssignmentOperatorDescription);
         }
 
         /// <summary>

Rules/PossibleIncorrectUsageOfAssignmentOperator.cs

@@ -0,0 +1,99 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+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>
+    /// PossibleIncorrectUsageOfRedirectionOperator: Warn if someone uses '>' or '>=' inside an if, elseif, while or do-while 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 PossibleIncorrectUsageOfRedirectionOperator : AstVisitor, IScriptRule
+    {
+        /// <summary>
+        /// The idea is to get all FileRedirectionAst inside all IfStatementAst clauses.
+        /// </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 fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst;
+                    if (fileRedirectionAst != null)
+                    {
+                        yield return new DiagnosticRecord(
+                            Strings.PossibleIncorrectUsageOfRedirectionOperatorError, fileRedirectionAst.Extent,
+                            GetName(), DiagnosticSeverity.Warning, 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.PossibleIncorrectUsageOfRedirectionOperatorName);
+        }
+
+        /// <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.PossibleIncorrectUsageOfRedirectionOperatorCommonName);
+        }
+
+        /// <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.PossibleIncorrectUsageOfRedirectionOperatorDescription);
+        }
+
+        /// <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);
+        }
+    }
+}