@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 ofComObject 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 aC, soC andCo are both valid (if not ideal) ways to supply theComObject 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 theHasComObjectParameter function.

  https://github.com/DrSkillIssue/PSScriptAnalyzer/blob/26c779cdf5c42da4ac5e5edc4a09e7954d8f85ad/Rules/AvoidUsingNewObject.cs#L134-L138

  It's looping through theCommandElements of theCommandAst. Each time it encounters aCommandParameterAst it setswaitingForParamValue to true, which skips the next element (assuming it's a parameter value).

  I like to useJason 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 indocs/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 inea70855

     	It"get Rules with no parameters supplied" {
     	$defaultRules=Get-ScriptAnalyzerRule
     	$expectedNumRules=70
     	if ($PSVersionTable.PSVersion.Major-le4)
     	{
     	# 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 forpassing andfailing 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 inEngine/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. ⭐