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.
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,
| "/PSCompatibilityCollector/optional_profiles": true | ||
| } | ||
| }, | ||
| "cSpell.words": [ |
There was a problem hiding this comment.
This is a personal editor preference and shouldn't be in this PR
| @@ -0,0 +1,47 @@ | |||
| --- | |||
| description: Avoid reserved words as function names | |||
There was a problem hiding this comment.
| description: Avoid reserved words as function names | |
| description: Avoid using ArrayList |
| @@ -0,0 +1,47 @@ | |||
| --- | |||
| description: Avoid reserved words as function names | |||
| ms.date: 08/31/2025 | |||
There was a problem hiding this comment.
| ms.date: 08/31/2025 | |
| ms.date: 04/15/2026 |
|
|
||
| ## Description | ||
|
|
||
| Important |
There was a problem hiding this comment.
Should this be alert style syntax?
> [!IMPORTANT]
> Note here
| | [AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | Yes | | | ||
| | [AvoidTrailingWhitespace](./AvoidTrailingWhitespace.md) | Warning | Yes | | | ||
| | [AvoidUsingAllowUnencryptedAuthentication](./AvoidUsingAllowUnencryptedAuthentication.md) | Warning | Yes | | | ||
| | [AvoidUsingArrayList](./AvoidUsingArrayList.md) | Warning | Yes | | |
There was a problem hiding this comment.
I don't think that this rule should be enabled by default
| 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. |
There was a problem hiding this comment.
| often unintendedly pollutes the PowerShell pipeline and therefore might cause unexpected issues. | |
| often unintentionally pollutes the PowerShell pipeline and therefore might cause unexpected issues. |
| break; | ||
| } | ||
| } | ||
| if (ArrayListName == null) { ArrayListName = new Regex(@"^(System\.)?Collections\.ArrayList", RegexOptions.IgnoreCase); } |
There was a problem hiding this comment.
I think this is missing the closing anchor?
| if (ArrayListName == null) { ArrayListName = new Regex(@"^(System\.)?Collections\.ArrayList", RegexOptions.IgnoreCase); } | |
| if (ArrayListName == null) { ArrayListName = new Regex(@"^(System\.)?Collections\.ArrayList$", RegexOptions.IgnoreCase); } |
| $ruleMessage = "The ArrayList class is used in '*'. Consider using a generic collection or a fixed array instead." | ||
| } | ||
|
|
||
| Describe "AvoidUsingWriteHost" { |
There was a problem hiding this comment.
| Describe "AvoidUsingWriteHost" { | |
| Describe "AvoidArrayList" { |
|
|
||
| Describe "AvoidUsingWriteHost" { | ||
| Context "When there are violations" { | ||
| $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) |
There was a problem hiding this comment.
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.
| // Check for -TypeName parameter | ||
| if ( | ||
| bindingResult.BoundParameters.ContainsKey("TypeName") && | ||
| ArrayListName.IsMatch(bindingResult.BoundParameters["TypeName"].ConstantValue as string) |
There was a problem hiding this comment.
ConstantValue could be null here - i.e. if it's a variable expression and not a compile-time constant.
$type = "System.Collections.ArrayList"
New-Object -TypeName $typeIf it were, IsMatch() would not be a happy camper.
Take a look at AvoidGlobalAliases and UseConstrainedLanguageMode for how they handle it.
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.