GeezerVison

@microsoft-github-policy-service agree

👋 Hey Dr. Skill Issue (@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.

  

• 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.
     

     PSScriptAnalyzer/Tests/Engine/GetScriptAnalyzerRule.tests.ps1

     Lines 64 to 76 in ea70855

     	It "get Rules with no parameters supplied" {
     	$defaultRules = Get-ScriptAnalyzerRule
     	$expectedNumRules = 70
     	if ($PSVersionTable.PSVersion.Major -le 4)
     	{
     	# for PSv3 PSAvoidGlobalAliases is not shipped because
     	# it uses StaticParameterBinder.BindCommand which is
     	# available only on PSv4 and above
     	$expectedNumRules--
     	}
     	$defaultRules.Count | Should -Be $expectedNumRules
     	}

• 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. ⭐

Dr. Skill Issue (@DrSkillIssue) There are 2 test failures, can you please look into fixing them?

Despite the nice work already put in this rule, it appears to me that the PR it is stalled on a minor step:

Secondly, we add an entry for our new rule in the docs/Rules/README.md file, in the PSScriptAnalyzer Rules table. We list it as enabled by default and of Warning severity.

Dr. Skill Issue (@DrSkillIssue),

Reading more closely through the comments, I see that there is more behind the stall of this PR than thought initially.
Is there any intention to finish this PR?
Otherwise, I will pick this PSAvoidUsingNewObject rule request up by the end of this month and with that, likely start from scratch again.

Please, let me know what you are planned with this pending PR.

Thanks for the contribution Dr. Skill Issue (@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.ps1 hardcodes the expected built-in rule count; that needs to bump for the new rule.

Two correctness bugs — both of which Liam Peters (@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. The waitingForParamValue state machine assumes every CommandParameterAst is followed by a value argument, but switches don't have one — CommandParameterAst.Argument is 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 -ComObject because PowerShell accepts any unique prefix, and New-Object only has one parameter starting with C. Removing the length floor and using prefix-based parameter resolution would handle this correctly.

Hygiene cleanup needed:

• Engine/Commands/InvokeScriptAnalyzerCommand.cs has whitespace-only changes unrelated to the rule — please revert those.
• Rules/Strings.resx has 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 Christoph Bergmeister (@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)

as previously commented I think we should not touch the settings preset files. we should just worry about defaults when running invoke-scriptanalyzer. It's up to PSGallery team to maintain this settings file, which they run when someone uploads a module to Gallery. The other file I am not sure who uses and classify it as legacy