Add new AvoidUsingNewObject rule#2186
Conversation
…nd suggest type initializer as a fix.
iRon7
requested review from
a team,
bergmeister,
michaeltlombardi and
sdwheeler
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new PSScriptAnalyzer rule that warns on New-Object usage and provides auto-fix suggestions, along with documentation, localized strings, and Pester tests.
Changes:
- Added
AvoidUsingNewObjectrule implementation with suggested corrections/fixes. - Added rule documentation and registered it in the rules index.
- Added localization strings and comprehensive Pester tests for detection/suppression/fix behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Rules/README.md | Registers the new rule in the documentation rules table. |
| docs/Rules/AvoidUsingNewObject.md | Adds rule reference documentation and examples. |
| Tests/Rules/AvoidUsingNewObject.tests.ps1 | Adds Pester coverage for rule behavior, suppression, and -fix. |
| Rules/Strings.resx | Adds localized strings for rule name/description/message/correction. |
| Rules/AvoidUsingNewObject.cs | Implements the rule logic and suggested corrections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <returns>A an enumerable type containing the violations</returns> | ||
| public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) | ||
| { | ||
| if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); |
There was a problem hiding this comment.
Not sure whether we want to do this this way as most other rules do have Strings.NullAstErrorMessage as argument...
| else if (boundResult.ConstantValue != null) | ||
| { | ||
| string valueText = boundResult.Value.Extent.Text; | ||
| if ( | ||
| boundResult.Value is StringConstantExpressionAst stringConstant && | ||
| stringConstant.StringConstantType == StringConstantType.BareWord | ||
| ) | ||
| { | ||
| valueText = '"' + valueText.Replace("\"", string.Empty) + '"'; // Test""123 --> "Test123" | ||
| } | ||
| correction = "[" + typeName + "]" + valueText; | ||
| } |
There was a problem hiding this comment.
I think I got this right, if the -ArgumentList contains a constant value, I might use a shorter (preferred) syntax by simply casting the value expression.
For the given example: New-Object DateTime 0 vs [DateTime]0, both return the same results:
Monday, January 1, 0001 12:00:00 AM| else if ( | ||
| boundResult.Value is ParenExpressionAst parenExpressionAst && | ||
| !parenExpressionAst.Pipeline.Extent.Text.StartsWith(",") | ||
| ) |
There was a problem hiding this comment.
The Pipeline.Extent.Text (the inner Ast of the ParenExpressionAst) is already trimmed.
Btw. I don't think there is an easier way to check for a wrapped object (unary comma operator).
iRon7
changed the title
|
@andyleejordan, thanks for trying to push this one through. I am currently on holidays, I will check next week on why it failed the workflow. |
|
Apparently I was behind with my fork which caused all kind of errors. All local tests (PowerShell 5.1 and 7) have a green light now... |
Closes #2046
PR Summary
Avoid using the
New-Objectcmdlet to create objects as it might perform poorly.Instead, use a type initializer to construct or cast the intended object.
Note
In most cases if there isn't an automatic correction (
-fix) available, the rule won't report any violation either.This is because if there isn't an automatic correction available, it generally means that there isn't a simple type-casting or type constructor that would be more efficient or has a better syntax than the using
New-Objectcmdlet.In other words, if e.g. the common
-Verboseparameter is used, or both the parameters-ArgumentListand-Propertyare used, there won't be a simple type initializer available and the rule won't report any violation for theNew-Objectcmdlet.Nevertheless, there are still some cases where the
New-Objectcmdlet might be replaceable with a type initializer that would be more efficient or has a better syntax, but an automatic correction can't be provided.For example if the
-ArgumentListparameter is used with a dynamic variable, the rule will report a violation, but won't be able to provide a correction (-fix), as it's not possible to determine from the AST alone whether the variable contains a single value that can be used in a type initializer, or if it contains multiple values that would require splatting.This PR replaces PR #2109
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.