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..fe34e6d2a --- /dev/null +++ b/Rules/PossibleIncorrectUsageOfAssignmentOperator.cs @@ -0,0 +1,124 @@ +// +// 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 + { + /// + /// 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); + + 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) + { + // Check if someone used '==', which can easily happen when the person is used to coding a lot in C#. + // In most cases, this will be a runtime error because PowerShell will look for a cmdlet name starting with '=', which is technically possible to define + if (assignmentStatementAst.Right.Extent.Text.StartsWith("=")) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + GetName(), DiagnosticSeverity.Warning, fileName); + } + else + { + // If the right hand side contains a CommandAst at some point, then we do not want to warn + // because this could be intentional in cases like 'if ($a = Get-ChildItem){ }' + var commandAst = assignmentStatementAst.Right.Find(testAst => testAst is CommandAst, searchNestedScriptBlocks: true) as CommandAst; + if (commandAst == null) + { + yield return new DiagnosticRecord( + Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent, + GetName(), DiagnosticSeverity.Information, fileName); + } + } + } + } + } + } + + /// + /// 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/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 new file mode 100644 index 000000000..9c48cb34b --- /dev/null +++ b/Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1 @@ -0,0 +1,63 @@ +Import-Module PSScriptAnalyzer +$ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator" + +Describe "PossibleIncorrectUsageOfAssignmentOperator" { + Context "When there are violations" { + 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 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 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 "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" { + 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 + } + + 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 + } + + 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 + } + + 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 + } + } +}