Add new AvoidUsingArrayList rule

[iRon7]

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

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

[liamjpeters]

👋 @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,

[liamjpeters]

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

[iRon7]

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.

[liamjpeters]

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.

[iRon7]

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.

[liamjpeters]

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 👍

[iRon7]

Liam,
Thanks you for your help and feedback, I learned a lot!

One general thought that came up:
Instead of testing all things afterwards (and still making a lot of copy-pasta misstakes, as I did), a small cmdlet like:

New-CSRule -Name <name> -Description <Description> -Message <Message> -Configurable ...

That prepares things as a stripped .cs rule and .ps1 test (without logic) along with the Strings.resx and the readme updates etc. could in my opinion be helpful and prevent a lot of issues at forehand.

[liamjpeters]

Liam, Thanks you for your help and feedback, I learned a lot!

One general thought that came up: Instead of testing all things afterwards (and still making a lot of copy-pasta misstakes, as I did), a small cmdlet like:

New-CSRule -Name <name> -Description <Description> -Message <Message> -Configurable ...

That prepares things as a stripped .cs rule and .ps1 test (without logic) along with the Strings.resx and the readme updates etc. could in my opinion be helpful and prevent a lot of issues at forehand.

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