Skip to content
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

ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used #1520

Merged
merged 7 commits into from
Jun 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions Rules/ReviewUnusedParameter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)

foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts)
{
// bail out if PS bound parameter used.
if (scriptBlockAst.Find(IsBoundParametersReference, searchNestedScriptBlocks: false) != null)
{
continue;
}

// find all declared parameters
IEnumerable<Ast> parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false);

Expand All @@ -45,12 +51,6 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
.GroupBy(variableName => variableName, StringComparer.OrdinalIgnoreCase)
.ToDictionary(variableName => variableName.Key, variableName => variableName.Count(), StringComparer.OrdinalIgnoreCase);

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

foreach (ParameterAst parameterAst in parameterAsts)
{
// there should be at least two usages of the variable since the parameter declaration counts as one
Expand All @@ -72,6 +72,47 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
}
}

/// <summary>
/// Checks for PS bound parameter reference.
/// </summary>
/// <param name="ast">AST to be analyzed. This should be non-null</param>
/// <returns>Boolean true indicating that given AST has PS bound parameter reference, otherwise false</returns>
private static bool IsBoundParametersReference(Ast ast)
{
// $PSBoundParameters
if (ast is VariableExpressionAst variableAst
&& variableAst.VariablePath.UserPath.Equals("PSBoundParameters", StringComparison.OrdinalIgnoreCase))
{
return true;
}

if (ast is MemberExpressionAst memberAst
&& memberAst.Member is StringConstantExpressionAst memberStringAst
&& memberStringAst.Value.Equals("BoundParameters", StringComparison.OrdinalIgnoreCase))
{
// $MyInvocation.BoundParameters
if (memberAst.Expression is VariableExpressionAst veAst
&& veAst.VariablePath.UserPath.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase))
{
return true;
}

// $PSCmdlet.MyInvocation.BoundParameters
if (memberAst.Expression is MemberExpressionAst meAstNested)
jegannathanmaniganadan marked this conversation as resolved.
Show resolved Hide resolved
{
if (meAstNested.Expression is VariableExpressionAst veAstNested
&& veAstNested.VariablePath.UserPath.Equals("PSCmdlet", StringComparison.OrdinalIgnoreCase)
&& meAstNested.Member is StringConstantExpressionAst sceAstNested
&& sceAstNested.Value.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase))
{
return true;
}
}
}

return false;
}

/// <summary>
/// GetName: Retrieves the name of this rule.
/// </summary>
Expand Down
12 changes: 12 additions & 0 deletions Tests/Rules/ReviewUnusedParameter.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,18 @@ Describe "ReviewUnusedParameter" {
$Violations.Count | Should -Be 0
}

It "has no violations when using MyInvocation.BoundParameters" {
$ScriptDefinition = 'function Bound { param ($Param1) $splat = $MyInvocation.BoundParameters; Get-Foo @splat }'
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
$Violations.Count | Should -Be 0
}

It "has no violations when using PSCmdlet.MyInvocation.BoundParameters" {
$ScriptDefinition = 'function Bound { param ($Param1) $splat = $PSCmdlet.MyInvocation.BoundParameters; Get-Foo @splat }'
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
$Violations.Count | Should -Be 0
}

It "has no violations when parameter is called in child scope" -skip {
$ScriptDefinition = 'function foo { param ($Param1) function Child { $Param1 } }'
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
Expand Down