The rule PSUseShouldProcessForStateChangingFunctions is far too simple

[Jaykul]

You can't determine if a command is "State Changing" based purely on the verb. This is the sort of stuff that makes me SO upset at this module that I just want to throw it out -- it takes too much time to go through and review stuff when most of the warnings/errors are wrong.

For one example, I'm getting that warning on a bunch of XML document commands like "New-XElement" and "New-XAttribute" which obviously don't modify system state, they just create xml nodes and attributes which can at least hypothetically be added to a document in memory.

For another example, almost every single command in the ShowUI module uses the "New" verb to generate UI elements on windows, not to modify the system state.

[raghushantha]

Hi Joel.

During static analysis we use the verb to determine if the cmdlet changes the system state. Perhaps we can do additional heuristics - such as the cmdlet calling into known state changing functions... Let us know if you have opinions on this.

"Profiles" support in ScriptAnalyzer can be helpful in such situations. We plan to ship a default set of profiles, where one of them could be a exclude list of rules - containing this rule.

Another option is to use Inline suppression to annotate your scripts.

-Raghu

[Jaykul]

I think the idea of requiring ShouldProcess based on the verb is wrong -- it's going to have far too many false-positives.

Looking at known state-changing functions might be better, but I'm not really sure that it makes sense for every function which calls functions which support ShouldProcess to also support ShouldProcess -- is that actually the right design?

[kilasuit]

I agree with @Jaykul - This is one that has tripped me over this morning due to calling into another function ( from Exchange Online) as a Custom Function.

Perhaps there needs to be a definition update to the "severity" of State Changes - examples being Removing items or modifying existing key items like registry.

Also looking into the Definition there doesn't seem to be any check in place for the Remove Verb which would be the most destructive and should IMO require ShouldProcess support

[KirkMunro]

Just thinking something out loud. The concern over false positives is because there are plenty of scenarios where you wouldn't want whatif for a given verb, even for what might be considered a more serious verb like "Remove" (removing something from a stored collection in memory, for example). I do like the idea, however, of PSScriptAnalyzer watching my back for those commands that really should have "ShouldProcess" on them, but the verb by itself doesn't seem to be enough. The noun is what's really important, in combination with the verb, because the noun is what identifies if you're changing system state. The only downside is that the impact of the noun cannot be determined programmatically without additional metadata. In that case, maybe there should be certain rules that "light up" only if additional optional metadata is present. For example, with a module containing dozens or even hundreds of commands, there are far fewer nouns, and if there was a way to identify which of those nouns are, or are not if that is easier/preferred, high impact nouns, then PSScriptAnalyzer could warn on specific verb use with those nouns in that module where ShouldProcess is not used. Today PSScriptAnalyzer does not analyze modules as a whole, so that would have to happen first I think. The idea might be too confusing, but like I indicated in the beginning, it was just an idea that I wanted to think out loud and add to the discussion here to see if it might be useful for future invention in this area that makes sense for the majority of cmdlets/functions that are being written as PSScriptAnalyzer moves forward.

[kilasuit]

That could be a scenario where Pester & PSScriptAnalyzer can work well together - perhaps it would be useful to add some pester Tests that call on PSScriptAnalyzer to show these tools working together which is something I'm currently looking at putting together myself

[jdhitsolutions]

Here's are related issue I'm running into. Let's say I have a function like this:

function Remove-Foo {
[CmdletBinding(supportsShouldProcess)]
Param(
    [Parameter(Position = 0, Mandatory, ValueFromPipeline,ValueFromPipelineByPropertyName]
    [string]$Path
)

Process {
    Write-Verbose "Removing $($path)"
    Remove-Item -Path $Path 
 }
}

I don't need to write any code to implement SupportsShouldProcess because it will get passed to Remove-Item. Yet this gets flagged as a problem.

[kapilmb]

Adding a reference to #194, as that seems to be the more appropriate issue for the problem @jdhitsolutions has posted.

[StingyJack]

A cmdlet that sets a user property value in a database, that would naturally be called Set-UserPropertyValue does not change system state and should not be prompting this rule violation.

Set is a common verb that is not tied to any state changing functions, and functions could use another verb that would change system state. If this rule isn't looking at what the script is actually doing, and is just keying off the verb, it needs to be removed until fixed.

Also, naming is hard, and the list of non-warning throwing verbs is already small. This just restricts it further.

[sagmor]

What about using ConfirmImpact to declare a function is not impactfull enough to merit supporting ShouldProcess. so something like this could be excluded from the validation:

function Set-SomethingMinor {
  [CmdletBinding(ConfirmImpact='None')]
  Param()
  ...
}

I like that it's a simple enough change that explicitly tells the user this is a function that set's something I won't even care to confirm)