PSShouldProcess rule is getting *worse* instead of better.

[Jaykul]

Consider the following code:

function Invoke-TSKill {
    [CmdletBinding()]
    param()
    if ($PSCmdlet.ShouldProcess("Stop MyProcess")) {
       TSKill "MyProcess"
    }
}

function Invoke-StopMyProcess {
    [CmdletBinding(SupportsShouldProcess)]
    param()

    Invoke-TSKill
}


function Stop-MyProcess {
    [CmdletBinding(SupportsShouldProcess=$false)]
    param()
    Invoke-StopMyProcess
}

ScriptAnalyzer output

RuleName                            Severity     FileName   Line  Message
--------                            --------     --------   ----  -------
PSShouldProcess                     Warning                 18    'Stop-MyProcess' calls ShouldProcess/ShouldContinue but
                                                                  does not have the ShouldProcess attribute.
PSUseShouldProcessForStateChangingF Warning                 18    Function ’Stop-MyProcess’ has verb that could change system
unctions                                                          state. Therefore, the function has to support
                                                                  'ShouldProcess'.

The intent of #608 and #521 was that you should not throw an error when a function chooses to support ShouldProcess in order to pass the ConfirmPreference through to another function.

As of 1.8.1, this is indeed fixed, and PSScriptAnalyzer does not warn about Invoke-StopMyProcess. Congratulations?

You've gone too far...

1. Why is there no warning on Invoke-TSKill?

Obviously this function needs to have SupportsShouldProcess on it, because it actually does call ShouldPocess without having SupportsShouldProcess on the CmdletBinding attribute.

2. Why does the warning refer to a "ShouldProcess attribute"?

'Stop-MyProcess' calls ShouldProcess/ShouldContinue but does not have the ShouldProcess attribute.

The fix is to add SupportsShouldProcess to the [CmdletBinding()] attribute. This is just confusing.

3. Why am I getting a warning about Stop-MyProcess at all?

The warning claims that I'm calling ShouldProcess or ShouldContinue, but I am not. This simple repro case is easy to follow, but in the real world, this warning happens on scripts with hundreds of lines of code and the actual ShouldProcess is probably happening in one of the functions I call, but I don't know which. Since you know which, you should say which.

Frankly, I should never be forced to pass thru SupportsShouldProcess at all -- there's no reason why I must pass the ShouldProcess support up through my function. This warning should be informational when it's talking about a recursive implementation.

There are lots of reasons why it might not be desirable to passthru.

Additionally, I've explicitly stated that I do not support it. The error does not notice that I've already added the parameter with $false

4. You are still using the verb as an excuse to tell me I nee ShouldProcess

It's ridiculous that I'm getting an extra warning for Stop-MyProcess just because it uses Stop. We've discussed this before #283

[kapilmb]

Why is there no warning on Invoke-TSKill?

This is a bug.

Why does the warning refer to a "ShouldProcess attribute"?

Yes, the error message is misleading.

Why am I getting a warning about Stop-MyProcess at all?

This is a scenario which we didn't take into consideration while writing the rule. The absence of SupportsShouldProcess and the attribute set to $false is treated the same. But, I think this is a valid point - if your function doesn't not support ShouldProcess explicitly then there is no reason to inspect the body for any functions that support ShouldProcess.

Also, as you pointed out, we need to indicate which called function supports ShouldProcess in the violation message.

You are still using the verb as an excuse to tell me I nee ShouldProcess

I do not see any conclusion to the discussion in #283. I think the rule's intent is right but the underlying assumption is too strong - it makes the assumption that if you are using verbs like new, set, remove, etc, you are changing the system's state. This is a very strong assumption which is not valid is many scenarios and I understand the frustration over the overzealous nature of the rule. I think the right thing to do in this case to perhaps lower its severity level to information and change the error message to suggest supporting ShouldProcess only if the function actually changes the system's state.

Thank you very much for finding and reporting the shortcomings of the PSShouldProcess rule. This will be immensely helpful in refining the rule.

[Jaykul]

You really can't require people to deal with ShouldProcess just because a cmdlet they call supports it -- and I don't think authors should have to explicitly set SupportsShouldProcess=$false just to indicate that they're not supporting it.

The way this rule is currently implemented, if I call any third party command, then that third party command can "break" my module by just adding a command attribute later.

That is, if a command adds support for ShouldProcess at some point, I'll suddenly start failing my PSScriptAnalyzer rules. That's not acceptable. The original request which I believe led to this change was to make it so that

• if I say I SupportShouldProcess, you would:
• not fail me for
• not calling ShouldProcess
• as long as one of the commands I call does call ShouldProcess (and/or also SupportShouldProcess).

But the new implementation also

• If I do not say that I SupportShouldProcess
• it fails me for
• any time one of the commands I call does call ShouldProcess.

Do not want. 😉

Basically, you should recursively look to not fail me, but you should not recursively look to fail me.

[kapilmb]

Thanks for the elaboration. I think now I understand the issue better. The rule should not be triggered if a function calls a command that supports ShouldProcess but the function itself does not support ShouldProcess. As such, #646 is the same issue where the function there calls foreach-object and it gets flagged for not supporting ShouldProcess.