diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index b35caafbc..90879259d 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -126,21 +126,7 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip foreach (AssignmentStatementAst assignmentAst in assignmentAsts) { - // Only checks for the case where lhs is a variable. Ignore things like $foo.property - VariableExpressionAst assignmentVarAst = assignmentAst.Left as VariableExpressionAst; - - if (assignmentVarAst == null) - { - // If the variable is declared in a strongly typed way, e.g. [string]$s = 'foo' then the type is ConvertExpressionAst. - // Therefore we need to the VariableExpressionAst from its Child property. - var assignmentVarAstAsConvertExpressionAst = assignmentAst.Left as ConvertExpressionAst; - if (assignmentVarAstAsConvertExpressionAst != null && assignmentVarAstAsConvertExpressionAst.Child != null) - { - assignmentVarAst = assignmentVarAstAsConvertExpressionAst.Child as VariableExpressionAst; - } - } - - if (assignmentVarAst != null) + foreach (VariableExpressionAst assignmentVarAst in GetAssignedVariables(assignmentAst.Left)) { // Ignore if variable is global or environment variable or scope is drive qualified variable if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst) @@ -166,21 +152,21 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(varKey)) { - varsInAssignment = assignmentsDictionary_OrdinalIgnoreCase[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true); + varsInAssignment = GetAssignedVariables(assignmentsDictionary_OrdinalIgnoreCase[varKey].Left); // Checks if this variableAst is part of the logged assignment foreach (VariableExpressionAst varInAssignment in varsInAssignment) { // Try casting to AssignmentStatementAst to be able to catch case where a variable is assigned more than once (https://github.com/PowerShell/PSScriptAnalyzer/issues/833) - var varInAssignmentAsStatementAst = varInAssignment.Parent as AssignmentStatementAst; - var varAstAsAssignmentStatementAst = varAst.Parent as AssignmentStatementAst; + var varInAssignmentAsStatementAst = GetAssignmentStatementOfTarget(varInAssignment); + var varAstAsAssignmentStatementAst = GetAssignmentStatementOfTarget(varAst); if (varAstAsAssignmentStatementAst != null) { if (varAstAsAssignmentStatementAst.Operator == TokenKind.Equals) { if (varInAssignmentAsStatementAst != null) { - inAssignment = varInAssignmentAsStatementAst.Left.Extent.Text.Equals(varAstAsAssignmentStatementAst.Left.Extent.Text, StringComparison.OrdinalIgnoreCase); + inAssignment = varInAssignment.Extent.Text.Equals(varAst.Extent.Text, StringComparison.OrdinalIgnoreCase); } else { @@ -192,6 +178,12 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip { inAssignment = varInAssignment.Equals(varAst); } + + // A multiple assignment has several variables on the lhs, so stop at the first match + if (inAssignment) + { + break; + } } if (!inAssignment) @@ -222,6 +214,38 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip } } + /// + /// Returns the variables assigned by the lhs of an assignment, e.g. $a, [string]$s or each of $a, $b. + /// Ignores things like $foo.property and $foo[$i], including the variables they read. + /// + private static IEnumerable GetAssignedVariables(ExpressionAst left) + { + // Assigning multiple variables, e.g. $a, $b = 1, 2, assigns each element of the array literal + IEnumerable leftAsts = left is ArrayLiteralAst arrayLiteralAst + ? arrayLiteralAst.Elements + : (IEnumerable)new[] { left }; + + foreach (ExpressionAst leftAst in leftAsts) + { + // If the variable is declared in a strongly typed way, e.g. [string]$s = 'foo' then the type is ConvertExpressionAst. + // Therefore we need to the VariableExpressionAst from its Child property. + VariableExpressionAst variableAst = leftAst as VariableExpressionAst ?? (leftAst as ConvertExpressionAst)?.Child as VariableExpressionAst; + if (variableAst != null) + { + yield return variableAst; + } + } + } + + /// + /// Returns the assignment whose lhs is this variable, or contains it as an element as in $a, $b = 1, 2. + /// + private static AssignmentStatementAst GetAssignmentStatementOfTarget(VariableExpressionAst variableAst) + { + return variableAst.Parent as AssignmentStatementAst + ?? (variableAst.Parent as ArrayLiteralAst)?.Parent as AssignmentStatementAst; + } + /// /// Detects variables retrieved by usage of Get-Variable and remove those /// variables from the entries in . diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 113563796..e9dca1ba9 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -117,6 +117,27 @@ function MyFunc2() { $results | Get-Count | Should -Be 1 $results[0].Extent | Should -Be '$mySecondvar' } + + It "flags each unused variable of a multiple assignment" { + Invoke-ScriptAnalyzer -ScriptDefinition '$a, $b = $null' -IncludeRule $violationName | ` + Get-Count | ` + Should -Be 2 + } + + It "flags only the unused variable of a multiple assignment" { + $results = Invoke-ScriptAnalyzer -ScriptDefinition '$a, $b = 1, 2; $b' -IncludeRule $violationName + $results | Get-Count | Should -Be 1 + $results[0].RuleSuppressionID | Should -Be 'a' + } + + It "flags variables of a multiple assignment that are only assigned again" -TestCases @( + @{ ScriptDefinition = '$a, $b = 1, 2; $a = 3' } + @{ ScriptDefinition = '$a, $b = 1, 2; $a, $b = 3, 4' } + ) { + param($ScriptDefinition) + $results = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $violationName + $results.RuleSuppressionID | Sort-Object | Should -Be @('a', 'b') + } } Context "When there are no violations" { @@ -139,6 +160,16 @@ function MyFunc2() { $results.Count | Should -Be 0 } + It "Does not flag variables of a multiple assignment that are used" { + $results = Invoke-ScriptAnalyzer -ScriptDefinition '$a, $b = 1, 2; $a + $b' -IncludeRule $violationName + $results.Count | Should -Be 0 + } + + It "Does not flag a variable of a multiple assignment that is used in an index of another target" { + $results = Invoke-ScriptAnalyzer -ScriptDefinition 'param($items); $index, $items[$index] = 0, 42' -IncludeRule $violationName + $results.Count | Should -Be 0 + } + It "Does not flag drive qualified variables such as env" { $results = Invoke-ScriptAnalyzer -ScriptDefinition '$env:foo = 1; function foo(){ $env:bar = 42 }' $results.Count | Should -Be 0