New rule - ReviewUnusedParameter #1382

mattmcnabb · 2019-12-10T05:32:53Z

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.

mattmcnabb · 2019-12-10T20:55:58Z

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

bergmeister · 2020-01-03T21:23:56Z

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)

bergmeister

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

mattmcnabb · 2020-01-03T22:05:11Z

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!

Rules/ReviewUnusedParameter.cs

rjmholt · 2020-01-03T22:27:48Z

Rules/ReviewUnusedParameter.cs

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


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

@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!

Rules/ReviewUnusedParameter.cs

Rules/Strings.resx

Tests/Rules/ReviewUnusedParameter.tests.ps1

bergmeister · 2020-01-10T07:53:38Z

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

bergmeister · 2020-01-13T09:43:37Z

@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

mattmcnabb · 2020-01-13T21:22:11Z

@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

Rules/ReviewUnusedParameter.cs

bergmeister · 2020-01-13T23:27:20Z

@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?

bergmeister · 2020-01-14T00:04:24Z

Close and re-open to rerun ci

rjmholt

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

rjmholt · 2020-01-14T00:22:32Z

Tests/Rules/ReviewUnusedParameter.tests.ps1

@@ -57,7 +57,13 @@ Describe "ReviewUnusedParameter" {
        }

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


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.

bergmeister · 2020-01-14T19:10:45Z

Rules/ReviewUnusedParameter.cs

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


@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

mattmcnabb · 2020-01-15T03:35:43Z

Well, it looks like I'm no longer needed here...

I'ma head to bed now - let me know if you guys need anything 🤣

mattmcnabb · 2020-01-17T14:01:21Z

@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.

New rule - ReviewUnusedParameter

929ae14

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

bergmeister added the Issue - New Rule label Jan 3, 2020

bergmeister self-assigned this Jan 3, 2020

bergmeister self-requested a review January 3, 2020 21:24

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

bergmeister approved these changes Jan 3, 2020

View reviewed changes

bergmeister requested review from JamesWTruher and rjmholt January 3, 2020 21:54

rjmholt reviewed Jan 3, 2020

View reviewed changes

Style fixes for PR #1382

938a283

rjmholt reviewed Jan 6, 2020

View reviewed changes

Tests/Rules/ReviewUnusedParameter.tests.ps1 Show resolved Hide resolved

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

rjmholt reviewed Jan 13, 2020

View reviewed changes

Rules/ReviewUnusedParameter.cs Outdated Show resolved Hide resolved

case insensitive dictionary and add test case

9f61f2e

bergmeister closed this Jan 14, 2020

bergmeister reopened this Jan 14, 2020

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

bergmeister approved these changes Jan 14, 2020

View reviewed changes

bergmeister reviewed Jan 14, 2020

View reviewed changes

bergmeister merged commit b8fec67 into PowerShell:master Jan 15, 2020

bergmeister mentioned this pull request May 5, 2020

ReviewUnusedParameter does not capture parameter usage within a scriptblock #1472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New rule - ReviewUnusedParameter #1382

New rule - ReviewUnusedParameter #1382

mattmcnabb commented Dec 10, 2019 •

edited

Loading

mattmcnabb commented Dec 10, 2019 •

edited

Loading

bergmeister commented Jan 3, 2020

bergmeister left a comment

mattmcnabb commented Jan 3, 2020

rjmholt Jan 3, 2020

mattmcnabb Jan 5, 2020

rjmholt Jan 6, 2020

mattmcnabb Jan 9, 2020 •

edited

Loading

bergmeister commented Jan 10, 2020

bergmeister commented Jan 13, 2020 •

edited

Loading

mattmcnabb commented Jan 13, 2020

bergmeister commented Jan 13, 2020 •

edited

Loading

bergmeister commented Jan 14, 2020

rjmholt left a comment

rjmholt Jan 14, 2020

bergmeister Jan 14, 2020

rjmholt Jan 14, 2020

bergmeister Jan 14, 2020

rjmholt Jan 14, 2020 •

edited

Loading

mattmcnabb commented Jan 15, 2020

mattmcnabb commented Jan 17, 2020

New rule - ReviewUnusedParameter #1382

New rule - ReviewUnusedParameter #1382

Conversation

mattmcnabb commented Dec 10, 2019 • edited Loading

PR Summary

PR Checklist

mattmcnabb commented Dec 10, 2019 • edited Loading

bergmeister commented Jan 3, 2020

bergmeister left a comment

Choose a reason for hiding this comment

mattmcnabb commented Jan 3, 2020

rjmholt Jan 3, 2020

Choose a reason for hiding this comment

mattmcnabb Jan 5, 2020

Choose a reason for hiding this comment

rjmholt Jan 6, 2020

Choose a reason for hiding this comment

mattmcnabb Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

bergmeister commented Jan 10, 2020

bergmeister commented Jan 13, 2020 • edited Loading

mattmcnabb commented Jan 13, 2020

bergmeister commented Jan 13, 2020 • edited Loading

bergmeister commented Jan 14, 2020

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt Jan 14, 2020

Choose a reason for hiding this comment

bergmeister Jan 14, 2020

Choose a reason for hiding this comment

rjmholt Jan 14, 2020

Choose a reason for hiding this comment

bergmeister Jan 14, 2020

Choose a reason for hiding this comment

rjmholt Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

mattmcnabb commented Jan 15, 2020

mattmcnabb commented Jan 17, 2020

mattmcnabb commented Dec 10, 2019 •

edited

Loading

mattmcnabb commented Dec 10, 2019 •

edited

Loading

mattmcnabb Jan 9, 2020 •

edited

Loading

bergmeister commented Jan 13, 2020 •

edited

Loading

bergmeister commented Jan 13, 2020 •

edited

Loading

rjmholt Jan 14, 2020 •

edited

Loading