diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 03d9b3caa..256984b13 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -1208,6 +1208,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 fd66ea2c4..ba97ea90d 100644 --- a/Engine/VariableAnalysis.cs +++ b/Engine/VariableAnalysis.cs @@ -329,6 +329,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/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md b/RuleDocumentation/AvoidPlusEqualsOperatorOnArraysOrStrings.md new file mode 100644 index 000000000..d2c31c475 --- /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. Another negative side effect is also that by re-creating an entire new array one loses the reference to the original object. + +## 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/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 77fb2bb22..737cb7b52 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -456,6 +456,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 for arrays with more than 1000 elements.. + /// + 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 66e1b086b..93d17089e 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1056,4 +1056,16 @@ Use space before pipe. + + 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 for arrays with more than 1000 elements. + + + Avoid using += operator for arrays or strings. + + + AvoidPlusEqualsOperatorOnArraysOrStrings + \ No newline at end of file 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 + } + } + } +}