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..f73b68d1e 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -932,6 +932,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.ps1 b/Tests/Rules/AvoidUsingArrayList.ps1 new file mode 100644 index 000000000..2430af6ed --- /dev/null +++ b/Tests/Rules/AvoidUsingArrayList.ps1 @@ -0,0 +1,19 @@ +using namespace system.collections + +# Using New-Object +$List = New-Object ArrayList +$List = New-Object 'ArrayList' +$List = New-Object "ArrayList" +$List = New-Object -Type ArrayList +$List = New-Object -TypeName ArrayLIST +$List = New-Object Collections.ArrayList +$List = New-Object System.Collections.ArrayList + +# Using type initializer +$List = [ArrayList](1,2,3) +$List = [ArrayLIST]@(1,2,3) +$List = [ArrayList]::new() +$List = [Collections.ArrayList]::New() +$List = [System.Collections.ArrayList]::new() + +1..3 | ForEach-Object { $null = $List.Add($_) } diff --git a/Tests/Rules/AvoidUsingArrayList.tests.ps1 b/Tests/Rules/AvoidUsingArrayList.tests.ps1 new file mode 100644 index 000000000..ad4e0f8c6 --- /dev/null +++ b/Tests/Rules/AvoidUsingArrayList.tests.ps1 @@ -0,0 +1,64 @@ +# 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" { + + BeforeDiscovery { + $violationFileName = "$PSScriptRoot\AvoidUsingArrayList.ps1" + $violationExtents = [Parser]::ParseFile($violationFileName, [ref] $null, [ref] $null).FindAll({ + $Args[0] -is [AssignmentStatementAst] -and + $Args[0].Left.Extent.Text -eq '$List' + }, $false).Right.Extent + } + + Context "When there are violations" { + + BeforeAll { + $violationFileName = "$PSScriptRoot\AvoidUsingArrayList.ps1" + $violations = Invoke-ScriptAnalyzer $violationFileName | Where-Object RuleName -eq $ruleName + $violationLines = @{} + foreach ($violation in $violations) { $violationLines[$violation.Line] = $violation } + } + + It "Should return 12 violations" { + $violations.Count | Should -Be 12 + } + + It "Each violation should contain" -ForEach $violationExtents { + $violation = $violationLines[$_.StartLineNumber] + $violation.Extent.Text | Should -Be $_.Text + $violation.Message | Should -Be ($ruleMessage -f $_.Text) + $violation.Severity | Should -Be Warning + $violation.ScriptName | Should -Be AvoidUsingArrayList.ps1 + } + + + It "Dynamic types shouldn't error" { + # but aren't covered by the rule + + $scriptDefinition = { + $type = "System.Collections.ArrayList" + New-Object -TypeName '$type' + }.ToString() + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + } + + Context "When there are no violations" { + It "returns no violations" { + $noViolations.Count | Should -Be 0 + } + } +} diff --git a/Tests/Rules/AvoidUsingArrayListNoViolations.ps1 b/Tests/Rules/AvoidUsingArrayListNoViolations.ps1 new file mode 100644 index 000000000..469d8214a --- /dev/null +++ b/Tests/Rules/AvoidUsingArrayListNoViolations.ps1 @@ -0,0 +1,19 @@ +using namespace System.Collections.Generic + +# Using a generic List +$List = New-Object List[Object] +1..3 | ForEach-Object { $List.Add($_) } # This will not return anything + +$List = [List[Object]]::new() +1..3 | ForEach-Object { $List.Add($_) } # This will not return anything + +# Creating a fixed array by using the PowerShell pipeline +$List = 1..3 | ForEach-Object { $_ } + +# This should not violate because there isn't a +# `using namespace System.Collections` directive +# and ArrayList could belong to another namespace +$List = New-Object ArrayList +$List = [ArrayList](1,2,3) +$List = [ArrayList]@(1,2,3) +$List = [ArrayList]::new() \ No newline at end of file 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 | |