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

PSUseDeclaredVarsMoreThanAssignments is not aware of Pester's scoping #946

Open
RemcoKapinga opened this issue Mar 25, 2018 · 9 comments
Open

Comments

@RemcoKapinga
Copy link

RemcoKapinga commented Mar 25, 2018

Steps to reproduce

Supposing a Pester Test file like below:

$here = $PsScriptRoot

Describe 'My Module' {

    BeforeAll { $module = Import-Module "$here\MyModule.psd1" }
    AfterAll  { $module | Remove-Module -Force }

    It 'can be loaded' {
        $module | Should Not Be $null
     }
}

Expected behavior

No warning on the $module variable, as it is used in the code below.

Actual behavior

Invoke-ScriptAnalyzer reports the following:

PSUseDeclaredVarsMoreThanAssignments  Warning  My.Tests.ps1 5  The variable 'module' is assigned but never used.

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
1.16.0
1.15.0
1.14.1
1.14.0
1.13.0
1.12.0
1.11.1
1.11.0
1.10.0
1.9.0
1.8.1
1.8.0
1.7.0
1.6.0
1.5.0
1.4.0
1.3.0
1.2.0
1.1.1
1.1.0
1.0.2
1.0.1
1.0
@bergmeister
Copy link
Collaborator

bergmeister commented Mar 25, 2018

This is due to #938
The rule currently does the analysis on a per scriptblock basis, therefore it does not see the connection between them. It would also mean that PSSA has to know about Pester syntax like Context, BeforeEach or BeforeAll, etc. and therefore this is not straightforward to fix but I can totally understand that this is annoying. A workaround is to use script scope instead, i.e. you assign and use the variable as $script:module.
In case you are interested, this is how it looks like in the debugger (i.e. those are the 5 scriptblocks that it analyses separately:
image

@RemcoKapinga
Copy link
Author

Using $script:module is a good workaround for this complex issue.

@jhoneill
Copy link

Checking on Per scriptblock causes a lot of false positives , for example in the begin block below.

 Get-ChildItem -Path "$injectPath\all"  -Directory   |     
        ForEach-Object -Begin   {$DefaultCopyDirectories = @{} }`
                       -Process {$DefaultCopyDirectories[$_.FullName] = "\" }

@bergmeister
Copy link
Collaborator

@jhoneill there is already issue #804 for begin, process, end blocks.
Extending the analysis beyond the current per-scriptblock approach is unfortunately not trivial and requires a lot of thinking and experimentation to come up with the right solution. An idea would be to properly implement SSA in a way that it understands PowerShell's scoping and operations such as dot-sourcing

@jhoneill
Copy link

@bergmeister thanks, I hadn't looked at #804 ; as you say it isn't trivial to figure out but as-is there are too many false positives … but perhaps not enough to make it a "must fix"

@bergmeister
Copy link
Collaborator

bergmeister commented May 16, 2018

@jhoneill Because of all issues related to PSUseDeclaredVarsMoreThanAssignments, I created a special label for it. I think the rule in its current state is very valuable but could definitely be improved. In issue #934 we were already spinning ideas. It is possible to continue tweaking and enhancing the rule but the current implementation itself has limitations, therefore using a different approach like SSA might be better and could lead to a much better behaviour but the SSA implementation and rewrite is a big undertaking. So it is a case of it maybe being better to just leave it for the moment and rather wait a bit longer until we have a better base/framework because having proper SSA will enable also other scenarios such as being able to warn on using += on objects of type array (which requires figuring out the type)

@dmitryserbin
Copy link

+1 we're having the same issue

@glennsarti
Copy link

Chiming in with a VSCode example

image

Given the number of false-positive issues, would it be possible to downgrade this rule violation from "Warning" to "Hint"/"Informational" or something less severe?

@rjmholt rjmholt changed the title Code in dependant scriptblocks / scopes shouldn't report PSUseDeclaredVarsMoreThanAssignments PSUseDeclaredVarsMoreThanAssignments is not aware of Pester's scoping Feb 9, 2021
@GityupSS
Copy link

Would it be possible to "acknowledge" this kind of error?

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

8 participants