New rule - ReviewUnusedParameter by mattmcnabb · Pull Request #1382 · PowerShell/PSScriptAnalyzer · GitHub

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 $PSBoundParameters is 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, .ps1 and .psm1 files 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.

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

…uding rule

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)

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!

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

Rob Holt (@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?

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 with new HashSet<string>(StringComparer.OrdinalIgnoreCase).

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!

Matt McNabb (@mattmcnabb) It seems we have a merge conflict here (probably just a simple one like the number of rules), please resolve

Matt McNabb (@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

Matt McNabb (@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!

…ormance

Rob Holt (@rjmholt) Matt McNabb (@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

Code looks good. Just left a comment on the test, but it's an edge case so not blocking.

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.

I'd rather avoid violations that are going to be false positives in most cases in order to avoid similar problems to PSUseDeclaredVarsMoreThanAssignments

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.

Rob Holt (@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 👏

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 🤣

Christoph Bergmeister (@bergmeister) Rob Holt (@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.

New rule - ReviewUnusedParameter

929ae14

Christoph Bergmeister (bergmeister) added 3 commits January 3, 2020 21:12

Merge https://github.com/PowerShell/PSScriptAnalyzer

0b5cba3

Fix test failures in AvoidAssignmentToAutomaticVariable by excluding …

d6b38f0

…the new rule as the parameter is not used in those tests

Fix test failures in other tests when parameters are not used by excl…

aaa4f41

…uding rule

Christoph Bergmeister (bergmeister) added the Issue - New Rule label Jan 3, 2020

Christoph Bergmeister (bergmeister) self-assigned this Jan 3, 2020

Christoph Bergmeister (bergmeister) self-requested a review January 3, 2020 21:24

Christoph Bergmeister (bergmeister) added 2 commits January 3, 2020 21:50

make linq query stop earlier for better performance, avoid redundant …

f360fdc

…ToList() and minor style tweaks (Linq variable naming and trailing whitespace trim)

format test file

cdbce04

Christoph Bergmeister (bergmeister) approved these changes Jan 3, 2020

View reviewed changes

Christoph Bergmeister (bergmeister) requested review from James Truher (JamesWTruher) and Rob Holt (rjmholt) January 3, 2020 21:54

Rob Holt (rjmholt) reviewed Jan 3, 2020

View reviewed changes

Style fixes for PR #1382

938a283

Rob Holt (rjmholt) reviewed Jan 6, 2020

View reviewed changes

Comment thread Tests/Rules/ReviewUnusedParameter.tests.ps1

fix merge conflict

79c24c7

Christoph Bergmeister added 2 commits January 13, 2020 09:45

Fix merge error by re-adding original change

67b4846

whitespace tidy

34379ab

Use OrderBy to get a dictionary of the variable Count for better perf…

e8d56c1

…ormance

Rob Holt (rjmholt) reviewed Jan 13, 2020

View reviewed changes

Comment thread Rules/ReviewUnusedParameter.cs Outdated

case insensitive dictionary and add test case

9f61f2e

Christoph Bergmeister (bergmeister) closed this Jan 14, 2020

Christoph Bergmeister (bergmeister) reopened this Jan 14, 2020

Rob Holt (rjmholt) approved these changes Jan 14, 2020

View reviewed changes

Christoph Bergmeister added 2 commits January 14, 2020 18:29

re-trigger ci

00d882d

case insensitive grouping

8c06a41

Christoph Bergmeister (bergmeister) approved these changes Jan 14, 2020

View reviewed changes

Christoph Bergmeister (bergmeister) reviewed Jan 14, 2020

View reviewed changes

Christoph Bergmeister (bergmeister) merged commit b8fec67 into PowerShell:master Jan 15, 2020

Christoph Bergmeister (bergmeister) mentioned this pull request May 5, 2020

ReviewUnusedParameter does not capture parameter usage within a scriptblock #1472

Open

+                              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);

+                              {
+                                  // 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

                       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 } }'

New rule - ReviewUnusedParameter by mattmcnabb · Pull Request #1382 · PowerShell/PSScriptAnalyzer · GitHub

Uh oh!

New rule - ReviewUnusedParameter - #1382

New rule - ReviewUnusedParameter#1382Christoph Bergmeister (bergmeister) merged 14 commits intoPowerShell:masterPowerShell/PSScriptAnalyzer:masterfrom mattmcnabb:masterCopy head branch name to clipboard

Conversation

Matt McNabb (mattmcnabb) commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Matt McNabb (mattmcnabb) commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Christoph Bergmeister (bergmeister) commented Jan 3, 2020

Uh oh!

Christoph Bergmeister (bergmeister) left a comment

Choose a reason for hiding this comment

Uh oh!

Matt McNabb (mattmcnabb) commented Jan 3, 2020

Uh oh!

Uh oh!

Uh oh!

Rob Holt (rjmholt) Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

Matt McNabb (mattmcnabb) Jan 5, 2020

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

Matt McNabb (mattmcnabb) Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Christoph Bergmeister (bergmeister) commented Jan 10, 2020

Uh oh!

Christoph Bergmeister (bergmeister) commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matt McNabb (mattmcnabb) commented Jan 13, 2020

Uh oh!

Uh oh!

Christoph Bergmeister (bergmeister) commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Christoph Bergmeister (bergmeister) commented Jan 14, 2020

Uh oh!

Rob Holt (rjmholt) left a comment

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matt McNabb (mattmcnabb) commented Jan 15, 2020

Uh oh!

Matt McNabb (mattmcnabb) commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

New rule - ReviewUnusedParameter#1382
Christoph Bergmeister (bergmeister) merged 14 commits into
PowerShell:masterfrom
mattmcnabb:master

Matt McNabb (mattmcnabb) commented Dec 10, 2019 •
edited
Loading

Matt McNabb (mattmcnabb) commented Dec 10, 2019 •
edited
Loading

Matt McNabb (mattmcnabb) Jan 9, 2020 •
edited
Loading

Christoph Bergmeister (bergmeister) commented Jan 13, 2020 •
edited
Loading

Christoph Bergmeister (bergmeister) commented Jan 13, 2020 •
edited
Loading

Rob Holt (rjmholt) Jan 14, 2020 •
edited
Loading