From bba8fed8ebb2e7086d276fe7aceebd2a84e4b856 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister [MVP]" Date: Mon, 19 Aug 2019 22:46:32 +0100 Subject: [PATCH] Enhance UseDeclaredVarsMoreThanAssignments to detect also take into account the usage of Get-Variable with an array of variables and usage of named parameter -Name (#1310) * Parse Array syntax of Get-Variable * check named parameter better to be -Name or shortened versions and add more tests * Fix last edge cases and refactor into method * Apply PR suggestions: Use shorter pattern matching syntax and inline/simplify local method --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 66 ++++++++++++++----- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 40 +++++++++-- 2 files changed, 86 insertions(+), 20 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 6d0f29fdf..5a8440ada 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -208,33 +208,67 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip } } - // Detect usages of Get-Variable + AnalyzeGetVariableCommands(scriptBlockAst, assignmentsDictionary_OrdinalIgnoreCase); + + foreach (string key in assignmentsDictionary_OrdinalIgnoreCase.Keys) + { + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key), + assignmentsDictionary_OrdinalIgnoreCase[key].Left.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + key); + } + } + + /// + /// Detects variables retrieved by usage of Get-Variable and remove those + /// variables from the entries in . + /// + /// + /// + private void AnalyzeGetVariableCommands( + ScriptBlockAst scriptBlockAst, + Dictionary assignmentsDictionary_OrdinalIgnoreCase) + { var getVariableCmdletNamesAndAliases = Helper.Instance.CmdletNameAndAliases("Get-Variable"); IEnumerable getVariableCommandAsts = scriptBlockAst.FindAll(testAst => testAst is CommandAst commandAst && getVariableCmdletNamesAndAliases.Contains(commandAst.GetCommandName(), StringComparer.OrdinalIgnoreCase), true); + foreach (CommandAst getVariableCommandAst in getVariableCommandAsts) { var commandElements = getVariableCommandAst.CommandElements.ToList(); - // The following extracts the variable name only in the simplest possibe case 'Get-Variable variableName' - if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst) + // The following extracts the variable name(s) only in the simplest possible usage of Get-Variable. + // Usage of a named parameter and an array of variables is accounted for though. + if (commandElements.Count < 2 || commandElements.Count > 3) { continue; } + + var commandElementAstOfVariableName = commandElements[commandElements.Count - 1]; + if (commandElements.Count == 3) { - var variableName = constantExpressionAst.Value; - if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) + if (!(commandElements[1] is CommandParameterAst commandParameterAst)) { continue; } + // Check if the named parameter -Name is used (PowerShell does not need the full + // parameter name and there is no other parameter of Get-Variable starting with n). + if (!commandParameterAst.ParameterName.StartsWith("n", StringComparison.OrdinalIgnoreCase)) { - assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); + continue; } } - } - foreach (string key in assignmentsDictionary_OrdinalIgnoreCase.Keys) - { - yield return new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key), - assignmentsDictionary_OrdinalIgnoreCase[key].Left.Extent, - GetName(), - DiagnosticSeverity.Warning, - fileName, - key); + if (commandElementAstOfVariableName is StringConstantExpressionAst constantExpressionAst) + { + assignmentsDictionary_OrdinalIgnoreCase.Remove(constantExpressionAst.Value); + continue; + } + + if (!(commandElementAstOfVariableName is ArrayLiteralAst arrayLiteralAst)) { continue; } + foreach (var expressionAst in arrayLiteralAst.Elements) + { + if (expressionAst is StringConstantExpressionAst constantExpressionAstOfArray) + { + assignmentsDictionary_OrdinalIgnoreCase.Remove(constantExpressionAstOfArray.Value); + } + } } } } diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 972c5cf82..43775b36d 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -72,7 +72,7 @@ function MyFunc2() { It "returns no violations" { $noViolations.Count | Should -Be 0 } - + It "Does not flag += operator" { $results = Invoke-ScriptAnalyzer -ScriptDefinition '$array=@(); $list | ForEach-Object { $array += $c }' | Where-Object { $_.RuleName -eq $violationName } $results.Count | Should -Be 0 @@ -88,9 +88,41 @@ function MyFunc2() { $results.Count | Should -Be 0 } - It "Using a variable via 'Get-Variable' does not trigger a warning" { - $noViolations = Invoke-ScriptAnalyzer -ScriptDefinition '$a=4; get-variable a' - $noViolations.Count | Should -Be 0 + It "No warning when using 'Get-Variable' with variables declaration '' and command parameter " -TestCases @( + @{ + DeclareVariables = '$a = 1'; GetVariableCommandParameter = 'a'; + } + @{ + DeclareVariables = '$a = 1'; GetVariableCommandParameter = '-Name a'; + } + @{ + DeclareVariables = '$a = 1'; GetVariableCommandParameter = '-n a'; + } + @{ + DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = 'a,b' + } + @{ + DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = '-Name a,b' + } + @{ + DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = '-n a,b' + } + @{ + DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = 'A,B' + } + ) { + Param( + $DeclareVariables, + $GetVariableCommandParameter + ) + $scriptDefinition = "$DeclareVariables; Get-Variable $GetVariableCommandParameter" + $noViolations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition + $noViolations.Count | Should -Be 0 -Because $scriptDefinition + } + + It "Does not misinterpret switch parameter of Get-Variable as variable" { + $scriptDefinition = '$ArbitrarySwitchParameter = 1; Get-Variable -ArbitrarySwitchParameter' + (Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition).Count | Should -Be 1 -Because $scriptDefinition } } }