-
Notifications
You must be signed in to change notification settings - Fork 389
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
Warn on Undeclared Variable #483
Comments
I afraid this is going to be noise. PowerShell allows using of global or parent scope variables. Thus, use of an unassigned variable is more likely by design. Otherwise, it is a bug which is fixed sooner or later (strict mode helps to discover such cases). Thus, the remaining cases are intentional and they should not cause warnings. |
I agree, I'm concerned that this would be noise as well, since variables can be defined and assigned values in so many different locations. |
I definitely see your point, but if this warning is noise, is the reverse not also true - that the current "UseDeclaredVarsMoreThanAssignments" rule is also just noise? |
I cannot imagine a practical case when assigning a variable and not using it is by design. But using not assigned parent scope variables or variables created without declaring is quite common. |
I could assign variable values in one script but call them in another, or just export them from a module. In fact, I could have a module of nothing but variables. That's an extreme case, but you see the point. Some modules I've created have a "config" file, which is nothing more than some variables that can be configured to support functionality in the module. In this case, PSScriptAnalyzer will flag the variables in this script because they haven't been called anywhere else in the script. |
Yes, I see the point now. You are right, it can be noise, too. Looking at my scripts through the analyser, I do not have too many such cases. On the other hand, I have quite a lot of use of "unassigned". Well, maybe it's just me. |
Depending on the day of the week, I seem to have scripts that have one or the other of these two cases, but rarely both. ;) So I guess the question is: Should we have as many potentially useful checks as possible and let the users decide which ones to run, or try to pare these down to universally agreed upon checks? If memory serves this question has been answered to some degree elsewhere, just can't remember where. |
We had this rule as part of the default rule set originally. People found this to be very noisy. |
Ok, sounds like the agreement is to leave this out. I'll concede the point! |
Sorry for necro-ing this post, but is there any way to include this rule for analysis of scripts? I find myself getting burned more often by the sloppy typing than whatever the noise could be.
Isn't this a best practice analyzer? Those all sound like hard to maintain or "bad" practices. |
I'd love to see this rule too. |
When you guys say you want this rule, how are you using PSSA?
Also: In either cases, do you use the out of the box experience or a settings file? Whilst we could re-add the rule, given historic feedback the rule would probably not be turned on by default both in the editor and probably not in the command line experience either. Happy to re-open the issue but let's establish some ground/expectations first. Cc @rjmholt @SydneyhSmith @JamesWTruher |
Thanks for a quick reply! |
@bergmeister - I do some in Visual Studio via an External Tool I hooked into the menus, but mostly in vscode now. |
I'd like to be able to run this rule on builds. Largely because uncovering bugs due to use of externally defined variables is really hard. |
This definitively should be one of the available rules & whilst it can be a noisy rule it can and will help in finding issues early and I would like it to be an optional rule that was available to use |
Ditto; It makes sense that it would be noisy in CI systems, but I think it could be very helpful in code editor linting; I primarily use in vscode and almost got burned by an undefined variable with no warning, googled this, and ended up here! What about .\ dependency resolution for avoiding noise? or a non-default AvoidUninitializedVariable flag that would primarily be in code editors? |
@rjmholt Has a great explanation about why this is a hard rule to get right. |
This comes up again and again on the PowerShell discord. Yeah it's probably gonna be real finicky. Yeah there's not a chance we can make it a default in VSCode. But it'd still be great to have. The engine already has some logic for this built in for classes. While far from perfect, it'd be nice to surface the logic from that semantic check into an optional analyzer. |
There is currently a rule that warns when I have assigned a variable but haven't called it (UseDeclaredVarsMoreThanAssignments), however, there doesn't appear to be a rule that warns me if I have used a variable that hasn't been assigned.
Is this a possibility? I can definitely see there would be some challenges around implementing it correctly, but this would be a useful warning.
The text was updated successfully, but these errors were encountered: