PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.85k stars 375 forks source link

false positive 'PSUseDeclaredVarsMoreThanAssignments' #711

Open mwallner opened 7 years ago

mwallner commented 7 years ago

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"
TheRealHotFix commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

kapilmb commented 7 years ago

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 commented 7 years ago

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

davebrannan commented 7 years ago

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

DexterPOSH commented 7 years ago

bump I am still seeing the false positives. As explained here https://github.com/PowerShell/PSScriptAnalyzer/issues/699

gwojan commented 7 years ago

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 commented 7 years ago

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

PowerDBAKlaas commented 7 years ago

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

ThubLives commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 }
    

    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 commented 4 years ago

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 commented 4 years ago

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!