diff --git a/Rules/AvoidUsingArrayList.cs b/Rules/AvoidUsingArrayList.cs new file mode 100644 index 000000000..092f14b14 --- /dev/null +++ b/Rules/AvoidUsingArrayList.cs @@ -0,0 +1,143 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Management.Automation.Language; +using System.Text.RegularExpressions; + +#if !CORECLR +using System.ComponentModel.Composition; +#endif + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + + /// + /// Rule that warns when the ArrayList class is used in a PowerShell script. + /// + public class AvoidUsingArrayListAsFunctionNames : IScriptRule + { + + /// + /// Analyzes the PowerShell AST for uses of the ArrayList class. + /// + /// The PowerShell Abstract Syntax Tree to analyze. + /// The name of the file being analyzed (for diagnostic reporting). + /// A collection of diagnostic records for each violation. + + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) { throw new ArgumentNullException(Strings.NullAstErrorMessage); } + + // If there is an using statement for the Collections namespace, check for the full typename. + // Otherwise also check for the bare ArrayList name. + Regex arrayListName = null; + var sbAst = ast as ScriptBlockAst; + foreach (UsingStatementAst usingAst in sbAst.UsingStatements) + { + if ( + usingAst.UsingStatementKind == UsingStatementKind.Namespace && + ( + usingAst.Name.Value.Equals("Collections", StringComparison.OrdinalIgnoreCase) || + usingAst.Name.Value.Equals("System.Collections", StringComparison.OrdinalIgnoreCase) + ) + ) + { + arrayListName = new Regex(@"^((System\.)?Collections\.)?ArrayList$", RegexOptions.IgnoreCase); + break; + } + } + if (arrayListName == null) { arrayListName = new Regex(@"^(System\.)?Collections\.ArrayList$", RegexOptions.IgnoreCase); } + + // Find all type initializers that create a new instance of the ArrayList class. + IEnumerable typeAsts = ast.FindAll(testAst => + ( + testAst is ConvertExpressionAst convertAst && + convertAst.StaticType != null && + convertAst.StaticType.FullName == "System.Collections.ArrayList" + ) || + ( + testAst is TypeExpressionAst typeAst && + typeAst.TypeName != null && + arrayListName.IsMatch(typeAst.TypeName.Name) && + typeAst.Parent is InvokeMemberExpressionAst parentAst && + parentAst.Member != null && + parentAst.Member is StringConstantExpressionAst memberAst && + memberAst.Value.Equals("new", StringComparison.OrdinalIgnoreCase) + ), + true + ); + + foreach (Ast typeAst in typeAsts) + { + yield return new DiagnosticRecord( + string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidUsingArrayListError, + typeAst.Parent.Extent.Text), + typeAst.Parent.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName + ); + } + + // Find all New-Object cmdlets that create a new instance of the ArrayList class. + var newObjectCommands = ast.FindAll(testAst => + testAst is CommandAst cmdAst && + cmdAst.GetCommandName() != null && + cmdAst.GetCommandName().Equals("New-Object", StringComparison.OrdinalIgnoreCase), + true); + + foreach (CommandAst cmd in newObjectCommands) + { + // Use StaticParameterBinder to reliably get parameter values + var bindingResult = StaticParameterBinder.BindCommand(cmd, true); + + // Check for -TypeName parameter + if ( + bindingResult.BoundParameters.ContainsKey("TypeName") && + bindingResult.BoundParameters["TypeName"] != null && + arrayListName.IsMatch(bindingResult.BoundParameters["TypeName"].ConstantValue as string) + ) + { + yield return new DiagnosticRecord( + string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidUsingArrayListError, + cmd.Extent.Text), + cmd.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName + ); + } + + } + + + } + + public string GetCommonName() => Strings.AvoidUsingArrayListCommonName; + + public string GetDescription() => Strings.AvoidUsingArrayListDescription; + + public string GetName() => string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidUsingArrayListName); + + public RuleSeverity GetSeverity() => RuleSeverity.Warning; + + public string GetSourceName() => Strings.SourceName; + + public SourceType GetSourceType() => SourceType.Builtin; + } +} \ No newline at end of file diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 2a04fd759..ebbf8fe80 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -933,6 +933,18 @@ Line ends with a semicolon + + Avoid using the ArrayList class + + + Avoid using the ArrayList class in PowerShell scripts. Consider using generic collections or fixed arrays instead. + + + AvoidUsingArrayList + + + The ArrayList class is used in '{0}'. Consider using a generic collection or a fixed array instead. + PlaceOpenBrace diff --git a/Tests/Rules/AvoidUsingArrayList.tests.ps1 b/Tests/Rules/AvoidUsingArrayList.tests.ps1 new file mode 100644 index 000000000..46c7919b6 --- /dev/null +++ b/Tests/Rules/AvoidUsingArrayList.tests.ps1 @@ -0,0 +1,223 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +using namespace System.Management.Automation.Language + +[Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'False positive')] +param() + +BeforeAll { + $ruleName = "PSAvoidUsingArrayList" + $ruleMessage = "The ArrayList class is used in '{0}'. Consider using a generic collection or a fixed array instead." +} + +Describe "AvoidArrayList" { + + Context "When there are violations" { + + BeforeAll { + $usingCollections = 'using namespace system.collections' + [Environment]::NewLine + } + + It "Unquoted New-Object type" { + $scriptDefinition = $usingCollections + { + $List = New-Object ArrayList + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object ArrayList}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object ArrayList}) + } + + It "Single quoted New-Object type" { + $scriptDefinition = $usingCollections + { + $List = New-Object 'ArrayList' + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object 'ArrayList'}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object 'ArrayList'}) + } + + It "Double quoted New-Object type" { + $scriptDefinition = $usingCollections + { + $List = New-Object "ArrayList" + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object "ArrayList"}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object "ArrayList"}) + } + + It "New-Object with full parameter name" { + $scriptDefinition = $usingCollections + { + $List = New-Object -TypeName ArrayList + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object -TypeName ArrayList}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object -TypeName ArrayList}) + } + + It "New-Object with abbreviated parameter name and odd casing" { + $scriptDefinition = $usingCollections + { + $List = New-Object -Type ArrayLIST + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object -Type ArrayLIST}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object -Type ArrayLIST}) + } + + It "New-Object with full type name" { + $scriptDefinition = $usingCollections + { + $List = New-Object -TypeName System.Collections.ArrayList + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object -TypeName System.Collections.ArrayList}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object -TypeName System.Collections.ArrayList}) + } + + It "New-Object with semi full type name and odd casing" { + $scriptDefinition = $usingCollections + { + $List = New-Object COLLECTIONS.ArrayList + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Object COLLECTIONS.ArrayList}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {New-Object COLLECTIONS.ArrayList}) + } + + It "Type initializer with 3 parameters" { + $scriptDefinition = $usingCollections + { + $List = [ArrayList](1,2,3) + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[ArrayList](1,2,3)}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {[ArrayList](1,2,3)}) + } + + It "Type initializer with array parameters" { + $scriptDefinition = $usingCollections + { + $List = [ArrayList]@(1,2,3) + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[ArrayList]@(1,2,3)}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {[ArrayList]@(1,2,3)}) + } + + It "Type initializer with new constructor" { + $scriptDefinition = $usingCollections + { + $List = [ArrayList]::new() + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[ArrayList]::new()}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {[ArrayList]::new()}) + } + + It "Full type name initializer with new constructor" { + $scriptDefinition = $usingCollections + { + $List = [System.Collections.ArrayList]::new() + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[System.Collections.ArrayList]::new()}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {[System.Collections.ArrayList]::new()}) + } + + It "Semi full type name initializer with new constructor and odd casing" { + $scriptDefinition = $usingCollections + { + $List = [COLLECTIONS.ArrayList]::new() + 1..3 | ForEach-Object { $null = $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[COLLECTIONS.ArrayList]::new()}.ToString() + $violations.Message | Should -Be ($ruleMessage -f {[COLLECTIONS.ArrayList]::new()}) + } + } + + Context "When there are no violations" { + + BeforeAll { + $usingGeneric = 'using namespace System.Collections.Generic' + [Environment]::NewLine + } + + It "New-Object List[Object]" { + $scriptDefinition = { + $List = New-Object List[Object] + 1..3 | ForEach-Object { $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + + It "[List[Object]]::new()" { + $scriptDefinition = { + $List = [List[Object]]::new() + 1..3 | ForEach-Object { $List.Add($_) } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + + It "Using the pipeline" { + $scriptDefinition = { + $List = 1..3 | ForEach-Object { $_ } + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + + It "Out of the namespace scope" { + $scriptDefinition = $usingGeneric + { + $List = New-Object ArrayList + $List = [ArrayList](1,2,3) + $List = [ArrayList]@(1,2,3) + $List = [ArrayList]::new() + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + } + + Context "Test for potential errors" { + + It "Dynamic types shouldn't error" { + $scriptDefinition = { + $type = "System.Collections.ArrayList" + New-Object -TypeName '$type' + }.ToString() + + $analyzer = { Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) } + $analyzer | Should -Not -Throw # but won't violate either (too complex to cover) + } + } +} diff --git a/docs/Rules/AvoidUsingArrayList.md b/docs/Rules/AvoidUsingArrayList.md new file mode 100644 index 000000000..b56d47424 --- /dev/null +++ b/docs/Rules/AvoidUsingArrayList.md @@ -0,0 +1,52 @@ +--- +description: Avoid using ArrayList +ms.date: 04/16/2025 +ms.topic: reference +title: AvoidUsingArrayList +--- +# AvoidUsingArrayList + +**Severity Level: Warning** + +## Description + +Per dotnet best practices, the +[`ArrayList` class](https://learn.microsoft.com/dotnet/api/system.collections.arraylist) +is not recommended for new development, the same recommendation applies to PowerShell: + +Avoid the ArrayList class for new development. +The `ArrayList` class is a non-generic collection that can hold objects of any type. This is inline with the fact +that PowerShell is a weakly typed language. However, the `ArrayList` class does not provide any explicit type +safety and performance benefits of generic collections. Instead of using an `ArrayList`, consider using either a +[`System.Collections.Generic.List[Object]`](https://learn.microsoft.com/dotnet/api/system.collections.generic.list-1) +class or a fixed PowerShell array. +Besides, the `ArrayList.Add` method returns the index of the added element which often unintentionally pollutes the +PowerShell pipeline and therefore might cause unexpected issues. + +## How to Fix + +In cases where only the `Add` method is used, you might just replace the `ArrayList` class with a generic +`List[Object]` class but you could also consider using the idiomatic PowerShell pipeline syntax instead. + +## Example + +### Wrong + +```powershell +# Using an ArrayList +$List = [System.Collections.ArrayList]::new() +1..3 | ForEach-Object { $List.Add($_) } # Note that this will return the index of the added element +``` + +### Correct + +```powershell +# Using a generic List +$List = [System.Collections.Generic.List[Object]]::new() +1..3 | ForEach-Object { $List.Add($_) } # This will not return anything +``` + +```PowerShell +# Creating a fixed array by using the PowerShell pipeline +$List = 1..3 | ForEach-Object { $_ } +``` \ No newline at end of file diff --git a/docs/Rules/README.md b/docs/Rules/README.md index fca031e33..40ef2958d 100644 --- a/docs/Rules/README.md +++ b/docs/Rules/README.md @@ -28,6 +28,7 @@ The PSScriptAnalyzer contains the following rule definitions. | [AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | Yes | | | [AvoidTrailingWhitespace](./AvoidTrailingWhitespace.md) | Warning | Yes | | | [AvoidUsingAllowUnencryptedAuthentication](./AvoidUsingAllowUnencryptedAuthentication.md) | Warning | Yes | | +| [AvoidUsingArrayList](./AvoidUsingArrayList.md) | Warning | Yes | | | [AvoidUsingBrokenHashAlgorithms](./AvoidUsingBrokenHashAlgorithms.md) | Warning | Yes | | | [AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes | Yes2 | | [AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | Yes | |