-
Notifications
You must be signed in to change notification settings - Fork 408
Add new AvoidUsingArrayList rule #2174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c34eb23
ba1167d
77384e1
77d1e6d
d9b88a8
1eb92e9
c0aa82b
7bc3dfa
b35becb
9ed1355
e1362e6
b80f01c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|
||
| /// <summary> | ||
| /// Rule that warns when the ArrayList class is used in a PowerShell script. | ||
| /// </summary> | ||
| public class AvoidUsingArrayListAsFunctionNames : IScriptRule | ||
| { | ||
|
|
||
| /// <summary> | ||
| /// Analyzes the PowerShell AST for uses of the ArrayList class. | ||
| /// </summary> | ||
| /// <param name="ast">The PowerShell Abstract Syntax Tree to analyze.</param> | ||
| /// <param name="fileName">The name of the file being analyzed (for diagnostic reporting).</param> | ||
| /// <returns>A collection of diagnostic records for each violation.</returns> | ||
|
|
||
| public IEnumerable<DiagnosticRecord> 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<Ast> 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.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") && | ||
| arrayListName != null && | ||
| arrayListName.IsMatch(bindingResult.BoundParameters["TypeName"].ConstantValue as string) | ||
| ) | ||
| { | ||
| yield return new DiagnosticRecord( | ||
| string.Format( | ||
| CultureInfo.CurrentCulture, | ||
| Strings.AvoidUsingArrayListError, | ||
| cmd.Extent.Text), | ||
| bindingResult.BoundParameters["TypeName"].Value.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; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| 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($_) } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # Copyright (c) Microsoft Corporation. All rights reserved. | ||
| # Licensed under the MIT License. | ||
|
|
||
| BeforeAll { | ||
| $ruleName = "PSAvoidArrayList" | ||
| $ruleMessage = "The ArrayList class is used in '*'. Consider using a generic collection or a fixed array instead." | ||
| } | ||
|
|
||
| Describe "AvoidArrayList" { | ||
| Context "When there are violations" { | ||
| $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've clearly put a lot of thought into the various edge cases, but there's some issues with how the tests are set up - they don't currently run. Take a look at something like AvoidExclaimOperator.tests.ps1 as an example of how to structure the tests. With the current approach, a file of lots of violations and another with none, you're really only writing 2 tests. If something changes in the future that breaks your rule, CI will just tell you that one (or both) of those tests no longer passes. e.g. That you got 11 violations instead of 12. Some troubleshooting would then be needed to work out what case no longer works. I'd really recommend writing more scoped tests. Describe "AvoidArrayList" {
Context "When using New-Object with ArrayList passed to TypeName" {
It "Should find a violation" {
$def = '$List = New-Object -TypeName ArrayLIST'
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $def
$violations.Count | Should -Be 1
}
}
}LLMs are fairly good at writing them - they just need the right input and examples. |
||
| It "has ArrayList violations" { | ||
| $violations.Count | Should -Be 12 | ||
| } | ||
|
|
||
| It "has the correct description message" { | ||
| $violations[0].Message | Should -Like $ruleMessage | ||
| } | ||
| } | ||
|
|
||
| Context "When there are no violations" { | ||
| It "returns no violations" { | ||
| $noViolations.Count | Should -Be 0 | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { $_ } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that this rule should be enabled by default
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that in a lot of cases the use of the |
||
| | [AvoidUsingBrokenHashAlgorithms](./AvoidUsingBrokenHashAlgorithms.md) | Warning | Yes | | | ||
| | [AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes | Yes<sup>2</sup> | | ||
| | [AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | Yes | | | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class name has a little copy-pasta 😊
Also, I think this should be a configurable rule, disabled by default.
Instead of directly implementing
IScriptRule, implementConfigurableRule. See AvoidExclaimOperator as an example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I change this, I notice that there are quite some inherited methods to be implemented.
Before going into this direction, can you argue why you think this rule should be disabled by default?
Or even configurable?