From 4a0485c8b7ebee1c2b7cfc1d053d160e6b3dd5ba Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 8 Feb 2018 22:17:04 +0000 Subject: [PATCH 01/20] first working prototype that warns against usage of the redirection operator inside if statements. TODO: rename rule, adapt documentation and error message strings --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 8 ++++++++ Rules/Strings.Designer.cs | 9 +++++++++ Rules/Strings.resx | 3 +++ 3 files changed, 20 insertions(+) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index fe34e6d2a..c3b2d224e 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -65,6 +65,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } } } + + var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst; + if (fileRedirectionAst != null) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfFileRedirectionOperatorError, fileRedirectionAst.Extent, // TODO: better error message string and rename rule + GetName(), DiagnosticSeverity.Warning, fileName); + } } } } diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 565e67629..e7a575398 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1609,6 +1609,15 @@ internal static string PossibleIncorrectUsageOfAssignmentOperatorName { } } + /// + /// Looks up a localized string similar to Did you really mean to use the redirection operator '>'? If you wanted to use an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators.. + /// + internal static string PossibleIncorrectUsageOfFileRedirectionOperatorError { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfFileRedirectionOperatorError", resourceCulture); + } + } + /// /// Looks up a localized string similar to Basic Comment Help. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 5e0e1314e..5b75c703b 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1005,4 +1005,7 @@ AvoidAssignmentToAutomaticVariable + + Did you really mean to use the redirection operator '>'? If you wanted to use an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators. + \ No newline at end of file From 2c0bc282d15db9d33f937b21103aaecaa9c0980d Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 8 Feb 2018 23:19:26 +0000 Subject: [PATCH 02/20] adapt error message strings and add tests --- ...sibleIncorrectUsageOfAssignmentOperator.md | 7 ----- ...sibleIncorrectUsageOfComparisonOperator.md | 7 +++++ ...sibleIncorrectUsageOfAssignmentOperator.cs | 17 +++++----- Rules/Strings.Designer.cs | 31 ++++++++++++------- Rules/Strings.resx | 15 +++++---- ...orrectUsageOfComparisonOperator.tests.ps1} | 31 +++++++++++++++++-- 6 files changed, 73 insertions(+), 35 deletions(-) delete mode 100644 RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md create mode 100644 RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md rename Tests/Rules/{PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 => PossibleIncorrectUsageOfComparisonOperator.tests.ps1} (67%) diff --git a/RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md deleted file mode 100644 index 42ced0406..000000000 --- a/RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md +++ /dev/null @@ -1,7 +0,0 @@ -# PossibleIncorrectUsageOfAssignmentOperator - -**Severity Level: Information** - -## Description - -In many programming languages, the equality operator is denoted as `==` or `=`, but `PowerShell` uses `-eq`. Since assignment inside if statements are very rare, this rule wants to call out this case because it might have been unintentional. diff --git a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md new file mode 100644 index 000000000..300bda8fe --- /dev/null +++ b/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. diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index c3b2d224e..78b20e11c 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -22,12 +22,13 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { /// - /// 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. /// #if !CORECLR [Export(typeof(IScriptRule))] #endif - public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule + public class PossibleIncorrectUsageOfComparisonOperator : 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. @@ -49,7 +50,7 @@ public IEnumerable 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,7 +61,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (commandAst == null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.Extent, GetName(), DiagnosticSeverity.Information, fileName); } } @@ -70,7 +71,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (fileRedirectionAst != null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfFileRedirectionOperatorError, fileRedirectionAst.Extent, // TODO: better error message string and rename rule + Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent, // TODO: better error message string and rename rule GetName(), DiagnosticSeverity.Warning, fileName); } } @@ -83,7 +84,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// The name of this rule public string GetName() { - return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfAssignmentOperatorName); + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfComparisonOperatorName); } /// @@ -92,7 +93,7 @@ public string GetName() /// The common name of this rule public string GetCommonName() { - return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorCommonName); + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorCommonName); } /// @@ -101,7 +102,7 @@ public string GetCommonName() /// The description of this rule public string GetDescription() { - return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingWriteHostDescription); + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorDescription); } /// diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index e7a575398..4fe51b808 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1583,38 +1583,47 @@ internal static string PossibleIncorrectComparisonWithNullName { } /// - /// Looks up a localized string similar to '=' operator means assignment. Did you mean the equal operator '-eq'?. + /// 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 PossibleIncorrectUsageOfAssignmentOperatorCommonName { + internal static string PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorCommonName", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError", resourceCulture); } } /// - /// Looks up a localized string similar to Did you really mean to make an assignment inside an if statement? If you rather meant to check for equality, use the '-eq' operator.. + /// Looks up a localized string similar to '=' operator means assignment. Did you mean the equal operator '-eq'?. /// - internal static string PossibleIncorrectUsageOfAssignmentOperatorError { + internal static string PossibleIncorrectUsageOfComparisonOperatorCommonName { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorError", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorCommonName", resourceCulture); } } /// - /// Looks up a localized string similar to PossibleIncorrectUsageOfAssignmentOperator. + /// 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 PossibleIncorrectUsageOfAssignmentOperatorName { + internal static string PossibleIncorrectUsageOfComparisonOperatorDescription { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorName", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorDescription", resourceCulture); } } /// /// Looks up a localized string similar to Did you really mean to use the redirection operator '>'? If you wanted to use an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators.. /// - internal static string PossibleIncorrectUsageOfFileRedirectionOperatorError { + internal static string PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to PossibleIncorrectUsageOfComparisonOperator. + /// + internal static string PossibleIncorrectUsageOfComparisonOperatorName { get { - return ResourceManager.GetString("PossibleIncorrectUsageOfFileRedirectionOperatorError", resourceCulture); + return ResourceManager.GetString("PossibleIncorrectUsageOfComparisonOperatorName", resourceCulture); } } diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 5b75c703b..0cbd4e3bd 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'? - - Did you really mean to make an assignment inside an if statement? If you rather meant to check for equality, use the '-eq' operator. + + 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. - - PossibleIncorrectUsageOfAssignmentOperator + + PossibleIncorrectUsageOfComparisonOperator Use a different variable name @@ -1005,7 +1005,10 @@ AvoidAssignmentToAutomaticVariable - + + '>', '=' 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 an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators. \ No newline at end of file diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 similarity index 67% rename from Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 rename to Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 index 9c48cb34b..39f41540d 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/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,31 @@ 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" { + $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" { From 29c7e3460c5b375c8c03f8b87a6f6b462980e7a4 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 8 Feb 2018 23:23:31 +0000 Subject: [PATCH 03/20] remove old todo comment --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 78b20e11c..02f5b3a61 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -71,7 +71,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (fileRedirectionAst != null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent, // TODO: better error message string and rename rule + Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent GetName(), DiagnosticSeverity.Warning, fileName); } } From 7310387b920ed0c5e25e52c38be9ffee6fc834d5 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 8 Feb 2018 23:25:07 +0000 Subject: [PATCH 04/20] remove duplicated test case --- .../PossibleIncorrectUsageOfComparisonOperator.tests.ps1 | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 index 39f41540d..d2696f6be 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 @@ -38,11 +38,6 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $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" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a > $b){}' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should Be 1 From 4078e41333719d9a7a3a7e50aff3801cb1abe54d Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 8 Feb 2018 23:26:36 +0000 Subject: [PATCH 05/20] syntax fix from last cleanup commits --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 02f5b3a61..9ffe18116 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -71,7 +71,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (fileRedirectionAst != null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent + Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); } } From b7ad069a18975e526958ec43cc7f95c9868a51bd Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 12 Feb 2018 22:04:57 +0000 Subject: [PATCH 06/20] 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') --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 9ffe18116..a6d04f565 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -50,7 +50,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (assignmentStatementAst.Right.Extent.Text.StartsWith("=")) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.Extent, + Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, GetName(), DiagnosticSeverity.Warning, fileName); } else @@ -61,7 +61,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (commandAst == null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.Extent, + Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, GetName(), DiagnosticSeverity.Information, fileName); } } From 8a48a628d52ea1fee78be3c8551f727eb0c3c5e3 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Feb 2018 21:06:54 +0000 Subject: [PATCH 07/20] 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 --- ...sibleIncorrectUsageOfComparisonOperator.md | 38 +++++++++++++++- ...sibleIncorrectUsageOfAssignmentOperator.cs | 43 +++++++++++++------ Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- ...correctUsageOfComparisonOperator.tests.ps1 | 14 +++--- 4 files changed, 76 insertions(+), 21 deletions(-) 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 } } From fd339f6b122ca53ebcb10e988062974b78675e7e Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 20 Feb 2018 20:30:49 +0000 Subject: [PATCH 08/20] tweak messages and readme --- ...sibleIncorrectUsageOfComparisonOperator.md | 38 ++++++++++++++++++- Rules/Strings.Designer.cs | 2 +- Rules/Strings.resx | 2 +- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md index 300bda8fe..82aa65c47 100644 --- a/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md +++ b/RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md @@ -4,4 +4,40 @@ ## 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. +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/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 4fe51b808..db62a7930 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1610,7 +1610,7 @@ internal static string PossibleIncorrectUsageOfComparisonOperatorDescription { } /// - /// Looks up a localized string similar to Did you really mean to use the redirection operator '>'? If you wanted to use an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators.. + /// 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.. /// internal static string PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 0cbd4e3bd..6e94354e0 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1009,6 +1009,6 @@ '>', '=' 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 an equality comparer then use the '-gt' (greater than) or '-ge' (greater or equal) operators. + 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. \ No newline at end of file From b467d1019e80062abc465fecb12eb86a42da9bff Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 20 Feb 2018 23:00:01 +0000 Subject: [PATCH 09/20] update to pester v4 syntax --- .../PossibleIncorrectUsageOfComparisonOperator.tests.ps1 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 index 39850889e..77b77429a 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 @@ -40,22 +40,22 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { 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 + $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 + $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 + $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 + $warnings.Count | Should -Be 1 } } From 594414faecb89f36bac49b36f7db392617e87862 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 19:32:19 +0000 Subject: [PATCH 10/20] 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 --- ...ssibleIncorrectUsageOAssignmentOperator.md | 54 ++++++++++ ...sibleIncorrectUsageOfComparisonOperator.md | 43 -------- ...ibleIncorrectUsageOfRedirectionOperator.md | 29 ++++++ Rules/ClangSuppresion.cs | 24 +++++ ...sibleIncorrectUsageOfAssignmentOperator.cs | 80 +++++---------- ...ibleIncorrectUsageOfRedirectionOperator.cs | 99 +++++++++++++++++++ Rules/Strings.Designer.cs | 64 +++++++++--- Rules/Strings.resx | 30 ++++-- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 4 +- ...orrectUsageOfAssignmentOperator.tests.ps1} | 45 ++++----- ...orrectUsageOfRedirectionOperator.tests.ps1 | 38 +++++++ 11 files changed, 361 insertions(+), 149 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} (63%) create mode 100644 Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 diff --git a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md new file mode 100644 index 000000000..4dcb13510 --- /dev/null +++ b/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 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 2bd45617c..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; @@ -18,7 +9,6 @@ #endif using System.Management.Automation.Language; using System.Globalization; -using System.Linq; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -27,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,67 +33,45 @@ 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; - if (assignmentStatementAst != null) + var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst; + 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 PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName); + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, 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.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); } } } } - private DiagnosticRecord PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(IScriptExtent errorPosition, string fileName) - { - return new DiagnosticRecord(Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, errorPosition, 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.PossibleIncorrectUsageOfComparisonOperatorName); + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfAssignmentOperatorName); } /// @@ -112,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); } /// @@ -121,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); } /// @@ -138,7 +106,7 @@ public SourceType GetSourceType() /// public RuleSeverity GetSeverity() { - return RuleSeverity.Warning; + return RuleSeverity.Information; } /// 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 63% rename from Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1 rename to Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 77b77429a..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,46 +37,41 @@ 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 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 + Context "When there are no violations" { + 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 "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 "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 "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 + 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 } - } - Context "When there are no violations" { - It "returns no violations when there is no equality operator" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){ }' | 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 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 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 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 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 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 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 } } 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 c4e242e4da209bd0e0a50f3467524e49be8df825 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 22:20:40 +0000 Subject: [PATCH 11/20] Minor fix resource variable naming --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 6 +++--- Rules/Strings.Designer.cs | 9 --------- Rules/Strings.resx | 3 --- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 6e26a6e4f..0e3f62c22 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -17,7 +17,7 @@ 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 PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule { @@ -41,7 +41,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (assignmentStatementAst.Right.Extent.Text.StartsWith("=")) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, GetName(), DiagnosticSeverity.Warning, fileName); } else @@ -56,7 +56,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (commandAst == null && invokeMemberExpressionAst == null && binaryExpressionAst == null) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, GetName(), DiagnosticSeverity.Information, fileName); } } diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 40ea3f156..02359c116 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1582,15 +1582,6 @@ 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 PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError { - get { - return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError", resourceCulture); - } - } - /// /// Looks up a localized string similar to '=' is not an assignment operator. Did you mean the equal operator '-eq'?. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 3b10c071e..2a915faa1 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -984,9 +984,6 @@ '=' 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. - PossibleIncorrectUsageOfAssignmentOperator From f7ce114ddf831b37a4dcd4bff37f8a9d3c92b9bc Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 25 Feb 2018 23:34:57 +0000 Subject: [PATCH 12/20] uncommented accidental comment out of ipmo pssa in tests --- .../Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 | 2 +- .../Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index f01a193bf..85500d9fd 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -1,4 +1,4 @@ -#Import-Module PSScriptAnalyzer +Import-Module PSScriptAnalyzer $ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator" Describe "PossibleIncorrectUsageOfComparisonOperator" { diff --git a/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 index c9c84e0b7..3b1a4791d 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfRedirectionOperator.tests.ps1 @@ -1,4 +1,4 @@ -#Import-Module PSScriptAnalyzer +Import-Module PSScriptAnalyzer $ruleName = "PSPossibleIncorrectUsageOfRedirectionOperator" Describe "PossibleIncorrectUsageOfComparisonOperator" { From bc002138c20c17e052ee1f8023f1ab07dbd3f1b8 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 26 Feb 2018 07:58:16 +0000 Subject: [PATCH 13/20] 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 --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 4 +--- ...ossibleIncorrectUsageOfAssignmentOperator.tests.ps1 | 10 +++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 0e3f62c22..d43565a9f 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -50,10 +50,8 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) 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 (commandAst == null && invokeMemberExpressionAst == null && binaryExpressionAst == null) + if (commandAst == null && invokeMemberExpressionAst == null) { yield return new DiagnosticRecord( Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 85500d9fd..778262c24 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -37,6 +37,11 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == "$b"){}' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 1 } + + It "using an expression like a Binaryexpressionastast on the RHS causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $b -match $c){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 1 + } } Context "When there are no violations" { @@ -60,11 +65,6 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings.Count | Should -Be 0 } - 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 From c6c4f4a945bbc39ccb750c3981fe94d5e15cbcc4 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 26 Feb 2018 16:53:10 +0000 Subject: [PATCH 14/20] update test to test against clang suppression --- .../Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 778262c24..e5f6ff84e 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -51,7 +51,7 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { } It "returns no violations when using implicit clang style suppresion" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ( ($a -eq $b) ){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ( ($a = $b) ){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 0 } From 691f2ce14753aa25cb5240069767d681c487a492 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 26 Feb 2018 19:16:04 +0000 Subject: [PATCH 15/20] make clang suppression work again --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index d43565a9f..dc42244a4 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -34,7 +34,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 && !ClangSuppresion.ScriptExtendIsWrappedInParenthesis(assignmentStatementAst.Extent)) + if (assignmentStatementAst != null && !ClangSuppresion.ScriptExtendIsWrappedInParenthesis(clause.Item1.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 From fe30f4d8242c693802950e120a8ccc4baf2ea951 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 27 Feb 2018 18:47:22 +0000 Subject: [PATCH 16/20] reword warning text to use 'equality operator' instead of 'equals operator' --- Rules/Strings.Designer.cs | 4 ++-- Rules/Strings.resx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 02359c116..4cfa34647 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1583,7 +1583,7 @@ internal static string PossibleIncorrectComparisonWithNullName { } /// - /// Looks up a localized string similar to '=' is not an assignment operator. Did you mean the equal operator '-eq'?. + /// Looks up a localized string similar to '=' is not an assignment operator. Did you mean the equality operator '-eq'?. /// internal static string PossibleIncorrectUsageOfAssignmentOperatorCommonName { get { @@ -1601,7 +1601,7 @@ internal static string PossibleIncorrectUsageOfAssignmentOperatorDescription { } /// - /// Looks up a localized string similar to Did you mean to use the assignment operator '='? The equals operator in PowerShell is 'eq'.. + /// Looks up a localized string similar to Did you mean to use the assignment operator '='? The equality operator in PowerShell is 'eq'.. /// internal static string PossibleIncorrectUsageOfAssignmentOperatorError { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 2a915faa1..3fbdfaab4 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -982,7 +982,7 @@ Assignment statements are not aligned - '=' is not an assignment operator. Did you mean the equal operator '-eq'? + '=' is not an assignment operator. Did you mean the equality operator '-eq'? PossibleIncorrectUsageOfAssignmentOperator @@ -1006,7 +1006,7 @@ '=' or '==' are not comparison operators in the PowerShell language and rarely needed inside if statements. - Did you mean to use the assignment operator '='? The equals operator in PowerShell is 'eq'. + Did you mean to use the assignment operator '='? The equality operator in PowerShell is 'eq'. '>' is not a comparison operator. Use '-gt' (greater than) or '-ge' (greater or equal). From 78ef4bf5bd6254ac34cbaee4a8b6f76f87dc4b64 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Wed, 28 Feb 2018 21:45:43 +0000 Subject: [PATCH 17/20] Always warn when LHS is $null --- ...ssibleIncorrectUsageOAssignmentOperator.md | 2 +- ...sibleIncorrectUsageOfAssignmentOperator.cs | 32 +++++++++++++------ ...correctUsageOfAssignmentOperator.tests.ps1 | 12 ++++++- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md index 4dcb13510..8f5f2cfa7 100644 --- a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md +++ b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md @@ -44,7 +44,7 @@ if ($a = Get-Something) # Only execute action if command returns something and 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: +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'])) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index dc42244a4..2352e6fa0 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -34,7 +34,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 && !ClangSuppresion.ScriptExtendIsWrappedInParenthesis(clause.Item1.Extent)) + 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 @@ -43,21 +43,33 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield return new DiagnosticRecord( Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, GetName(), DiagnosticSeverity.Warning, fileName); + + continue; } - else - { - // 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 (commandAst == null && invokeMemberExpressionAst == null) + // Check if LHS is $null and then always warn + if (assignmentStatementAst.Left is VariableExpressionAst variableExpressionAst) + { + if (variableExpressionAst.VariablePath.UserPath.Equals("null", StringComparison.OrdinalIgnoreCase)) { yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, - GetName(), DiagnosticSeverity.Information, fileName); + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + GetName(), DiagnosticSeverity.Warning, fileName); + continue; } } + + // 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(clause.Item1.Extent); + if (commandAst == null && invokeMemberExpressionAst == null && !doNotWarnBecauseImplicitClangStyleSuppressionWasUsed) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, + GetName(), DiagnosticSeverity.Information, fileName); + } } } } diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index e5f6ff84e..23ef81083 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -1,4 +1,4 @@ -Import-Module PSScriptAnalyzer +#Import-Module PSScriptAnalyzer $ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator" Describe "PossibleIncorrectUsageOfComparisonOperator" { @@ -42,6 +42,16 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $b -match $c){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 1 } + + It "always returns a violations when LHS is `$null even when RHS is a command that would normally not make it trigger" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($null = Get-ChildItem){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 1 + } + + It "always returns a violations when LHS is `$null even when using clang style" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if (($null = $b)){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 1 + } } Context "When there are no violations" { From 675e3eb59eb9a56b722495915f4f46956fe94d38 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 15 Mar 2018 22:57:18 +0000 Subject: [PATCH 18/20] 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 --- ...ssibleIncorrectUsageOAssignmentOperator.md | 2 +- ...sibleIncorrectUsageOfAssignmentOperator.cs | 93 ++++++++++++------- ...correctUsageOfAssignmentOperator.tests.ps1 | 15 +++ 3 files changed, 73 insertions(+), 37 deletions(-) diff --git a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md index 8f5f2cfa7..5089c103a 100644 --- a/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md +++ b/RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md @@ -6,7 +6,7 @@ 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. +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 diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 2352e6fa0..e18bf54f9 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -13,7 +13,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { /// - /// PossibleIncorrectUsageOfAssignmentOperator: Warn if someone uses '>', '=' or '==' operators inside an if or elseif 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. /// #if !CORECLR @@ -22,57 +22,78 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules 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. + /// 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. /// public IEnumerable 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.ErrorPosition, - GetName(), DiagnosticSeverity.Warning, fileName); + yield return diagnosticRecord; + } + } + } + } - continue; - } + 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 LHS is $null and then always warn - if (assignmentStatementAst.Left is VariableExpressionAst variableExpressionAst) - { - if (variableExpressionAst.VariablePath.UserPath.Equals("null", StringComparison.OrdinalIgnoreCase)) - { - yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, - GetName(), DiagnosticSeverity.Warning, fileName); - continue; - } - } + // 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); + } - // 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(clause.Item1.Extent); - if (commandAst == null && invokeMemberExpressionAst == null && !doNotWarnBecauseImplicitClangStyleSuppressionWasUsed) - { - yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.ErrorPosition, - GetName(), DiagnosticSeverity.Information, 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; } /// diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 23ef81083..64db526f3 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -8,6 +8,16 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings.Count | Should -Be 1 } + It "assignment inside while statement causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'while ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 1 + } + + It "assignment inside do-while statement causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'do {} while ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 1 + } + It "assignment inside if statement causes warning when when wrapped in command expression" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=($b)){}' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 1 @@ -84,5 +94,10 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){ }' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should -Be 0 } + + It "returns no violations when using for loop" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'for ($i = 0; $ -lt 42; $i++){ }' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 0 + } } } From ad008cde7cb01bf9314c017c7ce972cbdc87f3bb Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 15 Mar 2018 23:01:02 +0000 Subject: [PATCH 19/20] tweak message to be more generic in terms of the conditional statement --- Rules/Strings.Designer.cs | 2 +- Rules/Strings.resx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 4cfa34647..dd2eda13e 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1592,7 +1592,7 @@ internal static string PossibleIncorrectUsageOfAssignmentOperatorCommonName { } /// - /// 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 conditional statements.. /// internal static string PossibleIncorrectUsageOfAssignmentOperatorDescription { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 3fbdfaab4..c80680770 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1003,7 +1003,7 @@ 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 conditional statements. Did you mean to use the assignment operator '='? The equality operator in PowerShell is 'eq'. From f530d75c2c41ede818d4850207113a1eba8ba79e Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 23 Mar 2018 20:06:29 +0000 Subject: [PATCH 20/20] Address PR comments --- .../PossibleIncorrectUsageOfRedirectionOperator.md | 4 ++-- Rules/ClangSuppresion.cs | 2 +- Rules/PossibleIncorrectUsageOfRedirectionOperator.cs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md index 24b0efebf..6af16c0a7 100644 --- a/RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md +++ b/RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md @@ -6,7 +6,7 @@ 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. +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 @@ -22,7 +22,7 @@ if ($a > $b) ### Correct ```` PowerShell -if ($a -eq $b) +if ($a -gt $b) { ... } diff --git a/Rules/ClangSuppresion.cs b/Rules/ClangSuppresion.cs index 9313413e8..d0fcfec99 100644 --- a/Rules/ClangSuppresion.cs +++ b/Rules/ClangSuppresion.cs @@ -11,7 +11,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer 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 + /// 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 /// /// diff --git a/Rules/PossibleIncorrectUsageOfRedirectionOperator.cs b/Rules/PossibleIncorrectUsageOfRedirectionOperator.cs index ca309a172..9e33f19f2 100644 --- a/Rules/PossibleIncorrectUsageOfRedirectionOperator.cs +++ b/Rules/PossibleIncorrectUsageOfRedirectionOperator.cs @@ -13,7 +13,7 @@ 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. + /// 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. /// #if !CORECLR