Add new AvoidUsingArrayList rule#2174
Conversation
…lass is used in PowerShell scripts. Added tests for both violations and non-violations of this rule. Updated documentation to include the new rule and its guidelines.
iRon7
requested review from
a team,
bergmeister,
michaeltlombardi and
sdwheeler
as code owners
liamjpeters
left a comment
liamjpeters
left a comment
There was a problem hiding this comment.
👋 @iRon7 - great that you've taken the plunge, and thanks for being one of 3 people to read my blog post (myself included) 😊.
I hope you don't mind, but I took a look over the PR and left some suggestions. I'm happy to discuss or elaborate on anything.
Thanks,
I clearly used several other rules as a kind of template...🤪 Co-authored-by: Liam Peters <liamjpeters@gmail.com>
Co-authored-by: Liam Peters <liamjpeters@gmail.com>
…ptAnalyzer into #2147AvoidArrayList
Co-authored-by: Liam Peters <liamjpeters@gmail.com>
Co-authored-by: Liam Peters <liamjpeters@gmail.com>
|
Liam, One general thought that came up: That prepares things as a stripped |
There's the RuleMaker module. I've tried it before with mixed results. It needs a little TLC (copyright is outdated, doesn't append to string resources super well etc). |
|
After several additional commits based on the valuable feedback from Liam, I consider this PR as final now. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a new PSScriptAnalyzer rule to warn on usage of System.Collections.ArrayList in PowerShell scripts, along with accompanying tests and documentation updates.
Changes:
- Adds the
AvoidUsingArrayListrule implementation and localized strings. - Adds Pester tests covering violations, non-violations, and enable/disable behavior.
- Updates rules documentation to list and describe the new rule.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Rules/README.md | Adds the new rule to the rules index table. |
| docs/Rules/AvoidUsingArrayList.md | Introduces end-user documentation and examples for the new rule. |
| Tests/Rules/AvoidUsingArrayList.tests.ps1 | Adds Pester coverage for rule violations and configuration behavior. |
| Rules/Strings.resx | Adds localized name/description/message resources for the new rule. |
| Rules/AvoidUsingArrayList.cs | Implements the new analyzer rule logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <returns>A an enumerable type containing the violations</returns> | ||
| public override 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); } |
| 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; |
| ) | ||
| ) | ||
| { | ||
| arrayListName = new Regex(@"^((System\.)?Collections\.)?ArrayList$", RegexOptions.IgnoreCase); |
| break; | ||
| } | ||
| } | ||
| if (arrayListName == null) { arrayListName = new Regex(@"^(System\.)?Collections\.ArrayList$", RegexOptions.IgnoreCase); } |
| --- | ||
| description: Avoid using ArrayList | ||
| ms.date: 04/16/2025 | ||
| ms.topic: reference | ||
| title: AvoidUsingArrayList | ||
| --- |
|
|
||
| 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 |
There was a problem hiding this comment.
Thanks for sticking with this one, @iRon7, and thanks @liamjpeters for the thorough review — I read through everything you both worked through.
Most of the review threads have been addressed, which is great. There is one point I want to push back on before merging, plus one small follow-up.
Opt-in by default. Liam asked for Enable = false and you went with Enable = true to head off the pipeline-pollution gotcha. That gotcha is real and your reasoning is fair, but I think Liam's ask is the right call here, for the same reasons he laid out: existing scripts that intentionally use [ArrayList] (Add-Type wrappers, certain interop scenarios, lots of legacy code in the wild) would suddenly emit warnings on upgrade, and PSSA is heavily wired into CI/CD where new diagnostics turn into build failures. Our convention for new rules has been opt-in for that reason — see AvoidExclaimOperator, AvoidUsingDoubleQuotesForConstantString, AvoidSemicolonsAsLineTerminators, UseConstrainedLanguageMode, UseConsistentParametersKind. The pipeline-pollution discoverability problem is better addressed by the rule existing and being toggleable than by enabling it for everyone. Could you flip Enable = true to Enable = false in the constructor and update docs/Rules/README.md so the "Enabled by Default" column reads "No"? Thanks!
One small follow-up:
Tests/Rules/AvoidUsingArrayList.tests.ps1has solid coverage for the match cases. Would you mind adding one negative case for[ArrayList]::new()without ausing namespace System.Collections(so the regex's not-prefixed branch is exercised against a non-matching input)? Just to keep that branch covered.
Once those are settled, this is good to go from my side.
Drafted by Copilot (Claude Opus 4.7)
to warn when the ArrayList class is used in PowerShell scripts (#2147).
Add tests for both violations and non-violations of this rule.
Update documentation to include the new rule and its guidelines.
(For this, I followed Liam Peters' blog post, also note that this is my first formal
C#contribution to any GitHub project)PR Summary
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.