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

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/PossibleIncorrectUsageOfRedirectionOperator.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 statements because this is likely going to be unintentional usage.
+
+## Example
+
+### Wrong
+
+```` PowerShell
+if ($a > $b)
+{
+    ...
+}
+````
+
+### Correct
+
+```` PowerShell
+if ($a -eq $b)
+{
+    ...
+}
+````

Rules/ClangSuppresion.cs

@@ -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/PossibleIncorrectUsageOfAssignmentOperator.cs

@@ -1,14 +1,5 @@
-//
-// 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.
-//
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
 
 using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
 using System;
@@ -22,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>
@@ -93,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/PossibleIncorrectUsageOfRedirectionOperator.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 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 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);
+        }
+    }
+}