From 67d1620d331e0b84a34b264be9f539d66fcf683d Mon Sep 17 00:00:00 2001 From: Manigandan Jegannathan Date: Fri, 22 May 2020 19:34:31 +0400 Subject: [PATCH 1/6] Enhance ReviewUnusedParameter to bail out if $MyInvocation.BoundParmaeters encountered --- Rules/ReviewUnusedParameter.cs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 5a06500d8..87dfb45ca 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -51,6 +51,29 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) continue; } + // bail out if $MyInvocation.BoundParameters used + if (variableCount.ContainsKey("MyInvocation")) + { + bool isBound = false; + IEnumerable memExpAst = scriptBlockAst.FindAll(oneAst => oneAst is MemberExpressionAst, false); + foreach (MemberExpressionAst memberExpressionAst in memExpAst) + { + if (memberExpressionAst.Expression is VariableExpressionAst && + ((VariableExpressionAst)memberExpressionAst.Expression).VariablePath.UserPath == "MyInvocation" && + memberExpressionAst.Member is StringConstantExpressionAst && + ((StringConstantExpressionAst)memberExpressionAst.Member).Value == "BoundParameters") + { + isBound = true; + break; + } + } + + if (isBound) + { + continue; + } + } + foreach (ParameterAst parameterAst in parameterAsts) { // there should be at least two usages of the variable since the parameter declaration counts as one From 02bb9d02cda34d4cc3cf9d4fee0d76ae9af3a5b8 Mon Sep 17 00:00:00 2001 From: Manigandan Jegannathan Date: Sun, 24 May 2020 09:59:31 +0400 Subject: [PATCH 2/6] Account for $PSCmdlet.MyInvocation.BoundParameters --- Rules/ReviewUnusedParameter.cs | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 87dfb45ca..526988ced 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -51,21 +51,28 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) continue; } - // bail out if $MyInvocation.BoundParameters used - if (variableCount.ContainsKey("MyInvocation")) + // bail out if $MyInvocation.BoundParameters or $PSCmdlet.MyInvocation.BoundParameters used + if (variableCount.ContainsKey("MyInvocation") || variableCount.ContainsKey("PSCmdlet")) { bool isBound = false; IEnumerable memExpAst = scriptBlockAst.FindAll(oneAst => oneAst is MemberExpressionAst, false); foreach (MemberExpressionAst memberExpressionAst in memExpAst) - { - if (memberExpressionAst.Expression is VariableExpressionAst && - ((VariableExpressionAst)memberExpressionAst.Expression).VariablePath.UserPath == "MyInvocation" && - memberExpressionAst.Member is StringConstantExpressionAst && - ((StringConstantExpressionAst)memberExpressionAst.Member).Value == "BoundParameters") + { + if (memberExpressionAst.Member is StringConstantExpressionAst sceAst && sceAst.Value == "BoundParameters") { - isBound = true; - break; - } + // handle both regular and nested scenarios + if ((memberExpressionAst.Expression is VariableExpressionAst veAst && + veAst.VariablePath.UserPath == "MyInvocation") || + (memberExpressionAst.Expression is MemberExpressionAst meAst && + meAst.Expression is VariableExpressionAst veAstNested && + veAstNested.VariablePath.UserPath == "PSCmdlet" && + meAst.Member is StringConstantExpressionAst sceAstNested && + sceAstNested.Value == "MyInvocation")) + { + isBound = true; + break; + } + } } if (isBound) From 1720e7139bf11228c29415a23c610a74cd038d1b Mon Sep 17 00:00:00 2001 From: Manigandan Jegannathan Date: Sun, 24 May 2020 22:52:30 +0400 Subject: [PATCH 3/6] Optmised the way we find bound parameters reference with in script block --- Rules/ReviewUnusedParameter.cs | 70 +++++++++++++++++----------------- 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 526988ced..38e0574c7 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -36,6 +36,12 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) { + // bail out if PS bound parameter used. + if (scriptBlockAst.FindAll(IsBoundParametersReference, false).Any()) + { + continue; + } + // find all declared parameters IEnumerable parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false); @@ -45,42 +51,6 @@ public IEnumerable 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; - } - - // bail out if $MyInvocation.BoundParameters or $PSCmdlet.MyInvocation.BoundParameters used - if (variableCount.ContainsKey("MyInvocation") || variableCount.ContainsKey("PSCmdlet")) - { - bool isBound = false; - IEnumerable memExpAst = scriptBlockAst.FindAll(oneAst => oneAst is MemberExpressionAst, false); - foreach (MemberExpressionAst memberExpressionAst in memExpAst) - { - if (memberExpressionAst.Member is StringConstantExpressionAst sceAst && sceAst.Value == "BoundParameters") - { - // handle both regular and nested scenarios - if ((memberExpressionAst.Expression is VariableExpressionAst veAst && - veAst.VariablePath.UserPath == "MyInvocation") || - (memberExpressionAst.Expression is MemberExpressionAst meAst && - meAst.Expression is VariableExpressionAst veAstNested && - veAstNested.VariablePath.UserPath == "PSCmdlet" && - meAst.Member is StringConstantExpressionAst sceAstNested && - sceAstNested.Value == "MyInvocation")) - { - isBound = true; - break; - } - } - } - - if (isBound) - { - continue; - } - } - foreach (ParameterAst parameterAst in parameterAsts) { // there should be at least two usages of the variable since the parameter declaration counts as one @@ -102,6 +72,34 @@ meAst.Member is StringConstantExpressionAst sceAstNested && } } + /// + /// Checks for PS bound parameter reference. + /// + /// + /// + private static bool IsBoundParametersReference(Ast ast) + { + bool isBound = false; + // if $MyInvocation.BoundParameters or $PSCmdlet.MyInvocation.BoundParameters used. + if (ast is MemberExpressionAst memberAst + && memberAst.Member is StringConstantExpressionAst memberStringAst + && memberStringAst.Value.Equals("BoundParameters", StringComparison.OrdinalIgnoreCase)) + { + // handle both regular and nested scenarios. + isBound = (memberAst.Expression is VariableExpressionAst veAst + && veAst.VariablePath.UserPath.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase)) + || (memberAst.Expression is MemberExpressionAst meAst + && meAst.Expression is VariableExpressionAst veAstNested + && veAstNested.VariablePath.UserPath.Equals("PSCmdlet", StringComparison.OrdinalIgnoreCase) + && meAst.Member is StringConstantExpressionAst sceAstNested + && sceAstNested.Value.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase)); + } + + return isBound + || ast is VariableExpressionAst variableAst + && variableAst.VariablePath.UserPath.Equals("PSBoundParameters", StringComparison.OrdinalIgnoreCase); + } + /// /// GetName: Retrieves the name of this rule. /// From f512390f575e579c5c9a2c2e8231a4fc085e19ae Mon Sep 17 00:00:00 2001 From: Manigandan Jegannathan Date: Sun, 31 May 2020 22:43:26 +0400 Subject: [PATCH 4/6] Address review feedback --- Rules/ReviewUnusedParameter.cs | 47 +++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 38e0574c7..2f3a842db 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -37,7 +37,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) { // bail out if PS bound parameter used. - if (scriptBlockAst.FindAll(IsBoundParametersReference, false).Any()) + if (scriptBlockAst.Find(IsBoundParametersReference, false) != null) { continue; } @@ -75,29 +75,46 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// /// Checks for PS bound parameter reference. /// - /// - /// + /// AST to be analyzed. This should be non-null + /// Boolean true indicating that given AST has PS bound parameter reference, otherwise false private static bool IsBoundParametersReference(Ast ast) { - bool isBound = false; - // if $MyInvocation.BoundParameters or $PSCmdlet.MyInvocation.BoundParameters used. + // $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)) { - // handle both regular and nested scenarios. - isBound = (memberAst.Expression is VariableExpressionAst veAst + MemberExpressionAst meAst = (MemberExpressionAst)ast; + + // $MyInvocation.BoundParameters + if (meAst.Expression is VariableExpressionAst veAst && veAst.VariablePath.UserPath.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase)) - || (memberAst.Expression is MemberExpressionAst meAst - && meAst.Expression is VariableExpressionAst veAstNested - && veAstNested.VariablePath.UserPath.Equals("PSCmdlet", StringComparison.OrdinalIgnoreCase) - && meAst.Member is StringConstantExpressionAst sceAstNested - && sceAstNested.Value.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase)); + { + return true; + } + + // $PSCmdlet.MyInvocation.BoundParameters + if (meAst.Expression is MemberExpressionAst) + { + MemberExpressionAst meAstNested = (MemberExpressionAst)meAst.Expression; + + 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 isBound - || ast is VariableExpressionAst variableAst - && variableAst.VariablePath.UserPath.Equals("PSBoundParameters", StringComparison.OrdinalIgnoreCase); + return false; } /// From 09c0cf7dd3d8e096cb2af47ed4b01d5747abd6e7 Mon Sep 17 00:00:00 2001 From: Manigandan Jegannathan Date: Mon, 8 Jun 2020 19:27:50 +0400 Subject: [PATCH 5/6] Remove unnecessary casting --- Rules/ReviewUnusedParameter.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 2f3a842db..a1336aed7 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -90,20 +90,16 @@ private static bool IsBoundParametersReference(Ast ast) && memberAst.Member is StringConstantExpressionAst memberStringAst && memberStringAst.Value.Equals("BoundParameters", StringComparison.OrdinalIgnoreCase)) { - MemberExpressionAst meAst = (MemberExpressionAst)ast; - // $MyInvocation.BoundParameters - if (meAst.Expression is VariableExpressionAst veAst + if (memberAst.Expression is VariableExpressionAst veAst && veAst.VariablePath.UserPath.Equals("MyInvocation", StringComparison.OrdinalIgnoreCase)) { return true; } // $PSCmdlet.MyInvocation.BoundParameters - if (meAst.Expression is MemberExpressionAst) + if (memberAst.Expression is MemberExpressionAst meAstNested) { - MemberExpressionAst meAstNested = (MemberExpressionAst)meAst.Expression; - if (meAstNested.Expression is VariableExpressionAst veAstNested && veAstNested.VariablePath.UserPath.Equals("PSCmdlet", StringComparison.OrdinalIgnoreCase) && meAstNested.Member is StringConstantExpressionAst sceAstNested From 7c37cf46389078e25079f1eafbc1e2d443ff393a Mon Sep 17 00:00:00 2001 From: Manigandan Jegannathan Date: Sat, 13 Jun 2020 22:57:58 +0400 Subject: [PATCH 6/6] - Label boolean parameter - Add pester tests --- Rules/ReviewUnusedParameter.cs | 2 +- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index a1336aed7..ffaaa1334 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -37,7 +37,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) { // bail out if PS bound parameter used. - if (scriptBlockAst.Find(IsBoundParametersReference, false) != null) + if (scriptBlockAst.Find(IsBoundParametersReference, searchNestedScriptBlocks: false) != null) { continue; } diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index f5d6004cc..85d27d731 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -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