New Rule to detect unused parameters

[mattmcnabb]

Summary of the new feature

This idea came from a sidebar comment from @thomasrayner in #1308. The UseDeclaredVarsMoreThanAssignments rule currently warns for variables that are declared but never used. If this is a valuable rule then it follows that we would want to check for parameters that are declared and never used.

Essentially I'd like to see if anyone else considers this valuable based on the proposed initial implementation below, or if you can think of edge cases or roadblocks I might not have thought of.

Proposed technical implementation details (optional)

I have a POC rule that is working at a basic level:

// find all declared parameters
IEnumerable<Ast> parameterAsts = ast.FindAll(a => a is ParameterAst, true);

// list all variables
List<string> variables = ast.FindAll(a => a is VariableExpressionAst, true)
	.Cast<VariableExpressionAst>()
	.Select(v => v.VariablePath.ToString())
	.ToList();

foreach (ParameterAst parameterAst in parameterAsts)
{
	// 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
		.Where(x => x == parameterAst.Name.VariablePath.ToString())
		.Count();
	if (matchCount > 1)
	{
		continue;
	}

	// all bets are off if the script uses PSBoundParameters
	if (variables.Contains("PSBoundParameters"))
	{
		continue;
	}

	yield return new DiagnosticRecord...
}

I think this approach could work but there are definitely some tricky usages to work around:

1. a parameter could be used explicitly or splatted using $PSBoundParameters or $PSCmdlet.MyInvocation.BoundParameters. One way around this is to simply check if either of those are used in the scripblock, but this isn't a guarantee as the bound parameters collection could be modified prior to its use. Of course, we could also check that the Remove() method has not been called on either of these collections.

2. We'd need to check for the use of Get-Variable as well.

I'm sure there are other cases I'm not aware of but will discover in testing.

What is the latest version of PSScriptAnalyzer at the point of writing
1.8.3

[bergmeister]

Quite often a function is used from somewhere else in a different file (or even externally) but PSSA analyzes each file separately, therefore I am struggling to understand how this rule should work.

[mattmcnabb]

@bergmeister it would just look for declared parameters in a script or function that are never used in the scope of that same function or script:

Function Get-Foo {
Param ($MyParam)

}

The UseDeclaredVarsMoreThanAssignments pretty much functions the same way.

[bergmeister]

Oooh,, gotcha, my bad, at first I thought you were referring to parameter usage of the caller of the function. Yes, that makes sense. I think we can neglect psboundparameters and get-variable usage for the first simple version but I think splatting needs to be included as it is quite common (i.e. function foo($MyParam) { Initialize; Get-Bar @MyParam } )

[mattmcnabb]

No worries!

I'll get to work on this one, but a PR might not come immediately as this one is a little tricky to get right. The POC above doesn't yet account for scope of the variable, so if the variable is encountered anywhere in the script it won't trip the diag record. I think I'll solve that just by looping through all scriptblocks and just looking under that scope.

I think splatting might be easy to look for, since there are probably only a couple ways it would normally be done:

$Splat = @{
    SomeValue = $MyParam
}

Get-Something @Splat

or

Get-Something @PSBoundParameters

Thanks!

[bergmeister]

Yes, awesome. Take your time.
The scoping issue is a difficult one due to PowerShell's dynamic scoping. I think the best answer is to keep it simple. The UseDeclaredVarsMoreThanAssignments rule for example works only in the context/scope of one scriptblock.

[mattmcnabb]

Yes, awesome. Take your time.
The scoping issue is a difficult one due to PowerShell's dynamic scoping. I think the best answer is to keep it simple. The UseDeclaredVarsMoreThanAssignments rule for example works only in the context/scope of one scriptblock.

yeah, that's probably why I usually keep that one turned off 🤣

[mattmcnabb]

One other thing - what's a good name for this rule? I'm currently just using DeclareUnusedParameter but that seems off. I haven't really been able to determine a hard naming scheme for these rules. UseDeclaredVarsMoreThanAssignments is awful BTW, but that one has been baked in for a long time so I don't think it's going anywhere.

[bergmeister]

ReviewUnusedParameter maybe similar to the C# one? https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1801?view=vs-2019

[mattmcnabb]

I'm fine with that, although I don't think any of the current rules are named similar to C# rules. I could only suggest UseDeclaredParameter or AvoidUnusedParameter based on current rule names. I'll go with your suggestion for now.

[bergmeister]

I like to compare to C# because their language has much bigger reach, meaning much more people have already spent brainpower on a good name. I prefer the review prefix because it could be either that the user did not use the parameter or the parameter is not needed any more. Therefore we do not know if it is a use or avoid scenario.

[SydneyhSmith]

Closing as resolved as it looks like the PR has been merged...let me know if there is an additional request to track here that has not been resolved.

[bergmeister]

Closing is fine. I didn't check @mattmcnabb but if you use a closing keyword the next time, merging the PR would auto-close it:
https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords