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.
| 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; |
| break; | ||
| } | ||
| } | ||
| if (arrayListName == null) { arrayListName = new Regex(@"^(System\.)?Collections\.ArrayList$", RegexOptions.IgnoreCase); } |
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)
done
done |
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.