PSUseDeclaredVarsMoreThanAssignments: False positive when using ForEach-Object in special cases

[bergmeister]

Extracted from this comment here:

Steps to reproduce

$Doubles = 1..100 | ForEach { $collect = @() } { $Collect += $_ * 2 } { $Collect }

$Requirement = Get-Item .
[hashtable]$hash = $Requirement.PSObject.Properties.Where{$_.Value} | ForEach-Object { $ht = @{} } { $ht.Add($_.Name, $_.value) } { $ht }
$hash

Expected behavior

No PSUseDeclaredVarsMoreThanAssignments warnings

Actual behavior

1 or more PSUseDeclaredVarsMoreThanAssignments warnings in both cases

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSUseDeclaredVarsMoreThanAssignment Warning      a.ps1      2     The variable 'ht' is assigned but never used.
s

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSUseDeclaredVarsMoreThanAssignment Warning      a.ps1      1     The variable 'Doubles' is assigned but never used.
s
PSUseDeclaredVarsMoreThanAssignment Warning      a.ps1      1     The variable 'collect' is assigned but never used.
s
PSAvoidUsingCmdletAliases           Warning      a.ps1      1     'ForEach' is an alias of 'ForEach-Object'. Alias can
                                                                  introduce possible problems and make scripts hard to
                                                                  maintain. Please consider changing alias to its full
                                                                  content.

Environment data

> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.16299.251
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.16299.251
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.16.1 # or the latest development branch

[bin3377]

Maybe same issue

function testing {
  $ret = 0
  Get-ChildItem . | ForEach-Object {
    $ret += 1
  }
  $ret
}

PSUseDeclaredVarsMoreThanAssignments reported on $ret += 1
but it's fine if rewrite it as a foreach loop

[kalgiz]

Also here we have false negative:

while(true) { $b=1 $b } $b=2

Expected rule violation for the code outside of while loop but there was no message.

[kalgiz]

Currently we collect all the ScriptBlocks on the list and then for each of them run the AnalyzeScriptBlockAst method. In the method firstly we add all the variable from assignments to the dictionary and then we check if they were used beside assignment instruction. It generates false positives when the nested ScriptBlock is analyzed separately from the parent ScriptBlock (ForEach loop example) and false negative when variable was assigned and used in nested ScriptBlock and then is assigned in the parent block (the example above).

I think the rule should be rewritten so that it analyzes block instruction after instruction and stores two dictionaries: for the variables assigned in parent scopes and variables declared in current scope. When we encounter ScriptBlock the method should be called for it recursively.

[bergmeister]

Related issue is #938. Your proposed changes are sound and would be an improvement.
The ultimate solution would be to use SSA (static single assignment analysis), which is at the moment kind of implemented (see Helper and VariableAnalysisBase class) but only partially working (it kind of works in scripts themselves but not so well inside function definitions or scriptblocks). Therefore bear in mind that we need to make a decision at some point if we want to properly finish SSA (which has the potential to make a lot of custom code obsolete) but this is a big undertaking and an 'all or nothing' project. It would be interesting to get James's idea on that. Even if we wanted to do SSA we would need to decide whether we want to continue with the existing base (which has a lot of code smells and might not be a good foundation) or start from scratch....

[kalgiz]

@bergmeister By the existing codebase you mean just the SSA changes? Probably not the whole ScriptAnalyzer codebase... ? Could you, please, point me out to some PRs with the SSA changes or code itself?
Still probably PSUseDeclaredVarsMoreThanAssignments rule should be rewritten and I have to understand the SSA first to plan it.

[bergmeister]

@kalgiz Yes, by base I only meant the existing base around the SSA implementation. I am going to try to describe my understanding of SSA in this code base now to the best of my knowledge but take it with caution as I could be wrong since I did not have to implement/touch it until recently:
A few methods in the Helper class are indirectly using SSA, which is implemented in the VariableAnalysis and VariableAnalysisBase classes (over 4000 lines in total!). You might notice that PowerShell Core itself also has a VariableAnalysis class here and I believe that this class was just copied over to PSSA a few years ago. For more details about SSA, see e.g. the Wiki page here. Those classes have a few static members to store the discovered variables/commands, hence why there are so many locks in the Helper class but I see this as evil not only because it causes us all those multi-threading bugs but also because it sometimes does not work. To give you an example: In my current PR 933 I had to get the type of a variable and found that in the case of arrays, I had to use the InternalVariableAnalysisDictionary instead of the VariableAnalysisDictionary as a workaround due to the SSA implementation not being fully finished, this was then working in a script but not if the variables are defined inside a function, therefore you see that finishing SSA would have other positive side effects. The code is relatively complex and since it isincomplete and buggy it might be easier to start from a clean base. The previous maintainer states in this comment here :

Fixing this rule requires some non-trivial amount of work. Right now the rule implementation is pretty ad-hoc. We would need to completely rewrite the rule to use analysis by single static assignment (SSA) form. One positive thing is that we do have a preliminary implementation of SSA, but it needs some modification. Will try to see to what I can do for the upcoming release but there is a good chance I won't make it by then.

But coming back to the original question that we should aim to answer:
Do we want to spend the expense of implementing SSA or rather try to improve the rule as per your suggestion? I guess it is best to talk not only to James about this but also Jason who is very knowledgeable. I guess the risk/difficulty when implementing SSA properly is that it would need to take at least basic scoping rules of PowerShell into account. Maybe he or Bruce know even about a better approach. I would welcome any improvement (whether it is your proposal, SSA, or something else) but I think a senior person at MSFT should make this decision.

[kalgiz]

@bergmeister Thanks a lot for thorough clarification. I will try to schedule some discussions with seniors more aware of the codebase and hopefully we can come with a decision soon.

[jhoneill]

Another one which doesn't get picked up is
$worksheet1 = $refPrefix = $WorkSheetName[0]

Because something immediately assigned to the value in refPrefix it is assumed to have been used. It should be treated the same as

$refPrefix     =  $WorkSheetName[0]
$worksheet1 =  $WorkSheetName[0]

But in effect is treated as

$refPrefix     =  $WorkSheetName[0]
$worksheet1 = $refPrefix 

[rjmholt]

Duplicate of #1163