From 53cc1aad77c23785962ccf0c994e09e15b784b44 Mon Sep 17 00:00:00 2001 From: Ethan Aldrich Date: Sun, 18 Oct 2026 16:05:38 -0700 Subject: [PATCH 1/2] Flag unused variables in multiple assignments UseDeclaredVarsMoreThanAssignments only looked at assignments whose left-hand side is a single variable, so `$a, $b = $null` was never flagged. Treat each variable of an array literal on the left-hand side like a single-variable assignment, and stop at the first matching variable when checking whether a reference is the assignment itself. Fix #2164 --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 52 ++++++++++++------- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 17 ++++++ 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index b35caafbc..fdfd6c207 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -126,32 +126,40 @@ 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; + // Assigning multiple variables, e.g. $a, $b = 1, 2, assigns each element of the array literal + IEnumerable leftAsts = assignmentAst.Left is ArrayLiteralAst arrayLiteralAst + ? arrayLiteralAst.Elements + : (IEnumerable)new[] { assignmentAst.Left }; - if (assignmentVarAst == null) + 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. - var assignmentVarAstAsConvertExpressionAst = assignmentAst.Left as ConvertExpressionAst; - if (assignmentVarAstAsConvertExpressionAst != null && assignmentVarAstAsConvertExpressionAst.Child != null) + // Only checks for the case where lhs is a variable. Ignore things like $foo.property + VariableExpressionAst assignmentVarAst = leftAst as VariableExpressionAst; + + if (assignmentVarAst == null) { - assignmentVarAst = assignmentVarAstAsConvertExpressionAst.Child as VariableExpressionAst; + // 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 = leftAst as ConvertExpressionAst; + if (assignmentVarAstAsConvertExpressionAst != null && assignmentVarAstAsConvertExpressionAst.Child != null) + { + assignmentVarAst = assignmentVarAstAsConvertExpressionAst.Child as VariableExpressionAst; + } } - } - if (assignmentVarAst != null) - { - // Ignore if variable is global or environment variable or scope is drive qualified variable - if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst) - && !assignmentVarAst.VariablePath.IsScript - && assignmentVarAst.VariablePath.DriveName == null) + if (assignmentVarAst != null) { - string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath); - - if (!assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) + // Ignore if variable is global or environment variable or scope is drive qualified variable + if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst) + && !assignmentVarAst.VariablePath.IsScript + && assignmentVarAst.VariablePath.DriveName == null) { - assignmentsDictionary_OrdinalIgnoreCase.Add(variableName, assignmentAst); + string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath); + + if (!assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) + { + assignmentsDictionary_OrdinalIgnoreCase.Add(variableName, assignmentAst); + } } } } @@ -192,6 +200,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) diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 113563796..26f69bee7 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -117,6 +117,18 @@ 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' + } } Context "When there are no violations" { @@ -139,6 +151,11 @@ 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 drive qualified variables such as env" { $results = Invoke-ScriptAnalyzer -ScriptDefinition '$env:foo = 1; function foo(){ $env:bar = 42 }' $results.Count | Should -Be 0 From f875f8fe1ba907708e6a8f3b917442fec39a3ec9 Mon Sep 17 00:00:00 2001 From: Ethan Aldrich Date: Mon, 19 Oct 2026 08:17:34 -0700 Subject: [PATCH 2/2] Only treat assigned variables of a multiple assignment as writes Reassigning a variable from a multiple assignment, as in `$a, $b = 1, 2; $a = 3`, was counted as a use, unlike `$a = 1; $a = 3`. Variables read inside another target, as in `$i, $x[$i] = 0, 42`, were counted as writes, so `$i` was wrongly flagged. --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 78 +++++++++++-------- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 14 ++++ 2 files changed, 58 insertions(+), 34 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index fdfd6c207..90879259d 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -126,40 +126,18 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip foreach (AssignmentStatementAst assignmentAst in assignmentAsts) { - // Assigning multiple variables, e.g. $a, $b = 1, 2, assigns each element of the array literal - IEnumerable leftAsts = assignmentAst.Left is ArrayLiteralAst arrayLiteralAst - ? arrayLiteralAst.Elements - : (IEnumerable)new[] { assignmentAst.Left }; - - foreach (ExpressionAst leftAst in leftAsts) + foreach (VariableExpressionAst assignmentVarAst in GetAssignedVariables(assignmentAst.Left)) { - // Only checks for the case where lhs is a variable. Ignore things like $foo.property - VariableExpressionAst assignmentVarAst = leftAst as VariableExpressionAst; - - if (assignmentVarAst == null) + // Ignore if variable is global or environment variable or scope is drive qualified variable + if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst) + && !assignmentVarAst.VariablePath.IsScript + && assignmentVarAst.VariablePath.DriveName == 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 = leftAst as ConvertExpressionAst; - if (assignmentVarAstAsConvertExpressionAst != null && assignmentVarAstAsConvertExpressionAst.Child != null) - { - assignmentVarAst = assignmentVarAstAsConvertExpressionAst.Child as VariableExpressionAst; - } - } + string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath); - if (assignmentVarAst != null) - { - // Ignore if variable is global or environment variable or scope is drive qualified variable - if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst) - && !assignmentVarAst.VariablePath.IsScript - && assignmentVarAst.VariablePath.DriveName == null) + if (!assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) { - string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath); - - if (!assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName)) - { - assignmentsDictionary_OrdinalIgnoreCase.Add(variableName, assignmentAst); - } + assignmentsDictionary_OrdinalIgnoreCase.Add(variableName, assignmentAst); } } } @@ -174,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 { @@ -236,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 26f69bee7..e9dca1ba9 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -129,6 +129,15 @@ function MyFunc2() { $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" { @@ -156,6 +165,11 @@ function MyFunc2() { $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