| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # ReviewUnusedParameter | ||
|
|
||
| **Severity Level: Warning** | ||
|
|
||
| ## Description | ||
|
|
||
| This rule identifies parameters declared in a script, scriptblock, or function scope that have not been used in that scope. | ||
|
|
||
| ## How | ||
|
|
||
| Consider removing the unused parameter. | ||
|
|
||
| ## Example | ||
|
|
||
| ### Wrong | ||
|
|
||
| ``` PowerShell | ||
| function Test-Parameter | ||
| { | ||
| Param ( | ||
| $Parameter1, | ||
|
|
||
| # this parameter is never called in the function | ||
| $Parameter2 | ||
| ) | ||
|
|
||
| Get-Something $Parameter1 | ||
| } | ||
| ``` | ||
|
|
||
| ### Correct | ||
|
|
||
| ``` PowerShell | ||
| function Test-Parameter | ||
| { | ||
| Param ( | ||
| $Parameter1, | ||
|
|
||
| # now this parameter is being called in the same scope | ||
| $Parameter2 | ||
| ) | ||
|
|
||
| Get-Something $Parameter1 $Parameter2 | ||
| } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Management.Automation.Language; | ||
| using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; | ||
| #if !CORECLR | ||
| using System.ComponentModel.Composition; | ||
| #endif | ||
| using System.Globalization; | ||
|
|
||
| namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules | ||
| { | ||
| /// <summary> | ||
| /// ReviewUnusedParameter: Check that all declared parameters are used in the script body. | ||
| /// </summary> | ||
| #if !CORECLR | ||
| [Export(typeof(IScriptRule))] | ||
| #endif | ||
| public class ReviewUnusedParameter : IScriptRule | ||
| { | ||
| public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) | ||
| { | ||
| if (ast == null) | ||
| { | ||
| throw new ArgumentNullException(Strings.NullAstErrorMessage); | ||
| } | ||
|
|
||
| IEnumerable<Ast> scriptBlockAsts = ast.FindAll(oneAst => oneAst is ScriptBlockAst, true); | ||
| if (scriptBlockAsts == null) | ||
| { | ||
| yield break; | ||
| } | ||
|
|
||
| foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) | ||
| { | ||
| // find all declared parameters | ||
| IEnumerable<Ast> parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false); | ||
|
|
||
| // list all variables | ||
| 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); | ||
|
|
||
| // 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 | ||
| variableCount.TryGetValue(parameterAst.Name.VariablePath.UserPath, out int variableUsageCount); | ||
| if (variableUsageCount >= 2) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| yield return new DiagnosticRecord( | ||
| string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterError, parameterAst.Name.VariablePath.UserPath), | ||
| parameterAst.Name.Extent, | ||
| GetName(), | ||
| DiagnosticSeverity.Warning, | ||
| fileName, | ||
| parameterAst.Name.VariablePath.UserPath | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// GetName: Retrieves the name of this rule. | ||
| /// </summary> | ||
| /// <returns>The name of this rule</returns> | ||
| public string GetName() | ||
| { | ||
| return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ReviewUnusedParameterName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// GetCommonName: Retrieves the common name of this rule. | ||
| /// </summary> | ||
| /// <returns>The common name of this rule</returns> | ||
| public string GetCommonName() | ||
| { | ||
| return string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterCommonName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// GetDescription: Retrieves the description of this rule. | ||
| /// </summary> | ||
| /// <returns>The description of this rule</returns> | ||
| public string GetDescription() | ||
| { | ||
| return string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterDescription); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// GetSourceType: Retrieves the type of the rule, builtin, managed or module. | ||
| /// </summary> | ||
| public SourceType GetSourceType() | ||
| { | ||
| return SourceType.Builtin; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// GetSeverity: Retrieves the severity of the rule: error, warning of information. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public RuleSeverity GetSeverity() | ||
| { | ||
| return RuleSeverity.Warning; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// GetSourceName: Retrieves the module/assembly name the rule is from. | ||
| /// </summary> | ||
| public string GetSourceName() | ||
| { | ||
| return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); | ||
| } | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| Describe "ReviewUnusedParameter" { | ||
| BeforeAll { | ||
| $RuleName = 'PSReviewUnusedParameter' | ||
| $RuleSeverity = "Warning" | ||
| } | ||
|
|
||
| Context "When there are violations" { | ||
| It "has 1 violation - function with 1 unused parameter" { | ||
|
Comment thread
mattmcnabb marked this conversation as resolved.
|
||
| $ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) $Param1}' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 1 | ||
| } | ||
|
|
||
| It "has 2 violations - function with 2 unused parameters" { | ||
| $ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) }' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 2 | ||
| } | ||
|
|
||
| It "has 1 violation - scriptblock with 1 unused parameter" { | ||
| $ScriptDefinition = '{ param ($Param1) }' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 1 | ||
| } | ||
|
|
||
| It "doesn't traverse scriptblock scope" { | ||
| $ScriptDefinition = '{ param ($Param1) }; { $Param1 }' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 1 | ||
| } | ||
|
|
||
| It "violations have correct rule and severity" { | ||
| $ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) $Param1}' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Severity | Select-Object -Unique | Should -Be $RuleSeverity | ||
| $Violations.RuleName | Select-Object -Unique | Should -Be $RuleName | ||
| } | ||
| } | ||
|
|
||
| Context "When there are no violations" { | ||
| It "has no violations - function that uses all parameters" { | ||
| $ScriptDefinition = 'function GoodFunc1 { param ($Param1, $Param2) $Param1; $Param2}' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 0 | ||
| } | ||
|
|
||
| It "has no violations - function with splatting" { | ||
| $ScriptDefinition = 'function GoodFunc1 { param ($Param1) $Splat = @{InputObject = $Param1}}' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 0 | ||
| } | ||
|
|
||
| It "has no violations when using PSBoundParameters" { | ||
| $ScriptDefinition = 'function Bound { param ($Param1) Get-Foo @PSBoundParameters }' | ||
| $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 } }' | ||
|
Comment thread
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI 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.
Sorry, something went wrong.
mattmcnabb reacted with thumbs up emoji
All reactions
Copy link
Copy Markdown
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI'd rather avoid violations that are going to be false positives in most cases in order to avoid similar problems to PSUseDeclaredVarsMoreThanAssignments
Sorry, something went wrong.
rjmholt and mattmcnabb reacted with thumbs up emoji
All reactions
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI 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.
Sorry, something went wrong.
All reactions
|
||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 0 | ||
| } | ||
|
|
||
| It "has no violations when case of parameter and variable usage do not match" -skip { | ||
| $ScriptDefinition = 'function foo { param ($Param1, $param2) $param1; $Param2}' | ||
| $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName | ||
| $Violations.Count | Should -Be 0 | ||
| } | ||
| } | ||
| } | ||
| Back | FazBrowse Home | New Git URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityRob 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 👏
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityMakes me think that PowerShell and other PowerShell tools should have a few real-world test cases too
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.