Open rjmholt opened 3 years ago
A nice option would be to allow us to control when this rule (or any other) is suppressed or not. Right now:
Diagnostics.CodeAnalysis.SuppressMessageAttribute
.If we had some way of doing this at a script level would benefit and allow author to control ignoring rules they do not want (or know can't cover the structure in the script). I'm in the boat of Pester so it is the one rule that comes back right now on every file.
At most if I had the ability to tell VS Code to ignore this rule for *.tests.ps1
files would help clear the problems panel 😬 .
@wsmelton I've added your scenario to https://github.com/PowerShell/PSScriptAnalyzer/issues/1483
A couple of the examples given in the initial post seem like scenarios that would merit a warning to the writer that they've done something else wrong (overshadowed from outer scope, double-assigning before reference, etc). Or is that the kind of analysis that you're referring to? If not, those seem a bit out of scope.
I had to fully disable the rule because I couldn't get a suppress attribute to work anywhere in this pester test (The UpdateGithubReleaseCommonParams gets used as a splat in future It statements and it's not getting recognized). Guidance would be appreciated but I don't think it'll work.
https://github.com/JustinGrote/Press/blob/main/Source/Public/Update-GithubRelease.Tests.ps1
I had to fully disable the rule because I couldn't get a suppress attribute to work anywhere in this pester test
Yeah, ScriptAnalyzer doesn't understand Pester syntax, so it can't tell that those script blocks all execute in the same context...
@jaykul A workaround is to set the variables in the BeforeAll at Script scope it seems.
This may have already been pointed out, but I've noticed that when this issue occurs (with preference variables at least, e.g. $Transcript
), in addition to assigning the variables in BeforeAll
, another workaround seems to be identifying the scope of the variable.
So, for example:
Where assigning but 'never using' $Transcript
is identified as an error in VS Code, alternatively assigning $script:Transcript
or $global:Transcript
does not get the dreaded red squigglies. 😀
I'm assembling this issue as more of a map/canonical issue for all the issues we have today around the
PSUseDeclaredVarsMoreThanAssignments
rule, just so it's easier to deduplicate new issues and consolidate some of the discussion and explanation around this rule.You might notice we have a dedicated label for this rule, and that's because its false positives come up a lot. There's an explanation of those below, but first some links to specific issues:
I've written an explanation of some of this before in this comment: https://github.com/PowerShell/PSScriptAnalyzer/issues/711#issuecomment-535304534. However, since this is intended to be a reference issue, I'll expand on that here.
Why doesn't PSUseDeclaredVarsMoreThanAssignments work?
PowerShell has something called dynamic scope, meaning scopes are inherited based on the runtime call stack, rather than the lexical scope of the script. So when a variable is referenced, its value is determined not by reading up the page of the editor (where was
$x
defined last in my script) but by the caller (when was$x
defined last at runtime). This is technically impossible to analyse generally because it's possible to pick up variable values on the fly from any caller.For example:
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:
Or even:
In fact this is further complicated because:
global:
,script:
,local:
.PowerShell doesn't just pick up variable values dynamically, it can also set them dynamically. Consider:
Or indeed:
And perhaps that could be analysed, but what about this:
In this scenario, there's no way to know if
$x
was set without examining the local filesystem. And we could make it worse, because we could get the variable name from a web API or from Active Directory or absolutely anything and the only way to know the value would be to run the script, which probably does things, so we can't do that (and if we could, how do we know we'd get the same results -- the web API might return something different for example). Meaning we simply can't know.But of course there are some scenarios where a human can read the script and know, meaning that a sufficiently enlightened analyser could also know. What's the solution there? Probably to special-case those scenarios, but that takes time and work and diligence (things that seem simple to read as a human are often quite painstaking to implement as a PSScriptAnalyzer rule).
If it's not possible for it to always get things right, what's the point?
So based on the above, we've said this is an impossible problem to solve in general. Maybe we can special case things, but what's the point if there are always going to be things we can't determine?
Well there are a few reasons:
Finally, like with all linters, the point is also to help you improve your code style. If you hit a case that's bad enough that
PSUseDeclaredVarsMoreThanAssignments
gets it wrong, it's also an indicator that your code is hard to analyse (possibly also to a human), and that it might be worth reconsidering the pathological construct you've used. An example:Here the rule can't see into
Invoke-Expression
's string argument to know that$x
is being referenced, but the scripter really shouldn't be usingInvoke-Expression
, so the warning is helpful if a bit indirect.Ok, so what now?
Well we've consolidated the issues into a few scenarios, and in some cases there have even been some attempts to fix them:
We have a reasonable idea of what the cases are, but the work-to-result ratio has meant we haven't been able to prioritise the work. On the other hand, PSScriptAnalyzer always accepts contributions and the maintainers are here to help!