PSScriptAnalyzer icon indicating copy to clipboard operation
PSScriptAnalyzer copied to clipboard

Add PSAvoidUsingNewObject Rule

Open DrSkillIssue opened this issue 7 months ago • 2 comments

PR Summary

#2046

Adds a new built-in rule PSAvoidUsingNewObject that flags usage of the New-Object cmdlet and recommends using type literals with ::new() syntax instead for better performance and more idiomatic PowerShell code.

What Changed

  • New Rule: AvoidUsingNewObject
  • Severity: Warning
  • Scope: Flags New-Object -TypeName usage while preserving New-Object -ComObject (COM objects cannot use type literals)

Key Features

  • Smart COM Object Detection: Excludes New-Object -ComObject calls since they cannot be replaced with type literals
  • Parameter Splatting Support: Handles complex scenarios where parameters are passed via hashtable splatting (New-Object @params)
  • Variable Chain Resolution: Recursively follows variable assignments to determine if splatted parameters contain COM object references
  • Comprehensive AST Analysis: Supports both direct parameter usage and various splatting patterns including $using: variables

PR Checklist

DrSkillIssue avatar Jun 15 '25 16:06 DrSkillIssue

@microsoft-github-policy-service agree

DrSkillIssue avatar Jun 15 '25 16:06 DrSkillIssue

👋 Hey @DrSkillIssue, thanks for putting this together; you've clearly put a lot of thought into the many ways you could try and sneak a -ComObject in 😎.

Some things to look into:

  • When checking for splatting use of ComObject in a hashtable, you dismiss keys that are less than 3 characters. You do this to avoid false positives, but I'm not sure what would lead to a false positive?

    https://github.com/DrSkillIssue/PSScriptAnalyzer/blob/26c779cdf5c42da4ac5e5edc4a09e7954d8f85ad/Rules/AvoidUsingNewObject.cs#L423-L425

    Skipping 1 and 2 character keys does miss the below, which are both valid. New-Object only has 1 parameter starting with a C, so C and Co are both valid (if not ideal) ways to supply the ComObject parameter.

    $Splat = @{
        'C' = 'Word.Application'
    }
    New-Object @Splat
    
    $Splat2 = @{
        'Co' = 'Word.Application'
    }
    New-Object @Splat2
    
  • When a switch parameter (such as -Strict or -Verbose) precedes -ComObject in the command expression, a violation is still emitted.

    New-Object -Verbose -ComObject 'Word.Application'
    

    This is probably coming from the HasComObjectParameter function.

    https://github.com/DrSkillIssue/PSScriptAnalyzer/blob/26c779cdf5c42da4ac5e5edc4a09e7954d8f85ad/Rules/AvoidUsingNewObject.cs#L134-L138

    It's looping through the CommandElements of the CommandAst. Each time it encounters a CommandParameterAst it sets waitingForParamValue to true, which skips the next element (assuming it's a parameter value).

    I like to use Jason Shirk's Show-AST to visualise this stuff. CommandParameterAst can be sequential, without a variable expression or constant expression between them.

    image
  • You're getting 2 failing tests (you can run tests locally using .\build.ps1 -Test):

    1. You need to add an entry for your rule into the table in docs/Rules/README.md. The format should be obvious from all the other rules.
    2. You need to increment the rule count of the default rules. With your new rule, there'd be 71. https://github.com/PowerShell/PSScriptAnalyzer/blob/ea70855e9d6de8c214757b4dad7b6ed92e8348c8/Tests/Engine/GetScriptAnalyzerRule.tests.ps1#L64-L76
  • Regarding tests: it's great that you've got so many test cases documented for passing and failing scenarios. It would be so much better if these were all individual tests.

    Currently there are only 3 tests. One for the error message description, one for there being 0 violations in one file, another for there being 20 violations in the other file.

    Small focused tests help pinpoint any future regressions and ensure coverage across edge-cases (rather than a number of violations going from 20 to 19 - it would show which test case has changed).

  • You've changed some formatting in Engine/Commands/InvokeScriptAnalyzerCommand.cs. This formatting change is the only change you're making in that file and it's not related to your new rule or this PR.

I really liked your rule documentation, lots of detail and further reading links. ⭐

liamjpeters avatar Aug 17 '25 15:08 liamjpeters