From de4229503afa69b17069e73c0681c606f414bf6e Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 16 Aug 2019 18:13:46 +0100 Subject: [PATCH 1/4] Parse Array syntax of Get-Variable --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 17 ++++++++++++++++- ...UseDeclaredVarsMoreThanAssignments.tests.ps1 | 7 ++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 6d0f29fdf..7592c797a 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -215,7 +215,8 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip 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' + // The following extracts the variable name only in the simplest possible cases + // 'Get-Variable variableName' and 'Get-Variable variableName1, variableName2' if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst) { var variableName = constantExpressionAst.Value; @@ -224,6 +225,20 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); } } + if (commandElements.Count == 2 && commandElements[1] is ArrayLiteralAst arrayLiteralAst) + { + foreach (var expressionAst in arrayLiteralAst.Elements) + { + if (expressionAst is StringConstantExpressionAst constantExpressionAstOfArray) + { + var variableName = constantExpressionAstOfArray.Value; + if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) + { + assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); + } + } + } + } } foreach (string key in assignmentsDictionary_OrdinalIgnoreCase.Keys) diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 972c5cf82..f40ad8966 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 @@ -92,5 +92,10 @@ function MyFunc2() { $noViolations = Invoke-ScriptAnalyzer -ScriptDefinition '$a=4; get-variable a' $noViolations.Count | Should -Be 0 } + + It "Using a variable via 'Get-Variable' does not trigger a warning" { + $noViolations = Invoke-ScriptAnalyzer -ScriptDefinition '$a=4; $b = 8; get-variable a, b' + $noViolations.Count | Should -Be 0 + } } } From e6d52fe05378f36a47ce525b34370cff38460175 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 16 Aug 2019 19:48:43 +0100 Subject: [PATCH 2/4] check named parameter better to be -Name or shortened versions and add more tests --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 25 +++++++++++- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 38 +++++++++++++++---- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 7592c797a..06f499f58 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -217,7 +217,28 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip var commandElements = getVariableCommandAst.CommandElements.ToList(); // The following extracts the variable name only in the simplest possible cases // 'Get-Variable variableName' and 'Get-Variable variableName1, variableName2' - if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst) + + if (commandElements.Count < 2 || commandElements.Count > 3) + { + continue; + } + + var commandElementAstOfVariableName = commandElements[commandElements.Count - 1]; + if (commandElements.Count == 3) + { + if (commandElements[1] is CommandParameterAst commandParameterAst) + { + // 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)) + { + continue; + } + } + continue; + } + + if (commandElementAstOfVariableName is StringConstantExpressionAst constantExpressionAst) { var variableName = constantExpressionAst.Value; if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) @@ -225,7 +246,7 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); } } - if (commandElements.Count == 2 && commandElements[1] is ArrayLiteralAst arrayLiteralAst) + if (commandElementAstOfVariableName is ArrayLiteralAst arrayLiteralAst) { foreach (var expressionAst in arrayLiteralAst.Elements) { diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index f40ad8966..82f6ccf24 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -88,14 +88,36 @@ 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 "Using a variable via 'Get-Variable' does not trigger a warning" { - $noViolations = Invoke-ScriptAnalyzer -ScriptDefinition '$a=4; $b = 8; get-variable a, b' - $noViolations.Count | Should -Be 0 + It "No warning when using 'Get-Variable' with variables declaration '' and command paramter " -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 } } } From 5daadfd836c32abc8d8cb5b42f1d60e98182ac8d Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 16 Aug 2019 21:40:22 +0100 Subject: [PATCH 3/4] Fix last edge cases and refactor into method --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 88 ++++++++++--------- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 7 +- 2 files changed, 54 insertions(+), 41 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 06f499f58..ee4fb1d7e 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -208,70 +208,78 @@ 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 possible cases - // 'Get-Variable variableName' and 'Get-Variable variableName1, variableName2' - - if (commandElements.Count < 2 || commandElements.Count > 3) - { - continue; - } + // 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) { - if (commandElements[1] is CommandParameterAst commandParameterAst) + var commandParameterAst = commandElements[1] as CommandParameterAst; + if (commandParameterAst == null) { 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)) { - // 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)) - { - continue; - } + continue; } - continue; } if (commandElementAstOfVariableName is StringConstantExpressionAst constantExpressionAst) { - var variableName = constantExpressionAst.Value; - if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) + MarkVariableAsAssigned(constantExpressionAst.Value); + continue; + } + + var arrayLiteralAst = commandElementAstOfVariableName as ArrayLiteralAst; + if (arrayLiteralAst == null) { continue; } + foreach (var expressionAst in arrayLiteralAst.Elements) + { + if (expressionAst is StringConstantExpressionAst constantExpressionAstOfArray) { - assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); + MarkVariableAsAssigned(constantExpressionAstOfArray.Value); } } - if (commandElementAstOfVariableName is ArrayLiteralAst arrayLiteralAst) + + void MarkVariableAsAssigned(string variableName) { - foreach (var expressionAst in arrayLiteralAst.Elements) + if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) { - if (expressionAst is StringConstantExpressionAst constantExpressionAstOfArray) - { - var variableName = constantExpressionAstOfArray.Value; - if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) - { - assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); - } - } + assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); } } } - - 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); - } } } } diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 82f6ccf24..43775b36d 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -88,7 +88,7 @@ function MyFunc2() { $results.Count | Should -Be 0 } - It "No warning when using 'Get-Variable' with variables declaration '' and command paramter " -TestCases @( + It "No warning when using 'Get-Variable' with variables declaration '' and command parameter " -TestCases @( @{ DeclareVariables = '$a = 1'; GetVariableCommandParameter = 'a'; } @@ -119,5 +119,10 @@ function MyFunc2() { $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 + } } } From 9036c231ce593166278293e018a27a31fbff1394 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 19 Aug 2019 21:57:53 +0100 Subject: [PATCH 4/4] Apply PR suggestions: Use shorter pattern matching syntax and inline/simplify local method --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index ee4fb1d7e..5a8440ada 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -246,8 +246,7 @@ private void AnalyzeGetVariableCommands( var commandElementAstOfVariableName = commandElements[commandElements.Count - 1]; if (commandElements.Count == 3) { - var commandParameterAst = commandElements[1] as CommandParameterAst; - if (commandParameterAst == null) { continue; } + 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)) @@ -258,25 +257,16 @@ private void AnalyzeGetVariableCommands( if (commandElementAstOfVariableName is StringConstantExpressionAst constantExpressionAst) { - MarkVariableAsAssigned(constantExpressionAst.Value); + assignmentsDictionary_OrdinalIgnoreCase.Remove(constantExpressionAst.Value); continue; } - var arrayLiteralAst = commandElementAstOfVariableName as ArrayLiteralAst; - if (arrayLiteralAst == null) { continue; } + if (!(commandElementAstOfVariableName is ArrayLiteralAst arrayLiteralAst)) { continue; } foreach (var expressionAst in arrayLiteralAst.Elements) { if (expressionAst is StringConstantExpressionAst constantExpressionAstOfArray) { - MarkVariableAsAssigned(constantExpressionAstOfArray.Value); - } - } - - void MarkVariableAsAssigned(string variableName) - { - if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) - { - assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName); + assignmentsDictionary_OrdinalIgnoreCase.Remove(constantExpressionAstOfArray.Value); } } }