New rule - ReviewUnusedParameter by mattmcnabb · Pull Request #1382 · PowerShell/PSScriptAnalyzer
Conversation
PR Summary
This PR adds a new rule to detect declared parameters that are not used in the body of the script, function or scriptblock where they are declared. Conversation around this is in #1381.
I've added tests to cover the basic scenarios this will cover, but there may be some cases not covered yet. Some issues to discuss are:
- This currently looks only in the same scope for usages of the parameter and not in child scopes. Not sure if that's worth looking into as calling the parameter in a child scope may not be good practice. I included a skipped test for future reference.
- This doesn't look for instances of Get-Variable referencing the parameter.
- This will not flag parameters in a scriptblock where
$PSBoundParametersis called.
If these are not major issues then this may be ready for merge.
PR Checklist
- PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- Summarized changes
- Change is not breaking
- Make sure all
.cs,.ps1and.psm1files have the correct copyright header - Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
- This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix
WIP:to the beginning of the title and remove the prefix when the PR is ready.
- If the PR is work in progress, please add the prefix
Hmm.... did adding this rule break the tests for AvoidAssignmentToAutomaticVariables? At a glance it looks like those tests need to use IncludeRules rather than ExcluedRules, but I have no idea why that would only become a problem now.
…the new rule as the parameter is not used in those tests
Those tests have function definitions that do not use their parameters, I simply excluded this rule for those tests. It's always a difficult debate whether the tests should test the rules individually or run all rules. There's pros and cons against either but James suggested to run all rules if possible because it provides better integration coverage, especially due to PSSA's multi threaded nature where complex things can happen if all rules execute at once
…ToList() and minor style tweaks (Linq variable naming and trailing whitespace trim)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the other test failures that were not your fault and made some small tweaks. Very happy with this PR and one of the main reasons why I am so eager to come back and get this PR merged is because I am really looking forward to using it
I fixed the other test failures that were not your fault and made some small tweaks. Very happy with this PR and one of the main reasons why I am so eager to come back and get this PR merged is because I am really looking forward to using it
Thanks for the fixes. I'm eager to get this in there as well!
| { | ||
| // compare the list of variables to the parameter name | ||
| // there should be at least two matches of the variable name since the parameter declaration counts as one | ||
| int matchCount = variables |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, the algorithm becomes O(n^2) in the number of parameters, which is undesirable.
Instead, it should be possible to do the following:
- Create an empty case insensitive hashset of variable names
- Collect all the variable names used in the begin, process and end blocks of the script block
- Run through the parameters and warn for each one not found in the hashset
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjmholt Forgive me, but I'm failing to see how that will reduce the complexity. Isn't the current code doing essentially what you've described?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It almost does, but currently:
- The variable lookup is O(n) in the number of variables. The more variables that occur in a script, the longer it will take to verify that parameter is used in that script. Worse, we must look through the whole list of variables for each parameter. With particularly large scripts, the latency of that will start to hit (visible particularly in the VSCode scenario). Instead, we should use an O(1) lookup data structure like a
HashSet<string> - Variable name comparison is currently case sensitive (done with
.Where(variable => variable == parameterAst.Name.VariablePath.ToString())). Instead it should be case insensitive. This is easy to do withnew HashSet<string>(StringComparer.OrdinalIgnoreCase).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's definitely case-sensitive at the moment. I'll take a look at implementing the hashset - I think I can come up with something. Thanks!
@mattmcnabb It seems we have a merge conflict here (probably just a simple one like the number of rules), please resolve
@mattmcnabb In your last commit, you resolved the merge conflict by taking HEAD, therefore removing your changes. I've done this for you in your branch
Christoph Bergmeister added 2 commits
@mattmcnabb In your last commit, you resolved the merge conflict by taking
HEAD, therefore removing your changes. I've done this for you in your branch
well that was rookie!
@rjmholt @mattmcnabb I pushed one last commit to use .OrderBy and convert it to a dictionary to get a dictionary of the variable count so that we count only once and finding the variable count then in just O(1) after the expense of OrderBy. Given that the number of parameters is only between 1 and 10 in 95% of the cases, I am not sure this is really worth it but probably helps in extreme cases where there are much more auto-generated parameters. What do you think?
Close and re-open to rerun ci
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Just left a comment on the test, but it's an edge case so not blocking.
|
|
||
| It "has no violations when parameter is called in child scope" -skip { | ||
| $ScriptDefinition = 'function Param { param ($Param1) function Child { $Param1 } }' | ||
| $ScriptDefinition = 'function foo { param ($Param1) function Child { $Param1 } }' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think of this as a violation. Since PowerShell has dynamic, rather than lexical, scope, Child's $Param1 reference is not guaranteed to be foo's $Param1 parameter.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather avoid violations that are going to be false positives in most cases in order to avoid similar problems to PSUseDeclaredVarsMoreThanAssignments
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fair enough we want to not emit when at the boundary of our heuristic. Ideally we'd change this to an actual false case, but it's not that important.
Christoph Bergmeister added 2 commits
| IDictionary<string, int> variableCount = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) | ||
| .Select(variableExpressionAst => ((VariableExpressionAst)variableExpressionAst).VariablePath.UserPath) | ||
| .GroupBy(variableName => variableName, StringComparer.OrdinalIgnoreCase) | ||
| .ToDictionary(variableName => variableName.Key, variableName => variableName.Count(), StringComparer.OrdinalIgnoreCase); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjmholt Your recently added complex.psm1 test actually caught an edge case that happened when I first added StringComparer.OrdinalIgnoreCase only on .ToDictionary, which caused items of different cases (due to groupby originally being case sensitive) to to be added to the case insensitive dictionary, which then gave the error that the same item had already been added. Therefore I had to add it to GroupBy as well. Chapeau 👏
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me think that PowerShell and other PowerShell tools should have a few real-world test cases too
Well, it looks like I'm no longer needed here...
I'ma head to bed now - let me know if you guys need anything 🤣
@bergmeister @rjmholt thanks for the fixes on this! I wasn't able to devote much time to reviewing this after the start of the new year.