Skip to content

Fix PSUseDeclaredVarsMoreThanAssignments to not give false positives when using += operator #935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 30, 2018
Merged

Fix PSUseDeclaredVarsMoreThanAssignments to not give false positives when using += operator #935

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9f97de
fix false positives such as '$a=@(); $b | foreach-object { $a+=$c}' b…
bergmeister Mar 14, 2018
7daf5a4
add test (it turns out a regression happened for 'flags strongly type…
bergmeister Mar 14, 2018
bc1c007
fix regression for strongly typed variables by being more selective a…
bergmeister Mar 14, 2018
077d035
add additional test case for unassigned variable
bergmeister Mar 16, 2018
647efe7
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Mar 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions Rules/UseDeclaredVarsMoreThanAssignments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,19 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip
// 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;
if (varInAssignmentAsStatementAst != null && varAstAsAssignmentStatementAst != null)
if (varAstAsAssignmentStatementAst != null)
{
inAssignment = varInAssignmentAsStatementAst.Left.Extent.Text.Equals(varAstAsAssignmentStatementAst.Left.Extent.Text, StringComparison.OrdinalIgnoreCase);
if (varAstAsAssignmentStatementAst.Operator == TokenKind.Equals)
{
if (varInAssignmentAsStatementAst != null)
{
inAssignment = varInAssignmentAsStatementAst.Left.Extent.Text.Equals(varAstAsAssignmentStatementAst.Left.Extent.Text, StringComparison.OrdinalIgnoreCase);
}
else
{
inAssignment = varInAssignment.Equals(varAst);
}
}
}
else
{
Expand Down
5 changes: 5 additions & 0 deletions Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,10 @@ 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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the case where $a is not initialized? Does that fall into this rule or a different one?

Copy link
Collaborator Author

@bergmeister bergmeister Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already covered because the way the algorithm works is that it first adds variable assignments to a list and then removes variables from this list when they are being used and at the end the left-over variables are basically the unused variables. In this case, += was not seen as a usage and therefore not being removed from this list. As a result, not initialising the variable does not affect the analysis because it already only looks for variable usages that are not in the list of assigned variables.
Using the += operator on an uninitialised variable might seem to be dangerous at first but it makes no problem in PowerShell at all therefore I do not think we should warn when doing that. It would only be a problem when using Set-StrictMode using version 1 or higher.
I added your test case though because it is always good to have more test cases moving forward.

$results.Count | Should -Be 0
}
}
}