UseShouldProcessForStateChangingFunctions should prove that a function changes system state.

[imfrancisd]

Right now, the UseShouldProcessForStateChangingFunctions rule only looks at the name of the function (the verb) in order to determine that the function changes system state.

I think the rule should look at the body of the function and see if the function uses other functions that changes system state before it issues a warning.

If not, then I think that functions with the "New" verb should not be considered as functions that change system state. A lot of functions from the community with the "New" verb do not change system state, but are actually just different forms of "New-Object". See, for example, the ShowUI module.

[KirkMunro]

I actually typed this exact issue yesterday, then decided to wait and think about it a little before posting.

If you look in core PowerShell commands, there are 11 "New" commands that don't support ShouldProcess and 7 that do. New is often used to create things in memory or in session that are not state changes to a system, and for this data set the rule generates more noise than value.

Just to add perspective though, if you look across the entire cmdlet set on my system, there are 412 "New" cmdlets that don't implement ShouldProcess and only 11 that do, and when you review the 412 that don't, there are clearly many that should support/implement ShouldProcess because they make state changes (New-VirtualSwitch, etc.). From that data set, having a rule that warns on New commands seems like a good idea.

This is a tricky one. The current list of verbs in this rule (in my fork) are Stop, Reset, Restart, Set, Update, and New. What about Start? Add? Remove? Checkpoint? Install? Uninstall? Repair?Publish? Unpublish? Backup? Restore? Lock? Unlock? Register? Unregister? Approve? Grant? Deny? Revoke? Enable? Disable? Rename? Resume? Suspend? Block? Unblock? Protect? Unprotect? Compress? Expand? Import? Export? Register? Unregister? There are a ton of verbs where ShouldProcess should be used more often than not, but in many of them ShouldProcess would not apply as well.

I think analyzing the body is a great approach because if all your "New" command is doing is wrapping one or more commands that do not support ShouldProcess (I have one that wraps Set-PSBreakpoint, which is session-based and does not and should not support ShouldProcess) then why should users get warned about that? But this rule obviously would have limitations because if you're invoking a bunch of dot net methods internally and not calling commands, then it wouldn't be able to judge as well.

I think it should cover a larger set of verbs, analyze the internal commands being called as suggested, and if it positively identifies a command being invoked that supports ShouldProcess then it warns if ShouldProcess is not present. That would allow it to present accurate warnings across a larger set of commands. If not though, it shouldn't generate noise like it does now. Unfortunately that means it will miss some recommendations, but I'd prefer that so that PSScriptAnalyzer becomes a more useful tool for everyone than if it is too noisy for people to want to use it.

Related: it seems like there is a bug in this rule. Looking at the source, I don't think this rule properly supports if a command uses [CmdletBinding(SupportsShouldProcess=$false)]. The logic seems to indicate that would be treated as if it has the should process attribute when it actually does not. No time for me to test this right now though.

[imfrancisd]

I think it should cover a larger set of verbs, analyze the internal commands being called as suggested, and if it positively identifies a command being invoked that supports ShouldProcess then it warns if ShouldProcess is not present.

I don't think this rule should be looking at verbs at all.

    function new-something
    {
        [cmdletbinding()]
        Param()

        newSomethingHelper
    }

    function newSomethingHelper
    {
        #call to cmdlet that changes state
    }

    function get-something
    {
        [cmdletbinding()]
        Param()

        #some commands

        newSomethingHelper

        #some commands
    }

If the rule can figure out the "newSomethingHelper" can change system state, then it can figure out that all functions that call "newSomethingHelper" can change system state.

So, if I create a function called "Get-Something", and for some reason that function calls "newSomethingHelper", then looking at the verb will miss that case ("Get" typically does not change system state), but not looking at the verb will detect that case.

But this rule obviously would have limitations because if you're invoking a bunch of dot net methods internally and not calling commands, then it wouldn't be able to judge as well.

I think that this is an understandable limitation.

Once someone calls dot net methods (using things like reflection, embedded c#, pinvoke, and so on) then anything goes, and I think users will understand that that kind of thing is outside the scope of PSScriptAnalyzer. However, it can probably detect some well known methods that change system state, such as $xml.Save(...).

If not though, it shouldn't generate noise like it does now. Unfortunately that means it will miss some recommendations, but I'd prefer that so that PSScriptAnalyzer becomes a more useful tool for everyone than if it is too noisy for people to want to use it.

Completely agree.

[KirkMunro]

If it doesn't look at verbs though, it opens the door for more false positives.

For example, Set-Variable supports ShouldProcess, and I have had to use it in a Get command and other verbs that don't change state several times. If this rule looks at Get commands as well, it opens the door for more noise from false positives. It could special case Set-Variable, but there may be other commands that support ShouldProcess that are not special cases.

I would prefer putting all verbs in two categories, with the following improvements:

1. Having this rule check commands with verbs that are more likely to change state to see if internally ShouldProcess commands are in use, then suggesting that ShouldProcess should be added if it is missing.
2. Have another new rule that checks commands with verbs that are not likely to change state, looking internally to see if they contain any commands that support ShouldProcess that are not being invoked with -Confirm:$false -Whatif:$false and warn on those, suggesting that the containing command should be updated to a more appropriate verb and that ShouldProcess support should be added, or that Confirm and Whatif should be "turned off" for the internal command that supports ShouldProcess if the it does not actually modify state.

That approach would be much more helpful for a broader set of scenarios, moving this module closer to the must-have tool category by generating the right message at the right time.