Skip to content

PSUseDeclaredVarsMoreThanAssignments is thrown incorrectly #903

New issue

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

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
essentialexch opened this issue Feb 21, 2018 · 11 comments · Fixed by #935
Closed

PSUseDeclaredVarsMoreThanAssignments is thrown incorrectly #903

essentialexch opened this issue Feb 21, 2018 · 11 comments · Fixed by #935

Comments

@essentialexch
Copy link

essentialexch commented Feb 21, 2018

Steps to reproduce

$files = Get-ChildItem -Recurse $folder
$str = $files.Count.ToString( 'n0' )
$totalSize = 0
$files |
 Where-Object   { $_.PSIsContainer -eq $false } | 
 ForEach-Object { $totalSize += $_.Length } # warning for $totalSize
$str = $totalSize.ToString( 'n0' )

Expected behavior

On line 7, nothing should be reported.

Actual behavior

On line 7, $totalSize is underlined with a green squiggle, "The variable 'totalSize' is assigned but never used. (PSUseDeclaredVarsMoreThanAssignments)"; which is obviously false.


Environment data
----------------

Latest VSCode on Windows. PS 5.1 with Win10 patched to current.

<!-- Provide the output of the following 2 commands -->

```powershell
> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.16299.98
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.16299.98
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }

PS C:\Users\michael> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.16.1
PS C:\Users\michael>
@kilasuit
Copy link
Contributor

I believe that this is correctly throwing the PSScriptAnalzyer warning as on each iteration of += it recreates the Array, as opposed to adds to it.

This is due to the @() creating an empty, fixed size collection array

I would recommend using [System.Collections.ArrayList]@() and using $null = $totalsize.Add($_.Length) in place of this for both meeting the usage that this PSScriptAnalyzer rule covers and also script performance reasons

@essentialexch
Copy link
Author

In my example, $totalSize is an integer and it provides the correct answer. It is demonstrably NOT being recreated in the execution block.

Your analysis isn't accurate.

@essentialexch
Copy link
Author

Further back and forth led me to discover that the problem with PSSA rules seems to be "+=". If I change "$totalSize += $.Length" to "$totalSize = $totalSize + $.Length" then PSSA quits complaining.

That being said, see Jason Shirk's email of 21-Feb to psmvps describing a class of bugs (which includes this one).

@bergmeister
Copy link
Collaborator

bergmeister commented Feb 21, 2018

This seems to be a duplicate of #636 to me and that @kilasuit is right about the warning being ok, therefore I would like to close this issue if you are OK with that? There are various known issues with PSUseDeclaredVarsMoreThanAssignments, which has a minimum viable implementation. I plan to collect all those in a meta issue that will allow us to make a decision whether it is worth improving the existing implementation or re-writing the rule with a better approach to allow the rule to be more intelligent and then solve all those problems in one go.

@essentialexch
Copy link
Author

I agree that its a duplicate of #636 (once I discovered the workaround). I do not agree with @kilasuit 's analysis (and neither did Jason Shirk). The warning is not ok.

Jason's email on the topic:

It’s a PSSA bug. I get the same warning with:

$totalSize = 0
. { $totalSize += 1 }
$totalSize

PSSA is apparently analyzing each script block independently.

It needs to recognize when a script block is dot sourced and analyze it in the context of where it is >invoked. This isn’t always possible, but it’s usually easy to do when using ForEach-Object (which dot >sources like this).

In trying some variants on the idea, it seems like there are multiple bugs:

$totalSize = 0
function foo {
$x = $totalSize + 1
$totalSize = $x # Should warn, doesn't (bug)
}
& {
$x = $totalSize + 1
$totalSize = $x # Should warn, doesn't (bug)
}
. {
$x = $totalSize + 1
$totalSize = $x # Should not warn, doesn't (no bug)
}
. {
$totalSize += 1 # Should not warn, does (bug)
}
$totalSize

@bergmeister
Copy link
Collaborator

bergmeister commented Feb 21, 2018

@swngdnz That's fine, it is totally OK to disagree and discuss it. Thanks for providing more details and contributing to the discussion. Personally I am not 100% sure on which side I really am but Jason is usually the expert in this topic. It is good to first lay down examples to know what the expected behaviour is (which is really non-trivial as we see here) but the bottom line to me is also that one needs to think about better design ideas (because this is the underlying issue here) in order to make the rule better. I am wondering if it is better to have e.g. a monthly community call to discuss such issues together or follow an RFC like approach similar to the PowerShell core repo where a technical details are being proposed (so that everyone can read and think about it beforehand) and then discuss it together.
P.S. my personal workaround is to use script scoping ($script:variablename) to get rid of false positives.

@essentialexch
Copy link
Author

Hi... I don't know your email address, but perhaps you could consider joining the PSMVP mailing list. We do discuss these things there (and PSSA has been a fairly popular topic).

On this specific issue, using $script:variablename is another workaround. Is it NOT a solution to the problem.

As a specific comment, I have NO idea where or why @kilasuit introduced an array. That is NOT, and never was, part of my original post. It makes a significant impact on the proper decision and result.

Perhaps both a monthly call and an RFC would be good. The challenge is that users of PSSA (and PS in general) are worldwide. The monthly call works primarily for Western Europe and the Americas. The RFC allows for the rest of the world to contribute.

@essentialexch
Copy link
Author

Oh, and @lzybkr is Jason. I had to look up his GitHub user. This comment is to wrap him in.

@bergmeister
Copy link
Collaborator

bergmeister commented Feb 22, 2018

@swngdnz Is the PSMVP mailing list not public? I would rather prefer to discuss issues in public here especially since other folks can learn from the discussions as well. Although I am not a friend of emails (following Scott Hanselman's advice of There are a finite number of keystrokes left in your hands before you die.), but I can give it a go and listen in only for starters. I usually use the GitHub API https://api.github.com/users/INSERT_USERNAME_HERE/events/public to find people's email so feel free to add me.

@lzybkr
Copy link

lzybkr commented Feb 22, 2018

The PSVMP mailing list is not public. The discussion should take place here.

@bergmeister
Copy link
Collaborator

bergmeister commented Mar 14, 2018

I have a fix for this now, therefore I will extract the cases from Jason's valuable email into a new issue because the awareness of whether a scriptblock is being executed or dot sourced is a different issue.

# for free to join this conversation on GitHub. Already have an account? # to comment