Add PSAvoidUsingNewObject Rule
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 -TypeNameusage while preservingNew-Object -ComObject(COM objects cannot use type literals)
Key Features
-
Smart COM Object Detection: Excludes
New-Object -ComObjectcalls 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
- [x] PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- [x] Summarized changes
- [x] Change is not breaking
- [x] Make sure all
.cs,.ps1and.psm1files have the correct copyright header - [x] Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
- [x] 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.
- If the PR is work in progress, please add the prefix
@microsoft-github-policy-service agree
👋 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
ComObjectin 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-Objectonly has 1 parameter starting with aC, soCandCoare both valid (if not ideal) ways to supply theComObjectparameter.$Splat = @{ 'C' = 'Word.Application' } New-Object @Splat $Splat2 = @{ 'Co' = 'Word.Application' } New-Object @Splat2 -
When a switch parameter (such as
-Strictor-Verbose) precedes-ComObjectin the command expression, a violation is still emitted.New-Object -Verbose -ComObject 'Word.Application'This is probably coming from the
HasComObjectParameterfunction.https://github.com/DrSkillIssue/PSScriptAnalyzer/blob/26c779cdf5c42da4ac5e5edc4a09e7954d8f85ad/Rules/AvoidUsingNewObject.cs#L134-L138
It's looping through the
CommandElementsof theCommandAst. Each time it encounters aCommandParameterAstit setswaitingForParamValueto 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.
CommandParameterAstcan be sequential, without a variable expression or constant expression between them. -
You're getting 2 failing tests (you can run tests locally using
.\build.ps1 -Test):- 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. - 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
- You need to add an entry for your rule into the table in
-
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. ⭐