Add new AvoidUsingNewObject rule - #2186
Conversation
Sorry, something went wrong.
…nd suggest type initializer as a fix.
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 AvoidUsingNewObject rule 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.
Sorry, something went wrong.
| /// <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...
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| 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).
Sorry, something went wrong.
|
Andy Jordan (@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. |
Sorry, something went wrong.
|
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... |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Closes #2046
PR Summary
Avoid using the New-Object cmdlet 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-Object cmdlet.
In other words, if e.g. the common -Verbose parameter is used, or both the parameters -ArgumentList and -Property are used, there won't be a simple type initializer available and the rule won't report any violation for the New-Object cmdlet.
Nevertheless, there are still some cases where the New-Object cmdlet 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 -ArgumentList parameter 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