From 949dc3f8a39c5769dc7e60c2af222d5d41d7c9e5 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Wed, 14 Mar 2018 20:52:15 +0000 Subject: [PATCH 1/3] add warning for += operator on arrays. This covers only some use cases because SSA is not fully implemented (i.e. $a=@();$a+=$foo does not get caught or determining that the type is of type string. --- Engine/Helper.cs | 22 ++++ Engine/VariableAnalysis.cs | 22 ++++ ...voidPlusEqualsOperatorOnArraysOrStrings.cs | 112 ++++++++++++++++++ Rules/Strings.Designer.cs | 36 ++++++ Rules/Strings.resx | 12 ++ Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- ...EqualsOperatorForArraysOrStrings.tests.ps1 | 23 ++++ 7 files changed, 228 insertions(+), 1 deletion(-) create mode 100644 Rules/AvoidPlusEqualsOperatorOnArraysOrStrings.cs create mode 100644 Tests/Rules/AvoidUsingPlusEqualsOperatorForArraysOrStrings.tests.ps1 diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 31120ed99..945b5230d 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -1217,6 +1217,28 @@ public Type GetTypeFromAnalysis(VariableExpressionAst varAst, Ast ast) } } + /// + /// Get the type of varAst from the internal variable analysis. + /// + /// + /// + /// + public Type GetTypeFromInternalVariableAnalysis(VariableExpressionAst varAst, Ast ast) + { + try + { + if (VariableAnalysisDictionary.ContainsKey(ast)) + { + VariableAnalysis VarTypeAnalysis = VariableAnalysisDictionary[ast]; + VariableAnalysisDetails details = VarTypeAnalysis.GetInternalVariableAnalysis(varAst); + return details.Type; + } + } + catch { } + + return null; + } + /// /// Get type of variable from the variable analysis /// diff --git a/Engine/VariableAnalysis.cs b/Engine/VariableAnalysis.cs index 2c988a9c2..0357f9a7f 100644 --- a/Engine/VariableAnalysis.cs +++ b/Engine/VariableAnalysis.cs @@ -338,6 +338,28 @@ public VariableAnalysisDetails GetVariableAnalysis(VariableExpressionAst varTarg return VariablesDictionary[key]; } + /// + /// Get VariableAnalysisDetails of internal variables. + /// + /// + /// + public VariableAnalysisDetails GetInternalVariableAnalysis(VariableExpressionAst varTarget) + { + if (varTarget == null) + { + return null; + } + + string key = varTarget.VariablePath.UserPath; + + if (!InternalVariablesDictionary.ContainsKey(key)) + { + return null; + } + + return InternalVariablesDictionary[key]; + } + internal static string AnalysisDictionaryKey(VariableExpressionAst varExprAst) { if (varExprAst == null) diff --git a/Rules/AvoidPlusEqualsOperatorOnArraysOrStrings.cs b/Rules/AvoidPlusEqualsOperatorOnArraysOrStrings.cs new file mode 100644 index 000000000..4e828112a --- /dev/null +++ b/Rules/AvoidPlusEqualsOperatorOnArraysOrStrings.cs @@ -0,0 +1,112 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// Avoid using += operator on arrays or strings. + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class AvoidPlusEqualsOperatorOnArraysOrStrings : IScriptRule + { + /// + /// Checks for usage of += on variables of type array. + /// The script's ast + /// The script's file name + /// The diagnostic results of this rule + /// + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + IEnumerable assignmentStatementAstsUsingPlusEqualsOperator = ast.FindAll(testAst => testAst is AssignmentStatementAst assignmentStatementAst && + assignmentStatementAst.Operator == TokenKind.PlusEquals, searchNestedScriptBlocks: true); + + foreach (AssignmentStatementAst assignmentStatementAstUsingPlusEqualsOperator in assignmentStatementAstsUsingPlusEqualsOperator) + { + var variableExpressionAst = assignmentStatementAstUsingPlusEqualsOperator.Left as VariableExpressionAst; + if (variableExpressionAst != null) + { + if (variableExpressionAst.StaticType.IsArray) + { + yield return Warning(variableExpressionAst.Extent, fileName); + } + + var type = Helper.Instance.GetTypeFromInternalVariableAnalysis(variableExpressionAst, ast); + if (type != null && type.IsArray) + { + yield return Warning(variableExpressionAst.Extent, fileName); + } + } + } + } + + private DiagnosticRecord Warning(IScriptExtent extent, string fileName) + { + return new DiagnosticRecord(Strings.AvoidPlusEqualsOperatorOnArraysError, 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.AvoidPlusEqualsOperatorOnArraysName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidPlusEqualsOperatorOnArraysCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidPlusEqualsOperatorOnArraysDescription); + } + + /// + /// 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 565e67629..48af53407 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -448,6 +448,42 @@ internal static string AvoidNullOrEmptyHelpMessageAttributeName { } } + /// + /// Looks up a localized string similar to Use other .Net classes such as System.Collections.ArrayList or System.Texst.StringBuilder instead.. + /// + internal static string AvoidPlusEqualsOperatorOnArraysCommonName { + get { + return ResourceManager.GetString("AvoidPlusEqualsOperatorOnArraysCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Using the += operator is discouraged for arrays or strings. The reason is that it can result in bad performance because the underlying type System.Array is of fixed size and needs to be-recreated for each addition. Consider replacing it with other types such as System.Collections.ArrayList or System.Text.StringBuilder, especially in cases where more than 100 additions can happen.. + /// + internal static string AvoidPlusEqualsOperatorOnArraysDescription { + get { + return ResourceManager.GetString("AvoidPlusEqualsOperatorOnArraysDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Avoid using += operator for arrays or strings.. + /// + internal static string AvoidPlusEqualsOperatorOnArraysError { + get { + return ResourceManager.GetString("AvoidPlusEqualsOperatorOnArraysError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidPlusEqualsOperatorOnArraysOrStrings. + /// + internal static string AvoidPlusEqualsOperatorOnArraysName { + get { + return ResourceManager.GetString("AvoidPlusEqualsOperatorOnArraysName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using ShouldContinue Without Boolean Force Parameter. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 5e0e1314e..d2653f1dc 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1005,4 +1005,16 @@ AvoidAssignmentToAutomaticVariable + + Use other .Net classes such as System.Collections.ArrayList or System.Texst.StringBuilder instead. + + + Using the += operator is discouraged for arrays or strings. The reason is that it can result in bad performance because the underlying type System.Array is of fixed size and needs to be-recreated for each addition. Consider replacing it with other types such as System.Collections.ArrayList or System.Text.StringBuilder, especially in cases where more than 100 additions can happen. + + + Avoid using += operator for arrays or strings. + + + AvoidPlusEqualsOperatorOnArraysOrStrings + \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 4877becdc..1390d0ec6 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 diff --git a/Tests/Rules/AvoidUsingPlusEqualsOperatorForArraysOrStrings.tests.ps1 b/Tests/Rules/AvoidUsingPlusEqualsOperatorForArraysOrStrings.tests.ps1 new file mode 100644 index 000000000..cc888999f --- /dev/null +++ b/Tests/Rules/AvoidUsingPlusEqualsOperatorForArraysOrStrings.tests.ps1 @@ -0,0 +1,23 @@ +Import-Module PSScriptAnalyzer +$ruleName = "PSAvoidPlusEqualsOperatorOnArraysOrStrings" + +Describe "AvoidPlusEqualsOperatorOnArraysOrStrings" { + Context "When there are violations" { + It "assignment inside if statement causes warning" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '$array=@(); $list | ForEach-Object { $array += $stuff }' | 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 '$array=@(); $list | Where-Object { if($_ -eq $true){ $array += 4 }}' | 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 '$array = Get-UnknownObject; $array += $stuff' | Where-Object {$_.RuleName -eq $ruleName} + $warnings.Count | Should -Be 0 + } + } + } +} From 8b885573b6440d9d53f5795a9d4155f1ea03fd90 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Wed, 14 Mar 2018 21:16:38 +0000 Subject: [PATCH 2/3] Add documentation and tweak message --- ...voidPlusEqualsOperatorOnArraysOrStrings.md | 56 +++++++++++++++++++ Rules/Strings.Designer.cs | 2 +- Rules/Strings.resx | 2 +- 3 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md diff --git a/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md b/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md new file mode 100644 index 000000000..6f2366db2 --- /dev/null +++ b/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md @@ -0,0 +1,56 @@ +# AvoidPlusEqualsOperatorOnArraysOrStrings + +**Severity Level: Warning** + +## Description + +PowerShell's `+=` operator creates a new array everytime an element gets added to it. This can leads to a noticable performance hit on arrays that have more than 1000 elements and many elements are being added. + +## How + +Using a .Net list or builder type that supports methods for adding elements to it such as e.g. `System.Collections.ArrayList`, `System.Text.StringBuilder` or `[System.Collections.Generic.List[CustomType]]`. + +## Example + +### Wrong + +``` PowerShell +$array = @() +foreach($i in 1..1000) { + $array += Get-SecretSauce $i +} + +``` + +``` PowerShell +$arrayList = New-Object System.Collections.ArrayList +foreach($i in 1..1000) { + $arrayList += Get-SecretSauce $i +} +``` + +``` PowerShell +$message = "Let's start to enumerate a lot:" +foreach($i in 1..1000) { + $message += "$i;" +} +``` + +### Correct + +``` PowerShell +$array = New-Object System.Collections.ArrayList +foreach($i in 1..1000) { + $array.Add(Get-SecretSauce $i) +} +``` + +``` PowerShell +$stringBuilder = New-Object System.Text.StringBuilder +$null = $stringBuilder.Append("Let's start to enumerate a lot:") +foreach($i in 1..1000) { + $message += "$i;" + $null = $stringBuilder.Append("$i;") +} +$message = $stringBuilder.ToString() +``` diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 48af53407..2c6e3bb0c 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -458,7 +458,7 @@ internal static string AvoidPlusEqualsOperatorOnArraysCommonName { } /// - /// Looks up a localized string similar to Using the += operator is discouraged for arrays or strings. The reason is that it can result in bad performance because the underlying type System.Array is of fixed size and needs to be-recreated for each addition. Consider replacing it with other types such as System.Collections.ArrayList or System.Text.StringBuilder, especially in cases where more than 100 additions can happen.. + /// Looks up a localized string similar to Using the += operator is discouraged for arrays or strings. The reason is that it can result in bad performance because the underlying type System.Array is of fixed size and needs to be-recreated for each addition. Consider replacing it with other types such as System.Collections.ArrayList or System.Text.StringBuilder, especially for arrays with more than 1000 elements.. /// internal static string AvoidPlusEqualsOperatorOnArraysDescription { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index d2653f1dc..5c14ceda1 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1009,7 +1009,7 @@ Use other .Net classes such as System.Collections.ArrayList or System.Texst.StringBuilder instead. - Using the += operator is discouraged for arrays or strings. The reason is that it can result in bad performance because the underlying type System.Array is of fixed size and needs to be-recreated for each addition. Consider replacing it with other types such as System.Collections.ArrayList or System.Text.StringBuilder, especially in cases where more than 100 additions can happen. + Using the += operator is discouraged for arrays or strings. The reason is that it can result in bad performance because the underlying type System.Array is of fixed size and needs to be-recreated for each addition. Consider replacing it with other types such as System.Collections.ArrayList or System.Text.StringBuilder, especially for arrays with more than 1000 elements. Avoid using += operator for arrays or strings. From 6b4f0611c90ee3c7753908e5b881f8d09eac41d1 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Wed, 14 Mar 2018 21:38:34 +0000 Subject: [PATCH 3/3] tweak docs --- RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md b/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md index 6f2366db2..d2c31c475 100644 --- a/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md +++ b/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md @@ -4,7 +4,7 @@ ## Description -PowerShell's `+=` operator creates a new array everytime an element gets added to it. This can leads to a noticable performance hit on arrays that have more than 1000 elements and many elements are being added. +PowerShell's `+=` operator creates a new array everytime an element gets added to it. This can leads to a noticable performance hit on arrays that have more than 1000 elements and many elements are being added. Another negative side effect is also that by re-creating an entire new array one loses the reference to the original object. ## How