Add PSAvoidUsingNewObject Rule#2109
Conversation
|
@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 Some things to look into:
I really liked your rule documentation, lots of detail and further reading links. ⭐ |
|
@DrSkillIssue There are 2 test failures, can you please look into fixing them? |
bergmeister
left a comment
There was a problem hiding this comment.
Once my comments are being addressed, I'd be happy to approve, the most important one is that I think rule should not be enabled by default to start with
|
|
||
| foreach (var assignment in assignments) | ||
| { | ||
| if (assignment.Right is CommandExpressionAst cmdExpr) |
There was a problem hiding this comment.
there are a few areas like this one where indentation could be reduced. Here we could e.g. negate statement and continue with next loop member so that other code can just be below at same indentation level: if (assignment.Right is not CommandExpressionAst cmdExpr) { continue; }
There was a problem hiding this comment.
this file is to replicate setting used by PSGallery to scan code when users upload new module and send email if there are violations. I don't even know whether this file is still in sync as of today but this file should only be updated by PSGallery members who know the rules used by PSGallery (and consider adding new ones).
Can you comment on whether this file is up to date and if not open separate PR to sync please @alerickson ?
There was a problem hiding this comment.
I am not sure what the purpose of this settings file is but until then I'd not recommend touching it please
| #if !CORECLR | ||
| [Export(typeof(IScriptRule))] | ||
| #endif | ||
| public class AvoidUsingNewObject : IScriptRule |
There was a problem hiding this comment.
With this inheritance rule will run by default but I fear it will cause a big backlash as many people run PSSA in CI. I suggest to inherit from ConfigurableRule so that it's not enabled by default. See this PR for an example of how to: #2124
|
Despite the nice work already put in this rule, it appears to me that the PR it is stalled on a minor step:
|
|
Reading more closely through the comments, I see that there is more behind the stall of this PR than thought initially. Please, let me know what you are planned with this pending PR. |
andyleejordan
left a comment
There was a problem hiding this comment.
Thanks for the contribution @DrSkillIssue — and welcome! The rule itself is one we've previously said we'd accept (#2046), the documentation you wrote is genuinely good (clear examples, real rationale), and the variable-tracking work for splatting/$using: scenarios shows real care. Before this can land there's a mix of correctness, hygiene, and design things to work through.
CI is failing for two real reasons that need fixing:
- The new rule isn't listed in
docs/Rules/README.md. The doc-validation test enforces every rule appearing in that index. Tests/Engine/GetScriptAnalyzerRule.tests.ps1hardcodes the expected built-in rule count; that needs to bump for the new rule.
Two correctness bugs — both of which @liamjpeters already pointed out and are worth fixing with tests:
- Switch parameters before
-ComObject(e.g.New-Object -Verbose -ComObject 'Word.Application') get incorrectly flagged. ThewaitingForParamValuestate machine assumes everyCommandParameterAstis followed by a value argument, but switches don't have one —CommandParameterAst.Argumentis null in that case and we should detect it. - The 3-character minimum length filter on hashtable keys for splat detection rules out valid abbreviations:
@{ C = 'Word.App' }and@{ Co = 'Word.App' }both bind to-ComObjectbecause PowerShell accepts any unique prefix, andNew-Objectonly has one parameter starting withC. Removing the length floor and using prefix-based parameter resolution would handle this correctly.
Hygiene cleanup needed:
Engine/Commands/InvokeScriptAnalyzerCommand.cshas whitespace-only changes unrelated to the rule — please revert those.Rules/Strings.resxhas a few typos introduced into entries unrelated to the new rule (e.g. "equaltiy", "comaprision"). Looks like accidental rebase fallout — please restore those originals.
Design question — and this one I'd really like your view on, plus @bergmeister's: the rule is currently added to both PSGallery.psd1 and ScriptFunctions.psd1, which means it would be enabled by default the moment we ship it. New-Object is still legitimately used in a lot of production code (especially for COM, for -Property splatting, and in modules that target older PowerShell versions), and turning a Warning on by default for the entire ecosystem is a different commitment than shipping the rule itself. I'd lean toward starting it disabled — either omitted from the default presets, or in a strict/opt-in preset only — and revisiting once we see how it lands. Curious what you think.
Test structure — non-blocking, but worth considering: the bulk-file-with-assertion-count style ($violations.Count | Should -Be 17) makes regressions hard to localize — a count drifting from 17 to 16 doesn't tell you which scenario broke. Splitting the cases into individual It blocks (one per scenario) would pay off the next time someone touches this rule. Up to you whether to do that here or as a follow-up.
Once the CI failures, the two bugs (with regression tests), and the hygiene items are addressed, and we settle the default-on question, I think this is in good shape. Happy to help if you hit anything weird.
— Drafted by Copilot (Claude Opus 4.7)

PR Summary
#2046
Adds a new built-in rule
PSAvoidUsingNewObjectthat flags usage of theNew-Objectcmdlet and recommends using type literals with::new()syntax instead for better performance and more idiomatic PowerShell code.What Changed
AvoidUsingNewObjectNew-Object -TypeNameusage while preservingNew-Object -ComObject(COM objects cannot use type literals)Key Features
New-Object -ComObjectcalls since they cannot be replaced with type literalsNew-Object @params)$using:variablesPR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.