From 413e6c0f5189844ad256eca5bf42a6f46640e6ef Mon Sep 17 00:00:00 2001 From: Chris Bergmeister Date: Sun, 28 Jan 2018 22:56:33 +0000 Subject: [PATCH 1/7] first try at implementation rule. basically works but also catches case where the execution block of the if statement contains an assignment --- Engine/Generic/IScriptRule.cs | 4 - ...sibleIncorrectUsageOfAssignmentOperator.md | 7 ++ ...sibleIncorrectUsageOfAssignmentOperator.cs | 109 ++++++++++++++++++ Rules/Strings.Designer.cs | 35 +++++- Rules/Strings.resx | 66 ++++++----- ...correctUsageOfAssignmentOperator.tests.ps1 | 33 ++++++ 6 files changed, 217 insertions(+), 37 deletions(-) create mode 100644 RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md create mode 100644 Rules/PossibleIncorrectUsageOfAssignmentOperator.cs create mode 100644 Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 diff --git a/Engine/Generic/IScriptRule.cs b/Engine/Generic/IScriptRule.cs index df7de53fa..b081296ae 100644 --- a/Engine/Generic/IScriptRule.cs +++ b/Engine/Generic/IScriptRule.cs @@ -10,11 +10,7 @@ // THE SOFTWARE. // -using System; using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; using System.Management.Automation.Language; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic diff --git a/RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md b/RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md new file mode 100644 index 000000000..42ced0406 --- /dev/null +++ b/RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md @@ -0,0 +1,7 @@ +# 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/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs new file mode 100644 index 000000000..133e4fd53 --- /dev/null +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -0,0 +1,109 @@ +// +// 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. +// + +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 +{ + /// + /// PossibleIncorrectUsageOfAssignmentOperator: Warn if someone uses the '=' or '==' by accident in an if statement because in most cases that is not the intention. + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule + { + List records; + string fileName; + + /// + /// AnalyzeScript: + /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements. + /// This is just a simple smoke check to catch cases like 'if (($a=$b)){}' but was not made too complex avoid false positives. + /// It does not catch cases with expressions inside the if staement like e.g. 'if (($a=$b)){}' + /// + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + IEnumerable ifStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); + foreach (var aast in ifStatementAsts) + { + if (aast.Parent is IfStatementAst) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, aast.Extent, + GetName(), DiagnosticSeverity.Information, 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.PossibleIncorrectUsageOfAssignmentOperatorName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingWriteHostDescription); + } + + /// + /// 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.Information; + } + + /// + /// 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 1d6f12a04..9905be482 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -11,8 +11,8 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { using System; using System.Reflection; - - + + /// /// A strongly-typed resource class, for looking up localized strings, etc. /// @@ -1537,6 +1537,33 @@ internal static string PossibleIncorrectComparisonWithNullName { } } + /// + /// Looks up a localized string similar to '=' operator means assignment. Did you mean the equal operator '-eq'?. + /// + internal static string PossibleIncorrectUsageOfAssignmentOperatorCommonName { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorCommonName", 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.. + /// + internal static string PossibleIncorrectUsageOfAssignmentOperatorError { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to PossibleIncorrectUsageOfAssignmentOperator. + /// + internal static string PossibleIncorrectUsageOfAssignmentOperatorName { + get { + return ResourceManager.GetString("PossibleIncorrectUsageOfAssignmentOperatorName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Basic Comment Help. /// @@ -2366,7 +2393,7 @@ internal static string UseShouldProcessForStateChangingFunctionsDescrption { } /// - /// Looks up a localized string similar to Function '{0}' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.. + /// Looks up a localized string similar to Function '{0}' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.. /// internal static string UseShouldProcessForStateChangingFunctionsError { get { @@ -2636,7 +2663,7 @@ internal static string UseVerboseMessageInDSCResourceDescription { } /// - /// Looks up a localized string similar to There is no call to Write-Verbose in DSC function '{0}'. If you are using Write-Verbose in a helper function, suppress this rule application.. + /// Looks up a localized string similar to There is no call to Write-Verbose in DSC function '{0}'. If you are using Write-Verbose in a helper function, suppress this rule application.. /// internal static string UseVerboseMessageInDSCResourceErrorFunction { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index aa12ff43f..69de3a14f 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1,17 +1,17 @@  - @@ -957,7 +957,6 @@ Use space after a semicolon. - UseSupportsShouldProcess @@ -982,4 +981,13 @@ 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. + + + PossibleIncorrectUsageOfAssignmentOperator + + \ No newline at end of file diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 new file mode 100644 index 000000000..30c921512 --- /dev/null +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -0,0 +1,33 @@ +#Import-Module PSScriptAnalyzer +$ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator" + +Describe "PossibleIncorrectUsageOfAssignmentOperator" { + Context "When there are violations" { + It "assignment inside if statemenet causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 1 + } + + It "assignment inside elseif statemenet causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){}elseif($a = $b){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 1 + } + + It "assignment inside else statemenet causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){}else{$a = $b}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 1 + } + + It "double equals inside if statemenet causes warning" { + $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 there is no equality operator" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){$a=$b}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 0 + } + } +} \ No newline at end of file From e452a99dbed79778d46f39a0d568cd074a11ca37 Mon Sep 17 00:00:00 2001 From: Chris Bergmeister Date: Sun, 28 Jan 2018 23:09:22 +0000 Subject: [PATCH 2/7] remove unused variables and fix test to import pssa and fix some rule counter tests --- Rules/PossibleIncorrectUsageOfAssignmentOperator.cs | 3 --- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 4 ++-- .../PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 133e4fd53..13c290070 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -29,9 +29,6 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule { - List records; - string fileName; - /// /// AnalyzeScript: /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements. diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 28980e462..0ca96f243 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 = 52 + $expectedNumRules = 53 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/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 30c921512..104113290 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 "PossibleIncorrectUsageOfAssignmentOperator" { From 647a6a6244c11197c20a69d2cb786bf54962c7d2 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 1 Feb 2018 07:14:04 +0000 Subject: [PATCH 3/7] Improve rule to only assert against clause in if statement. TODO: Make sure that the following case does not get flagged up (added a test case for it) 'if ( $files = get-childitem ) { }' --- ...ssibleIncorrectUsageOfAssignmentOperator.cs | 18 ++++++++++-------- ...ncorrectUsageOfAssignmentOperator.tests.ps1 | 5 +++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 13c290070..c6fcba612 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -32,21 +32,23 @@ public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRul /// /// AnalyzeScript: /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements. - /// This is just a simple smoke check to catch cases like 'if (($a=$b)){}' but was not made too complex avoid false positives. - /// It does not catch cases with expressions inside the if staement like e.g. 'if (($a=$b)){}' /// public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - IEnumerable ifStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); - foreach (var aast in ifStatementAsts) + IEnumerable ifStatementAsts = ast.FindAll(testAst => testAst is IfStatementAst, searchNestedScriptBlocks: true); + foreach (IfStatementAst ifStatememntAst in ifStatementAsts) { - if (aast.Parent is IfStatementAst) + foreach (var clause in ifStatememntAst.Clauses) { - yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, aast.Extent, - GetName(), DiagnosticSeverity.Information, fileName); + var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false); + if (assignmentStatementAst != null) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + GetName(), DiagnosticSeverity.Information, fileName); + } } } } diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 104113290..913ac745b 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -29,5 +29,10 @@ Describe "PossibleIncorrectUsageOfAssignmentOperator" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){$a=$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} + $warnings.Count | Should Be 0 + } } } \ No newline at end of file From 93660bd8a0fe2513fb8fed365ba277203da58a9c Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Thu, 1 Feb 2018 23:24:15 +0000 Subject: [PATCH 4/7] Improve rule to reduce false warnings and adds lots more tests. The only false positive that is left is 'if ($a = (Get-ChildItem)){}' where the RHS is wrapped in an exprssion --- ...sibleIncorrectUsageOfAssignmentOperator.cs | 12 +++---- ...correctUsageOfAssignmentOperator.tests.ps1 | 34 +++++++++++++++---- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index c6fcba612..10b5d593f 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -30,20 +30,20 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule { /// - /// AnalyzeScript: /// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements. /// public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - IEnumerable ifStatementAsts = ast.FindAll(testAst => testAst is IfStatementAst, searchNestedScriptBlocks: true); - foreach (IfStatementAst ifStatememntAst in ifStatementAsts) + var ifStatementAsts = ast.FindAll(testAst => testAst is IfStatementAst, searchNestedScriptBlocks: true); + foreach (IfStatementAst ifStatementAst in ifStatementAsts) { - foreach (var clause in ifStatememntAst.Clauses) + foreach (var clause in ifStatementAst.Clauses) { - var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false); - if (assignmentStatementAst != null) + var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst; + // if the right hand side is an CommandExpressionAst, then it is going to be a variable assignment. Otherwise also check if someone used '==' + if (assignmentStatementAst != null && (assignmentStatementAst.Right is CommandExpressionAst || assignmentStatementAst.Right.Extent.Text.StartsWith("="))) { yield return new DiagnosticRecord( Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 913ac745b..b090b5f66 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -1,27 +1,42 @@ -Import-Module PSScriptAnalyzer +#Import-Module PSScriptAnalyzer $ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator" Describe "PossibleIncorrectUsageOfAssignmentOperator" { Context "When there are violations" { - It "assignment inside if statemenet causes warning" { + It "assignment inside if statement causes warning" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should Be 1 } - It "assignment inside elseif statemenet causes warning" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){}elseif($a = $b){}' | Where-Object {$_.RuleName -eq $ruleName} + 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 } - It "assignment inside else statemenet causes warning" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){}else{$a = $b}' | Where-Object {$_.RuleName -eq $ruleName} + It "assignment 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 "double equals inside if statemenet causes warning" { + It "assignment 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 "double equals inside if statement causes warning" { $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == $b){}' | Where-Object {$_.RuleName -eq $ruleName} $warnings.Count | Should Be 1 } + + It "double equals inside if statement causes warning when wrapping it in command expresion" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == ($b)){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 1 + } + + It "double equals 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 + } } Context "When there are no violations" { @@ -34,5 +49,10 @@ Describe "PossibleIncorrectUsageOfAssignmentOperator" { $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" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 0 + } } } \ No newline at end of file From 3a35ba36464dc7d68d66a21926ef10a4d9e55256 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 10:31:44 +0000 Subject: [PATCH 5/7] fix false positive when the command is wrapped in an expression --- ...sibleIncorrectUsageOfAssignmentOperator.cs | 29 +++++++++++++++---- ...correctUsageOfAssignmentOperator.tests.ps1 | 5 ++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 10b5d593f..37fd1b2e5 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -42,12 +42,31 @@ 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 the right hand side is an CommandExpressionAst, then it is going to be a variable assignment. Otherwise also check if someone used '==' - if (assignmentStatementAst != null && (assignmentStatementAst.Right is CommandExpressionAst || assignmentStatementAst.Right.Extent.Text.StartsWith("="))) + if (assignmentStatementAst != null) { - yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, - GetName(), DiagnosticSeverity.Information, fileName); + // if the right hand side is an CommandExpressionAst, then it is going to be a variable assignment. Otherwise also check if someone used '==' + // Check if someone used '==', which can easily happen when the person is used to coding a lot in C# + if (assignmentStatementAst.Right.Extent.Text.StartsWith("=")) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + GetName(), DiagnosticSeverity.Information, fileName); + } + else + { + // If the right hand side is an CommandExpressionAst, then it is going to be either a variable expression in some way or a command + var rightHandSideCommandExpressioAst = assignmentStatementAst.Right as CommandExpressionAst; + if (rightHandSideCommandExpressioAst != null) + { + var commandAst = rightHandSideCommandExpressioAst.Find(testAst => testAst is CommandAst, searchNestedScriptBlocks: true) as CommandAst; + if (commandAst == null) // allow CommandAst for cases like 'if ($a = Get-ChildItem){ }' + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + GetName(), DiagnosticSeverity.Information, fileName); + } + } + } } } } diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index b090b5f66..3cca42ec1 100644 --- a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -54,5 +54,10 @@ Describe "PossibleIncorrectUsageOfAssignmentOperator" { $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 and also includes a variable" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem $b)){}' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should Be 0 + } } } \ No newline at end of file From 79fa3696828706b963934d145b369cf25a09286f Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 11:18:19 +0000 Subject: [PATCH 6/7] simplify and cleanup --- ...sibleIncorrectUsageOfAssignmentOperator.cs | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs index 37fd1b2e5..fe34e6d2a 100644 --- a/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -44,27 +44,24 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst; if (assignmentStatementAst != null) { - // if the right hand side is an CommandExpressionAst, then it is going to be a variable assignment. Otherwise also check if someone used '==' - // Check if someone used '==', which can easily happen when the person is used to coding a lot in C# + // Check if someone used '==', which can easily happen when the person is used to coding a lot in C#. + // In most cases, this will be a runtime error because PowerShell will look for a cmdlet name starting with '=', which is technically possible to define if (assignmentStatementAst.Right.Extent.Text.StartsWith("=")) { yield return new DiagnosticRecord( Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, - GetName(), DiagnosticSeverity.Information, fileName); + GetName(), DiagnosticSeverity.Warning, fileName); } else { - // If the right hand side is an CommandExpressionAst, then it is going to be either a variable expression in some way or a command - var rightHandSideCommandExpressioAst = assignmentStatementAst.Right as CommandExpressionAst; - if (rightHandSideCommandExpressioAst != null) + // 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 commandAst = rightHandSideCommandExpressioAst.Find(testAst => testAst is CommandAst, searchNestedScriptBlocks: true) as CommandAst; - if (commandAst == null) // allow CommandAst for cases like 'if ($a = Get-ChildItem){ }' - { - yield return new DiagnosticRecord( - Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, - GetName(), DiagnosticSeverity.Information, fileName); - } + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + GetName(), DiagnosticSeverity.Information, fileName); } } } From 402935989549f2056859e00f3e47f6907ae91fb2 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 17:15:06 +0000 Subject: [PATCH 7/7] Uncomment import of PSSA in test to be consistent with other tests --- .../PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 index 3cca42ec1..9c48cb34b 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 "PossibleIncorrectUsageOfAssignmentOperator" { @@ -60,4 +60,4 @@ Describe "PossibleIncorrectUsageOfAssignmentOperator" { $warnings.Count | Should Be 0 } } -} \ No newline at end of file +}