Skip to content

PSShouldProcess false positive on function inside of a begin/process/end function #521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rkeithhill opened this issue May 4, 2016 · 5 comments
Assignees

Comments

@rkeithhill
Copy link
Contributor

It isn't uncommon to have helper functions inside the begin function to process items. These functions can use $pscmdlet.ShouldProcess() but because they are not marked with the [CmdletBinding(SupportsShouldProcess)] this rule fires.

Here's a simple repro:

function Foo {
   [CmdletBinding(SupportsShouldProcess)]
   param()

   begin {
       function helper {
           if ($PSCmdlet.ShouldProcess('','') {

           }
       }
   }
}
@kapilmb
Copy link

kapilmb commented May 10, 2016

@rkeithhill This is part of a bigger issue - analyzing dependencies within and/or across PowerShell scrips/modules - that PSScriptAnalyzer doesn't address as of yet.

How common is this pattern of using $pscmdlet.shouldprocess in a helper function? If such a feature were to exist would it reduce the false alarm rate by a significant amount? Also, would RuleSuppression be helpful in reducing these kinds of false alarms?

@rkeithhill
Copy link
Contributor Author

I can't speak for other modules but I use it commonly enough. Let me try the suppressing it. I think I tried once by putting the attribute on the outermost function but it didn't suppress. I just tried with the latest version of PSScriptAnalyzer and that is working now.

So that is a reasonable workaround for now. But I do think it should be fixed. Seems like you would just examine the context of where the function is defined. If defined inside the context of an advanced function, then if that adv function has the SupportsShouldProcess attribute then the current function is covered. Easy peasy, right? :-) I know, it's never that easy.

@kapilmb
Copy link

kapilmb commented May 10, 2016

I agree that it should be fixed. But, do we just examine the context of the function? What if someone if dot-sourcing the helper function? What is the level of nesting that we should examine? There may be other cases that I am probably missing but I think this raises more questions and we need more discussion around this issue on how do we handle such situations in general. But of course, we can just go ahead and start with examining the context of the function to see if it is defined in an advanced function.

@rkeithhill
Copy link
Contributor Author

rkeithhill commented May 13, 2016

But, do we just examine the context of the function?

I think that is the best place to start as it will cover the vast majority of cases. I often have helper functions defined within a begin function. And I often dot source scripts. But I never dot source at that level. That is, I almost always dot source at the script/module level and rarely (never?) within the body of a function.

@kilasuit
Copy link
Contributor

Also reported in #608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants