Skip to content
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.

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

false positive 'PSUseDeclaredVarsMoreThanAssignments' #711

Open
mwallner opened this issue Feb 14, 2017 · 18 comments
Open

false positive 'PSUseDeclaredVarsMoreThanAssignments' #711

mwallner opened this issue Feb 14, 2017 · 18 comments

Comments

@mwallner
Copy link

mwallner commented Feb 14, 2017

I get the message 'The variable 'param' is assigned but never used.'
My script looks like this:

$param = $MyInvocation.BoundParameters

a couple of lines further down in the same script:

. "$PSScriptRoot\wrap.script.ps1 @param"
@sheehand
Copy link

sheehand commented Feb 20, 2017

Same here:
The variable 'NewFileArray' is assigned but never used.
Here is the code:
`# Create an array to hold the new file attributes (skipping the "Entry #" attribute since that is not stored in the AD attribute field).
$NewFileArray = @()
$NewFileEntry.PSObject.Properties | Where-Object {$.Name -notlike "Entry"} | ForEach-Object {
# Join the name and value of each attribute, using the colon character as a delimiter.
$NewFileArray += $
.Name + ":" + $_.Value
}

Create an ADSI connection to the Service Account defined in the Main Window, and set the custom attribute (extensionAttribute) to the Entry #.

$UpdateAccount = [ADSI]$($SvcAccount.Properties.adspath).ToString()
$ADAttribute = "extensionAttribute" + $NewFileEntry.Entry

Check to see if there were any entries in the array after excluding the Entry #.

If ($NewFileArray) {
# There were so join them in a contiguous string using the comma character as a delimiter, and then store that string in the custom attribute.
$UpdateAccount.$ADAttribute = $NewFileArray -join ","
} Else {
# There weren't so blank the attribute because someone must have cleared them in the Edit Window.
$UpdateAccount.$ADAttribute.Clear()
}`

@mwallner
Copy link
Author

bump

It'd be great to get this fixed - getting a lot of false positives in a couple of projects.
(yes - most likely due to a certain coding style - but it's still an issue ;-))

@daviwil
Copy link
Contributor

daviwil commented Mar 31, 2017

Hey @kapilmb can you take a look and see how much effort it might take to get this resolved for an upcoming release?

@kapilmb
Copy link

kapilmb commented Mar 31, 2017

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.

@kapilmb
Copy link

kapilmb commented Mar 31, 2017

@daviwil When are we planning the next feature release? I might be able to make dash for this if there is some margin.

@davebrannan
Copy link

I also see this when I declare a variable in a module and export it using Export-ModuleMember.

@DexterPOSH
Copy link

bump
I am still seeing the false positives. As explained here #699

@gwojan
Copy link

gwojan commented Jul 20, 2017

I just noticed this bug... It appears any time a variable is assigned inside {} resulting in a false positive being identified by PSScriptAnalyzer.

$bubba = 'Hi there'
1..2 | % { $bubba = 'Some contrived value' }
$bubba

OR

$bubba = ''
{ $bubba = 'assign something here' }
$bubba

@DarqueWarrior
Copy link

It is also triggered when you assign an environment variable like this.
$env:TEAM_PAT = $Pat

@PowerDBAKlaas
Copy link

I have this example where a property of the variable is used:
image

@ThubLives
Copy link

I'm seeing this when a variable is assigned a value inside specific script block types and no other reference is made within the same script block. I'm using VS Code 1.16.0 and ms-vscode.powershell 1.4.2.

So far I've only seen two types of script blocks exhibiting the problem: ForEach-Object and Where-Object. Someone above mentioned arbitrary script blocks like

$ScriptBlock = {
  $ScriptBlock = 'No script here'
}
Invoke-Command $ScriptBlock

but I'm pretty sure it's working as designed, because it's got its own scope, so $ScriptBlock should not redefine itself.

These ones are unaffected, though if they are wrapped in another, problematic script block type, the problem will still occur: if, else, elseif, do, while, try, catch, and finally.

For example, the second instance of $IsEven is marked as PSUseDeclaredVarsMoreThanAssignments, but moving it outside the ForEach-Object block clears the warning:

$IsEven = $false
@(2, 3, 5, 7, 11, 13, 17) | ForEach-Object {
  if (($_ % 2) -eq 0) {
    $IsEven = $true
  }
}

if ($IsEven) {
  Write-Host "At least one of the numbers is even."
}

@jdkang
Copy link

jdkang commented Sep 18, 2017

I'm seeing this specifically with assigning values to collections inside a a foreach-object and using the collection later -- either as a reference or iterating over it.

EXAMPLE:

    # Export Functions and DSC Resources in src\Public
    [string[]]$funcsToExport = @()
    [string[]]$dscResToExport = @()
    Get-ChildItem -Path "$moduleDestPath\src\Public" -Filter *.ps1 |
        & $BuildRoot\build\Find-PsFuncOrDscRes.ps1 |
        Foreach-Object {
            if($_.Functions.Count -gt 0) {
                $funcsToExport += ($_.Functions.Name)
            }
            if($_.DscResources -gt 0) {
                $dscResToExport += ($_.DscResources.Name)
            }
        }
    if($funcsToExport.Count -gt 0) {
        $moduleManifestSplat['FunctionsToExport'] = $funcsToExport
    }
    if($dscResToExport.Count -gt 0) {
        $moduleManifestSplat['DscResourcesToExport'] = $dscResToExport
    }

@VimalShekar
Copy link

VimalShekar commented Mar 20, 2018

bump -
This also returns a false positive for global variables even when they are qualified with "$global:", if the variable is not re-used within the scope of the current block

Ex:

Function MakeTrue {
 $Global:g_GlobalVariable = $true
}

Function MakeFalse {
 $Global:g_GlobalVariable = $false
}

@stibinator
Copy link

stibinator commented Jan 3, 2019

I have a variable declared within a function and then possibly modified in an conditional statement inside a loop, then finally returned by the function and I'm getting a false positive. If I move the return into the foreach-object loop then the message goes away (but obv that's not the behaviour I want from this function)

function foo
{
    param($bar)
    $baz = $false
    Get-ChildItem $bar|ForEach-Object
    {
        if ($_.name.match("fuz"))
        {
            $baz = $true
        }
    }
    return $baz
}

@DarkLite1
Copy link

DarkLite1 commented Mar 26, 2019

Same here with a Pester test case:

image

Test code in the latest VS Code insiders:

Describe 'incorrect configuration is corrected and registered' {
    BeforeAll {
        ."$here\$sut" @testParams

        $testPrintConfig = Get-PrintConfiguration -PrinterName $testPrinter.PrinterName
    }
    it 'Color' {
        $testPrintConfig.Color | Should -Be $false

    } -Skip:$Skip
    it 'Collate' {
        $testPrintConfig.Collate | Should -Be $false
    } -Skip:$Skip
}

@rjmholt
Copy link
Contributor

rjmholt commented Sep 26, 2019

Just reading through this issue to see if anyone's already asked for Pester variable special casing (which they have).

I should note here that this rule will never be able to fully correctly detect when a variable isn't used in every case. There are several ways to go behind its back because of PowerShell's dynamic nature (dynamic meaning that the only way to know is to execute the code, which is potentially side-effectful -- this is known as "undecidable").

  1. Dynamic scope (the way PowerShell resolves variables, as opposed to lexical scope):

    $x = 7
    
    function Test-X
    {
        $x
    }
    
    function Test-InnerX
    {
        $x = 5
        Test-X
    }
    
    $x             # 7
    Test-X         # 7
    Test-InnerX    # 5
    $x             # 7 (Just to prove that Test-InnerX didn't set the outer $x)

    How many times is outer $x referred to here? Depends on where Test-X is called, because the $x it references it resolved at call time, not at definition time like it would be in Python for example.

    Also "call time" here could mean after script execution is started:

    $x = 7
    function Test-X { $x }
    
    [scriptblock]::Create("Test-X").Invoke()

    Or even:

    Set-Content -Path ./script.ps1 -Value "Test-X"
    
    function Test-X { $x }
    
    $x = 111
    
    ./script.ps1     # 111
  2. Unsual scoping (in some cases impossible to know)

    • $env:var could be used by other processes
    • $global:var could be used by other runspaces
    • $using:var should be straightforward since it's a local variable copy
    • $script:var is actually easy to check, but different to local
    • $local:var is even easier
  3. Accessing variables through the variable: provider

    $x = 8
    
    Get-Item 'variable:/x' | Write-Output
  4. Using Get-/Set-Variable

    $x = 10
    
    Get-Variable -ValueOnly x
  5. Using either of the last two with non-constant variable names:

    Set-Content -Path ./variableName.txt -Value "contentOfFile"
    $varname = Get-Content -Raw ./variableName.txt
    "Hi" | Set-Variable $varname
    
    Write-Output $contentOfFile
  6. Variables in contexts with mangled scopes (Pester, ForEach-Object, dot-sourcing)

    Describe "Tests" {
      BeforeAll {
        $x = 9
      }
      It {
        $x | Should -Be 9
      }
    }

A few of these could be solved in easy cases where constant arguments are given, it's just a case of "shaving the yak". But as a static analyzer, PSScriptAnalyzer can only do so much. Ultimately this rule is supposed to be a helpful heuristic rather than an absolute.

@m8ram
Copy link

m8ram commented Mar 10, 2020

This might be covered by some of the other examples but just in case here is another sample where the $ln1 variable is being incorrectly marked as unused:
1, 2, 3 | ForEach-Object -begin { $ln1=0 } -Process { '{0,6}<<:{1}' -f ++$ln1,$_ }

@plastikfan
Copy link

A few of these could be solved in easy cases where constant arguments are given, it's just a case of "shaving the yak". But as a static analyzer, PSScriptAnalyzer can only do so much. Ultimately this rule is supposed to be a helpful heuristic rather than an absolute.

That's a profound statement and I will taking this on board with relish!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests