diff --git a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md index 300bda8fe..9cfb98cfd 100644 --- a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md +++ b/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md @@ -1,7 +1,43 @@ # PossibleIncorrectUsageOfComparisonOperator -**Severity Level: Information** +** Severity Level: Warning ** ## 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. + +The rule looks for usages of `==`, `=` and `>` operators inside if statements and for the case of assignments, it will only warn if the variable is not being used at all in the statement block to avoid false positives because assigning a variable inside an if statement is an elegant way of getting an object and performing an implicit null check on it in one line. + +## Example + +### Wrong + +``` PowerShell +if ($superman > $batman) +{ + +} +``` + +``` PowerShell +if ($superman == $batman) +{ + +} +``` + +``` PowerShell +if ($superman = $batman) +{ + # Not using the assigned variable is an indicator that assignment was either by accident or unintentional +} +``` + +### Correct + +``` PowerShell +if ($superman = Get-Superman) +{ + Save-World $superman +} +``` diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index a6d04f565..2bd45617c 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -18,6 +18,7 @@ #endif using System.Management.Automation.Language; using System.Globalization; +using System.Linq; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -42,29 +43,42 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { foreach (var clause in ifStatementAst.Clauses) { - var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst; + var ifStatementCondition = clause.Item1; + var assignmentStatementAst = ifStatementCondition.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.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, - GetName(), DiagnosticSeverity.Warning, fileName); + yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, 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) + var leftVariableExpressionAstOfAssignment = assignmentStatementAst.Left as VariableExpressionAst; + if (leftVariableExpressionAstOfAssignment != null) { - yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, - GetName(), DiagnosticSeverity.Information, fileName); + var statementBlockOfIfStatenent = clause.Item2; + var variableExpressionAstsInStatementBlockOfIfStatement = statementBlockOfIfStatenent.FindAll(testAst => + testAst is VariableExpressionAst, searchNestedScriptBlocks: true); + if (variableExpressionAstsInStatementBlockOfIfStatement == null) // no variable usages at all + { + yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName); + } + else + { + var variableOnLHSIsBeingUsed = variableExpressionAstsInStatementBlockOfIfStatement.Where(x => x is VariableExpressionAst) + .Any(x => ((VariableExpressionAst)x).VariablePath.UserPath.Equals( + leftVariableExpressionAstOfAssignment.VariablePath.UserPath, StringComparison.OrdinalIgnoreCase)); + + if (!variableOnLHSIsBeingUsed) + { + yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName); + } + } } } + } var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst; @@ -78,6 +92,11 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } } + private DiagnosticRecord PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(IScriptExtent errorPosition, string fileName) + { + return new DiagnosticRecord(Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, errorPosition, GetName(), DiagnosticSeverity.Warning, fileName); + } + /// /// GetName: Retrieves the name of this rule. /// @@ -119,7 +138,7 @@ public SourceType GetSourceType() /// public RuleSeverity GetSeverity() { - return RuleSeverity.Information; + return RuleSeverity.Warning; } /// diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 647301e3c..6314211c4 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -159,7 +159,7 @@ Describe "TestSeverity" { It "filters rules based on multiple severity inputs"{ $rules = Get-ScriptAnalyzerRule -Severity Error,Information - $rules.Count | Should be 15 + $rules.Count | Should be 14 } It "takes lower case inputs" { diff --git a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 index d2696f6be..dd7acea86 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 @@ -61,22 +61,22 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { 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 = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $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} + It "returns no violations when using assignment but the assigned variable on the LHS is used" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $b){ $a.DoSomething() }' | 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} + It "returns no violations when there is an evaluation on the RHS but the assigned variable on the LHS is used" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = Get-ChildItem){ Get-Something $a }' | 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} + It "returns no violations when there is an evaluation on the RHS wrapped in an expression but the assigned variable on the LHS is used" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){ $b = $a }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should Be 0 } }