Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,8 @@
"**/bower_components": true,
"/PSCompatibilityCollector/profiles": true,
"/PSCompatibilityCollector/optional_profiles": true
}
},
"cSpell.words": [
Comment thread
iRon7 marked this conversation as resolved.
Outdated
"CORECLR"
]
}
144 changes: 144 additions & 0 deletions Rules/AvoidUsingArrayList.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
// 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;
using System.ComponentModel;
Comment thread
iRon7 marked this conversation as resolved.
Outdated


#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
Copy link
Copy Markdown
Contributor

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, implement ConfigurableRule. See AvoidExclaimOperator as an example.

Copy link
Copy Markdown
Author

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will start by saying that this is just my own personal opinion. I'm not a part of Microsoft or the PSScriptAnalyzer team - just an interested community member. @bergmeister is the community maintainer and will have their own take 😊.

I've submitted a few PRs to the project and added a few new rules over the years, and this is just from my own experience from going through PR review.

  • ArrayList is extremely common in existing PowerShell scripts. Enabling this by default would flood users with warnings on legacy code they may not own or want to refactor. PSSA is used commonly in CICD pipelines - introducing a bunch of new warnings (which you would usually treat as a build failure) would not be popular - maybe even with the PowerShell team themselves.

  • Replacing ArrayList with List[Object] changes behavior (no return value from .Add(). Code that relied on the index return from ArrayList.Add() - even accidentally via pipeline pollution - would break. Admittedly, 99.9% of the time, you don't want the collection size returned when you're appending to it - but there's always someone!

  • Making it opt-in lets people set it up for new code, or work through their legacy code base in their own time more easily.

As to why ConfigurableRule...

Only ConfigurableRule can be truly disabled by default. While it's true that all rules, including IScriptRule rules, can be stopped from participating in analysis by using -ExcludeRule as a cmdlet parameter or in a settings files - that is explicit opt-out. Any analysis with no settings file or include/exclude parameter will run all IScriptRule rules.

If you're interested, here's where the enabled rules are checked. If it's not a configurable rule, it's considered enabled.

private static bool IsRuleEnabled(IRule rule)
{
var configurableRule = rule as ConfigurableRule;
return configurableRule == null || configurableRule.Enable;
}

The Refactor should be fairly simple. ConfigurableRule is an abstract class which itself implements IScriptRule, so when you change from implementing IScriptRule to ConfigurableRule you will just need to mark your existing methods as override - it's not a wholesale rewrite. So for instance:

- public string GetCommonName() => Strings.AvoidUsingArrayListCommonName;
+ public override string GetCommonName() => Strings.AvoidUsingArrayListCommonName;

That should be it within the class. You would also need to update your tests to enable your rule for testing.

@{ Rules = @{ PSAvoidUsingArrayList = @{ Enable = $true } } }

As I said - just my opinion 🙂

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for you thoughts, I will take it in consideration.

{

/// <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;
Comment thread
iRon7 marked this conversation as resolved.
Outdated
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); }
Comment thread
iRon7 marked this conversation as resolved.
Outdated

