Skip to content

UseCmdletCorrectly : This rule only fires for the cmdlets / functions loaded in the default runspace #883

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
LaurentDardenne opened this issue Feb 10, 2018 · 6 comments

Comments

@LaurentDardenne
Copy link

 $sb={
 Function Get-MyCommand {
   param( 
    [Parameter(Mandatory=$true,Position=1)] 
   $ParamMandatory,
   
    [Parameter(Position=2)]
   $B,
   
    [Parameter(Position=3)]
   $C
  )
  "Test"
  }
  Get-MyCommand
  Write-verbose
 }
 Invoke-ScriptAnalyzer -ScriptDefinition "$sb"

This rule does not fire because it only calls the GetCommandInfo() method.

The test file does not contain this use case.

@bergmeister
Copy link
Collaborator

Even if this got implemented in the future, it would probably be limited to one scope and functions defined in the same file because PSSA analyses the files one by one. Loading all scripts into memory would be a huge memory and performance impact. Maybe a possible workaround would be to allow it to sniff the defined functions in the current runspace, which would add the cmdlets of the installed modules or require the user to import the modules on which the files to be analysed depend on before running PSSA. This would allow the following to throw a UseCmdletCorrectly warning Invoke-ScriptAnalyzer -ScriptDefinition 'Invoke-ScriptAnalyzer'. However, this is not going to work out of the box in the PowerShell extension of VSCode, which has its own pwsh, therefore some code would be needed to get the modules folder via the environment variable and then load up and then load an instance of PowerShell itself, which is quite expensive. Maybe someone else can think of a better design idea?
I think for now a note in the documentation is the best short term action.

@LaurentDardenne
Copy link
Author

it would probably be limited to one scope and functions defined in the same file
Yes, I do not ask for more, but not less :-)

Maybe someone else can think of a better design idea?
The UseShouldProcessCorrectly rule builds a graph of the commands declared in the current ast.

Unfortunately this class is private, if in a script module I write a rule needing this type of search, I have to extract it and insert it into my module or a dll.
It is regrettable.

Several rules existing or request could also benefit from this class.

@bergmeister
Copy link
Collaborator

@LaurentDardenne Thanks for the pointer to UseShouldProcessCorrectly, this seems to be a good way to include locally defined functions, very useful especially since I myself have not written the codebase and know only the things which I have touched. You seem to know the codebase quite well in general, we'd appreciate it if you could make a PR for some of your raised issues but there is no rush so feel free to take your time. Just in general, since we both seem to be pretty active in this repo: I myself am also only a community member that helps to maintain PSSA and I try to balance my work 50-50 between PRs that I myself would find really useful and PRs about general PSSA issues. As you can see, there are quite a few issues in PSSA that mostly have to be approached on a priority order so I cannot guarantee that I can resolve all your issues in general now, some of them I will take a look at and add to my own backlog of things to do when I have some spare time but others might take more time to get resolved.
According to Kapils commente here it seems the rule at least used to check also installed cmdlets, which is something that the rule should also cover because at the moment Invoke-ScriptAnalyzer -ScriptDefinition 'Invoke-ScriptAnalyzer' does not trigger a warning either.

@LaurentDardenne
Copy link
Author

You seem to know the codebase quite well in general
Not really, I read it a lot to understand how to implement a prototype (module script).

but others might take more time to get resolved.
I know, I'm waiting for the resolution of a bug since September 2016.

at the moment Invoke-ScriptAnalyzer -ScriptDefinition 'Invoke-ScriptAnalyzer' does not trigger a warning either.
Yes but this case is related to code dependencies, if it is not defined in the AST to analyze I do not try to know it.
This only works with runtime cmdlets that are loaded into the session.
To load in the runspace the prerequisites to the analyzed code would cover more cases.

it if you could make a PR for some of your raised issues
I do not have time for the moment.
Why not post a request for help here?

@LaurentDardenne
Copy link
Author

In addition :
In the event of a function name collision (example: new-module), the error message does not reference a qualified name (Microsoft.PowerShell.Core \ new-module).
In my case, I spent a few minutes before realizing that the message spoke of another function than mine (present in a module) ...

@JamesWTruher
Copy link
Contributor

there's an additional issue with UseCmdletCorrectly, it will only fire if the number of mandatory parameters is equal or greater than the number of parameter sets, so while it should report an issue with remove-item in isolation it does not because there is only one mandatory parameter but 2 parameter sets.

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