WIP Warn when using += on array objects

Engine/Helper.cs

@@ -1208,6 +1208,28 @@ public Type GetTypeFromAnalysis(VariableExpressionAst varAst, Ast ast)
             }
         }
 
+        /// <summary>
+        /// Get the type of varAst from the internal variable analysis.
+        /// </summary>
+        /// <param name="varAst"></param>
+        /// <param name="ast"></param>
+        /// <returns></returns>
+        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;
+        }
+
         /// <summary>
         /// Get type of variable from the variable analysis
         /// </summary>

Engine/VariableAnalysis.cs

@@ -329,6 +329,28 @@ public VariableAnalysisDetails GetVariableAnalysis(VariableExpressionAst varTarg
             return VariablesDictionary[key];
         }
 
+        /// <summary>
+        /// Get VariableAnalysisDetails of internal variables.
+        /// </summary>
+        /// <param name="varTarget"></param>
+        /// <returns></returns>
+        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)

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()
+```

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
+{
+    /// <summary>
+    /// Avoid using += operator on arrays or strings.
+    /// </summary>
+#if !CORECLR
+[Export(typeof(IScriptRule))]
+#endif
+    public class AvoidPlusEqualsOperatorOnArraysOrStrings : IScriptRule
+    {
+        /// <summary>
+        /// Checks for usage of += on variables of type array.
+        /// <param name="ast">The script's ast</param>
+        /// <param name="fileName">The script's file name</param>
+        /// <returns>The diagnostic results of this rule</returns>
+        /// </summary>
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
+            IEnumerable<Ast> 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);
+        }
+
+        /// <summary>
+        /// GetName: Retrieves the name of this rule.
+        /// </summary>
+        /// <returns>The name of this rule</returns>
+        public string GetName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidPlusEqualsOperatorOnArraysName);
+        }
+
+        /// <summary>
+        /// GetCommonName: Retrieves the common name of this rule.
+        /// </summary>
+        /// <returns>The common name of this rule</returns>
+        public string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidPlusEqualsOperatorOnArraysCommonName);
+        }
+
+        /// <summary>
+        /// GetDescription: Retrieves the description of this rule.
+        /// </summary>
+        /// <returns>The description of this rule</returns>
+        public string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidPlusEqualsOperatorOnArraysDescription);
+        }
+
+        /// <summary>
+        /// GetSourceType: Retrieves the type of the rule: builtin, managed or module.
+        /// </summary>
+        public SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        /// <summary>
+        /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
+        /// </summary>
+        /// <returns></returns>
+        public RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        /// <summary>
+        /// GetSourceName: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+}

Rules/Strings.resx

@@ -1056,4 +1056,16 @@
   <data name="UseConsistentWhitespaceErrorSpaceBeforePipe" xml:space="preserve">
     <value>Use space before pipe.</value>
   </data>
+  <data name="AvoidPlusEqualsOperatorOnArraysCommonName" xml:space="preserve">
+    <value>Use other .Net classes such as System.Collections.ArrayList or System.Texst.StringBuilder instead.</value>
+  </data>
+  <data name="AvoidPlusEqualsOperatorOnArraysDescription" xml:space="preserve">
+    <value>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.</value>
+  </data>
+  <data name="AvoidPlusEqualsOperatorOnArraysError" xml:space="preserve">
+    <value>Avoid using += operator for arrays or strings.</value>
+  </data>
+  <data name="AvoidPlusEqualsOperatorOnArraysName" xml:space="preserve">
+    <value>AvoidPlusEqualsOperatorOnArraysOrStrings</value>
+  </data>
 </root>

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
+            }
+        }
+    }
+}