From d39e7ccaa6879a2317ea6630c5a1ecee0e044db4 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 18:05:54 +0000 Subject: [PATCH 1/4] revert rule to not do check of variable usage but improve it to not warn about certain operators on the RHS --- ...sibleIncorrectUsageOfAssignmentOperator.cs | 47 +++++++------------ ...correctUsageOfComparisonOperator.tests.ps1 | 24 +++++++--- 2 files changed, 33 insertions(+), 38 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 2bd45617c..787c9b503 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -18,7 +18,6 @@ #endif using System.Management.Automation.Language; using System.Globalization; -using System.Linq; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -43,42 +42,33 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { foreach (var clause in ifStatementAst.Clauses) { - var ifStatementCondition = clause.Item1; - var assignmentStatementAst = ifStatementCondition.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst; + 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 PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName); + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + GetName(), DiagnosticSeverity.Warning, fileName); } else { - var leftVariableExpressionAstOfAssignment = assignmentStatementAst.Left as VariableExpressionAst; - if (leftVariableExpressionAstOfAssignment != null) - { - 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 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; + // If the RHS contains a BinaryExpressionAst, then we also do not want to warn because this could e.g. be 'if ($a = $b -match $c){ }' + var binaryExpressionAst = assignmentStatementAst.Right.Find(testAst => testAst is BinaryExpressionAst, searchNestedScriptBlocks: true) as BinaryExpressionAst; - if (!variableOnLHSIsBeingUsed) - { - yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName); - } - } + if (commandAst == null && invokeMemberExpressionAst == null && binaryExpressionAst == null) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + GetName(), DiagnosticSeverity.Information, fileName); } } - } var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst; @@ -92,11 +82,6 @@ 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. /// @@ -138,7 +123,7 @@ public SourceType GetSourceType() /// public RuleSeverity GetSeverity() { - return RuleSeverity.Warning; + return RuleSeverity.Information; } /// diff --git a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 index 77b77429a..d8e34ffe3 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 @@ -60,23 +60,33 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { } Context "When there are no violations" { - It "returns no violations when there is no equality operator" { + It "returns no violations when correct equality operator is used" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 0 } - 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} + It "returns no violations when using an InvokeMemberExpressionAst like a .net method on the RHS" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = [System.IO.Path]::GetTempFileName()){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 0 } - 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} + It "returns no violations when there is an InvokeMemberExpressionAst on the RHS that looks like a variable" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $PSCmdlet.GetVariableValue($foo){ }' | 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 but the assigned variable on the LHS is used" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){ $b = $a }' | Where-Object {$_.RuleName -eq $ruleName} + It "returns no violations when using an expression like a Binaryexpressionastast on the RHS" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $b -match $c){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 0 + } + + It "returns no violations when there is a command 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 a command 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 } } From c3fdef44849bdef260a74b8a92ec289dea3d67f8 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 18:25:32 +0000 Subject: [PATCH 2/4] implement clang and more tweaks --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 13 ++++++++++++- ...ibleIncorrectUsageOfComparisonOperator.tests.ps1 | 12 +++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 787c9b503..62dfd359f 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -43,7 +43,7 @@ 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; - if (assignmentStatementAst != null) + if (assignmentStatementAst != null && !ClangStylesuppresionIsUsed(assignmentStatementAst.Extent)) { // 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 @@ -82,6 +82,17 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } } + /// + /// 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 + /// + /// + /// + private bool ClangStylesuppresionIsUsed(IScriptExtent scriptExtent) + { + return scriptExtent.Text.StartsWith("(") && scriptExtent.Text.EndsWith(")"); + } + /// /// GetName: Retrieves the name of this rule. /// diff --git a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 index d8e34ffe3..fe45813e1 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 @@ -43,6 +43,11 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings.Count | Should -Be 1 } + It "File redirection operator with equals sign inside if statement causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a >=){}' | 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 @@ -65,13 +70,18 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings.Count | Should -Be 0 } + It "returns no violations when using implicit clang style suppresion" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ( ($a -eq $b) ){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 0 + } + It "returns no violations when using an InvokeMemberExpressionAst like a .net method on the RHS" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = [System.IO.Path]::GetTempFileName()){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 0 } It "returns no violations when there is an InvokeMemberExpressionAst on the RHS that looks like a variable" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $PSCmdlet.GetVariableValue($foo){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $PSCmdlet.GetVariableValue($foo)){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 0 } From 33a19ecd726ac791b5a8a3a4461209b495624934 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 19:26:22 +0000 Subject: [PATCH 3/4] final tweaks and add docs --- ...ssibleIncorrectUsageOAssignmentOperator.md | 54 ++++++++++ ...sibleIncorrectUsageOfComparisonOperator.md | 43 -------- ...ibleIncorrectUsageOfRedirectionOperator.md | 29 ++++++ Rules/ClangSuppresion.cs | 24 +++++ ...sibleIncorrectUsageOfAssignmentOperator.cs | 48 ++------- ...ibleIncorrectUsageOfRedirectionOperator.cs | 99 +++++++++++++++++++ Rules/Strings.Designer.cs | 64 +++++++++--- Rules/Strings.resx | 30 ++++-- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 4 +- ...orrectUsageOfAssignmentOperator.tests.ps1} | 27 +---- ...orrectUsageOfRedirectionOperator.tests.ps1 | 38 +++++++ 11 files changed, 328 insertions(+), 132 deletions(-) create mode 100644 RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md delete mode 100644 RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md create mode 100644 RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md create mode 100644 Rules/ClangSuppresion.cs create mode 100644 Rules/PossibleIncorrectUsageOfRedirectionOperator.cs rename Tests/Rules/{PossibleIncorrectUsageOfComparisonOperator.tests.ps1 => PossibleIncorrectUsageOfAssignmentOperator.tests.ps1} (73%) create mode 100644 Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 diff --git a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md new file mode 100644 index 000000000..1c8bbdd82 --- /dev/null +++ b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md @@ -0,0 +1,54 @@ +# PossibleIncorrectUsageOfComparisonOperator + +**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 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: + +```` powershell +if (($shortVariableName = $SuperLongVariableName['SpecialItem']['AnotherItem'])) +{ + ... +} +```` \ No newline at end of file diff --git a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md deleted file mode 100644 index 82aa65c47..000000000 --- a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md +++ /dev/null @@ -1,43 +0,0 @@ -# PossibleIncorrectUsageOfComparisonOperator - -**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 `>=`. Similarly 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 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 -} -```` \ No newline at end of file diff --git a/RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md new file mode 100644 index 000000000..24b0efebf --- /dev/null +++ b/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) +{ + ... +} +```` \ No newline at end of file diff --git a/Rules/ClangSuppresion.cs b/Rules/ClangSuppresion.cs new file mode 100644 index 000000000..9313413e8 --- /dev/null +++ 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 +{ + /// + /// 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. + /// + internal static class ClangSuppresion + { + /// + /// 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 + /// + /// + /// + internal static bool ScriptExtendIsWrappedInParenthesis(IScriptExtent scriptExtent) + { + return scriptExtent.Text.StartsWith("(") && scriptExtent.Text.EndsWith(")"); + } + } +} diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 62dfd359f..6e26a6e4f 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/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; @@ -26,9 +17,9 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules /// The origin of this rule is that people often forget that operators change when switching between different languages such as C# and PowerShell. /// #if !CORECLR -[Export(typeof(IScriptRule))] + [Export(typeof(IScriptRule))] #endif - public class PossibleIncorrectUsageOfComparisonOperator : AstVisitor, IScriptRule + public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule { /// /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements. @@ -43,14 +34,14 @@ 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; - if (assignmentStatementAst != null && !ClangStylesuppresionIsUsed(assignmentStatementAst.Extent)) + if (assignmentStatementAst != null && !ClangSuppresion.ScriptExtendIsWrappedInParenthesis(assignmentStatementAst.Extent)) { // 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, + Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, GetName(), DiagnosticSeverity.Warning, fileName); } else @@ -65,41 +56,22 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (commandAst == null && invokeMemberExpressionAst == null && binaryExpressionAst == null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, 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); - } } } } - /// - /// 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 - /// - /// - /// - private bool ClangStylesuppresionIsUsed(IScriptExtent scriptExtent) - { - return scriptExtent.Text.StartsWith("(") && scriptExtent.Text.EndsWith(")"); - } - /// /// GetName: Retrieves the name of this rule. /// /// The name of this rule public string GetName() { - return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfComparisonOperatorName); + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfAssignmentOperatorName); } /// @@ -108,7 +80,7 @@ public string GetName() /// The common name of this rule public string GetCommonName() { - return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorCommonName); + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorCommonName); } /// @@ -117,7 +89,7 @@ public string GetCommonName() /// The description of this rule public string GetDescription() { - return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorDescription); + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorDescription); } /// diff --git a/Rules/PossibleIncorrectUsageOfRedirectionOperator.cs b/Rules/PossibleIncorrectUsageOfRedirectionOperator.cs new file mode 100644 index 000000000..ca309a172 --- /dev/null +++ b/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 +{ + /// + /// 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. + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class PossibleIncorrectUsageOfRedirectionOperator : AstVisitor, IScriptRule + { + /// + /// The idea is to get all FileRedirectionAst inside all IfStatementAst clauses. + /// + public IEnumerable 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); + } + } + } + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfRedirectionOperatorName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfRedirectionOperatorCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfRedirectionOperatorDescription); + } + + /// + /// GetSourceType: Retrieves the type of the rule: builtin, managed or module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// GetSeverity: Retrieves the severity of the rule: error, warning of information. + /// + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// GetSourceName: Retrieves the module/assembly name the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index db62a7930..40ea3f156 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1585,45 +1585,81 @@ internal static string PossibleIncorrectComparisonWithNullName { /// /// Looks up a localized string similar to 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.. /// - internal static string PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError { + internal static string PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError", resourceCulture); } } /// - /// Looks up a localized string similar to '=' operator means assignment. Did you mean the equal operator '-eq'?. + /// Looks up a localized string similar to '=' is not an assignment operator. Did you mean the equal operator '-eq'?. /// - internal static string PossibleIncorrectUsageOfComparisonOperatorCommonName { + internal static string PossibleIncorrectUsageOfAssignmentOperatorCommonName { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorCommonName", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorCommonName", resourceCulture); } } /// - /// Looks up a localized string similar to '>', '=' or '==' are not comparison operators in the PowerShell language and rarely needed inside if statements.. + /// Looks up a localized string similar to '=' or '==' are not comparison operators in the PowerShell language and rarely needed inside if statements.. /// - internal static string PossibleIncorrectUsageOfComparisonOperatorDescription { + internal static string PossibleIncorrectUsageOfAssignmentOperatorDescription { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorDescription", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorDescription", resourceCulture); } } /// - /// Looks up a localized string similar to Did you really mean to use the redirection operator '>'? If you wanted to use a comparison operator then use the '-gt' (greater than) or '-ge' (greater or equal) operators.. + /// Looks up a localized string similar to Did you mean to use the assignment operator '='? The equals operator in PowerShell is 'eq'.. /// - internal static string PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError { + internal static string PossibleIncorrectUsageOfAssignmentOperatorError { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorError", resourceCulture); } } /// - /// Looks up a localized string similar to PossibleIncorrectUsageOfComparisonOperator. + /// Looks up a localized string similar to PossibleIncorrectUsageOfAssignmentOperator. /// - internal static string PossibleIncorrectUsageOfComparisonOperatorName { + internal static string PossibleIncorrectUsageOfAssignmentOperatorName { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorName", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to '>' is not a comparison operator. Use '-gt' (greater than) or '-ge' (greater or equal).. + /// + internal static string PossibleIncorrectUsageOfRedirectionOperatorCommonName { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfRedirectionOperatorCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to When switching between different languages it is easy to forget that '>' does not mean 'great than' in PowerShell.. + /// + internal static string PossibleIncorrectUsageOfRedirectionOperatorDescription { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfRedirectionOperatorDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Did you mean to use the redirection operator '>'? The comparison operators in PowerShell are '-gt' (greater than) or '-ge' (greater or equal).. + /// + internal static string PossibleIncorrectUsageOfRedirectionOperatorError { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfRedirectionOperatorError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to PossibleIncorrectUsageOfRedirectionOperator. + /// + internal static string PossibleIncorrectUsageOfRedirectionOperatorName { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfRedirectionOperatorName", resourceCulture); } } diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 6e94354e0..3b10c071e 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -981,14 +981,14 @@ Assignment statements are not aligned - - '=' operator means assignment. Did you mean the equal operator '-eq'? + + '=' is not an assignment operator. Did you mean the equal operator '-eq'? - + 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. - - PossibleIncorrectUsageOfComparisonOperator + + PossibleIncorrectUsageOfAssignmentOperator Use a different variable name @@ -1005,10 +1005,22 @@ AvoidAssignmentToAutomaticVariable - - '>', '=' or '==' are not comparison operators in the PowerShell language and rarely needed inside if statements. + + '=' or '==' are not comparison operators in the PowerShell language and rarely needed inside if statements. - - Did you really mean to use the redirection operator '>'? If you wanted to use a comparison operator then use the '-gt' (greater than) or '-ge' (greater or equal) operators. + + Did you mean to use the assignment operator '='? The equals operator in PowerShell is 'eq'. + + + '>' is not a comparison operator. Use '-gt' (greater than) or '-ge' (greater or equal). + + + When switching between different languages it is easy to forget that '>' does not mean 'great than' in PowerShell. + + + Did you mean to use the redirection operator '>'? The comparison operators in PowerShell are '-gt' (greater than) or '-ge' (greater or equal). + + + PossibleIncorrectUsageOfRedirectionOperator \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 723f12017..942ef3aec 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -61,7 +61,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 54 + $expectedNumRules = 55 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" { diff --git a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 similarity index 73% rename from Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 rename to Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index fe45813e1..f01a193bf 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -1,5 +1,5 @@ #Import-Module PSScriptAnalyzer -$ruleName = "PSPossibleIncorrectUsageOfComparisonOperator" +$ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator" Describe "PossibleIncorrectUsageOfComparisonOperator" { Context "When there are violations" { @@ -37,31 +37,6 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $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 with equals sign inside if statement causes warning" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a >=){}' | 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" { diff --git a/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 new file mode 100644 index 000000000..c9c84e0b7 --- /dev/null +++ b/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 @@ -0,0 +1,38 @@ +#Import-Module PSScriptAnalyzer +$ruleName = "PSPossibleIncorrectUsageOfRedirectionOperator" + +Describe "PossibleIncorrectUsageOfComparisonOperator" { + Context "When there are violations" { + 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 with equals sign inside if statement causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a >=){}' | 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" { + It "returns no violations when using correct greater than operator" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -gt $b){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 0 + } + } +} From 00f520a6c12eecc853590c0d0ae309a7b93b9e91 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 19:31:23 +0000 Subject: [PATCH 4/4] fix rule name in doc --- RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md index 1c8bbdd82..4dcb13510 100644 --- a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md +++ b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md @@ -1,4 +1,4 @@ -# PossibleIncorrectUsageOfComparisonOperator +# PossibleIncorrectUsageOfAssignmentOperator **Severity Level: Information**