// 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.IsMatch(bindingResult.BoundParameters["TypeName"].ConstantValue as string)
Comment thread
iRon7 marked this conversation as resolved.
Outdated
)
{
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;
}
}
12 changes: 12 additions & 0 deletions Rules/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -932,6 +932,18 @@
</data>
<data name="AvoidSemicolonsAsLineTerminatorsError" xml:space="preserve">
<value>Line ends with a semicolon</value>
</data>
<data name="AvoidUsingArrayListCommonName" xml:space="preserve">
<value>Avoid using the ArrayList class</value>
</data>
<data name="AvoidUsingArrayListDescription" xml:space="preserve">
<value>Avoid using the ArrayList class in PowerShell scripts. Consider using generic collections or fixed arrays instead.</value>
</data>
<data name="AvoidUsingArrayListName" xml:space="preserve">
<value>AvoidUsingArrayList</value>
</data>
<data name="AvoidUsingArrayListError" xml:space="preserve">
<value>The ArrayList class is used in '{0}'. Consider using a generic collection or a fixed array instead.</value>
</data>
<data name="PlaceOpenBraceName" xml:space="preserve">
<value>PlaceOpenBrace</value>
Expand Down
18 changes: 18 additions & 0 deletions Tests/Rules/AvoidUsingArrayList.ps1
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($_) }
26 changes: 26 additions & 0 deletions Tests/Rules/AvoidUsingArrayList.tests.ps1
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 "AvoidUsingWriteHost" {
Comment thread
iRon7 marked this conversation as resolved.
Outdated
Context "When there are violations" {
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

@iRon7 iRon7 Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests went completely bogus, I was under the impression that . './build.ps1' -Test also covered my Pester test but apparently not...
Anyways, I took the Tests\Rules\AvoidUsingWriteHost.tests.ps1 as an example as I wanted to test from a file to confirm the that the statement using namespace system.collections would make a difference. I have now added a -ForEach test to check each line that should result in a violation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad you got it sorted!

You shouldn't need a separate file for testing namespaces. The below example, with 2 scoped tests works fine.

BeforeAll {
    $settings = @{
        IncludeRules = @('PSAvoidUsingArrayList')
    }
}

Describe "AvoidArrayList" {
    Context "When using namespaces" {
        It "Should not find a violation calling new on an unrelated ArrayList Type" {
            $def = '
                using namespace System.Collections.Generic
                $List = [ArrayList]::new()
            '
            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
            $violations.Count | Should -Be 0
        }

        It "Should not find a violation casting an array on an unrelated ArrayList Type" {
            $def = '
                using namespace System.Collections.Generic
                $List = [ArrayList](1,2,3)
            '
            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
            $violations.Count | Should -Be 0
        }
    }
}

One of your tests is broken and is only passing because $noViolations is never defined so it's $null. $null.Count just happens to be 0.

Context "When there are no violations" {
    It "returns no violations" {
        $noViolations.Count | Should -Be 0
    }
}

I would strongly suggest revisiting the way you've structured the testing.

Especially the part below. No other rule tests call the parser to parse a file BEFORE Pester has even done test discovery. It feels very fragile, and really isn't needed.

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
    }

You still only have 2 test scenarios that you're making more assertions about.

When each test covers one scenario, it makes it far easier to more confidently change the rule in the future - and for others to not be able to break it doing unrelated changes.

Anyway - I'll stop Pestering (😉) you about this now and leave you to it. Hopefully you've had some fun and learned something. Feel free to reach out, if you want to discuss anything 👍

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
}
}
}
19 changes: 19 additions & 0 deletions Tests/Rules/AvoidUsingArrayListNoViolations.ps1
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()
47 changes: 47 additions & 0 deletions docs/Rules/AvoidUsingArrayList.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
---
description: Avoid reserved words as function names
Comment thread
iRon7 marked this conversation as resolved.
Outdated
ms.date: 08/31/2025
Comment thread
iRon7 marked this conversation as resolved.
Outdated
ms.topic: reference
title: AvoidUsingArrayList
---
# AvoidUsingArrayList

**Severity Level: Warning**

## Description

Important
Comment thread
iRon7 marked this conversation as resolved.
Outdated

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 unintendedly pollutes the PowerShell pipeline and therefore might cause unexpected issues.
Comment thread
iRon7 marked this conversation as resolved.
Outdated


## How to Fix

## 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 { $_ }
```
1 change: 1 addition & 0 deletions docs/Rules/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 | |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this rule should be enabled by default

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that in a lot of cases the use of the ArrayList class might cause a PowerShell pitfall (due to unintentionally polluting the PowerShell pipeline with the Add method), I would really like to see it enabled by default.
Anyways, I leave it to your team to make the final decision on this.

| [AvoidUsingBrokenHashAlgorithms](./AvoidUsingBrokenHashAlgorithms.md) | Warning | Yes | |
| [AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes | Yes<sup>2</sup> |
| [AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | Yes | |
Expand Down
Loading