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

Warn on Undeclared Variable #483

Open
mattmcnabb opened this issue Mar 29, 2016 · 19 comments
Open

Warn on Undeclared Variable #483

mattmcnabb opened this issue Mar 29, 2016 · 19 comments

Comments

@mattmcnabb
Copy link
Contributor

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.

@nightroman
Copy link

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.

@KirkMunro
Copy link
Contributor

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.

@mattmcnabb
Copy link
Contributor Author

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?

@nightroman
Copy link

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.

@mattmcnabb
Copy link
Contributor Author

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.

@nightroman
Copy link

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.

@mattmcnabb
Copy link
Contributor Author

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.

@raghushantha
Copy link
Member

We had this rule as part of the default rule set originally. People found this to be very noisy.

https://github.com/PowerShell/PSScriptAnalyzer/blob/development/DeprecatedRules/AvoidUninitializedVariable.cs

@mattmcnabb
Copy link
Contributor Author

Ok, sounds like the agreement is to leave this out. I'll concede the point!

@StingyJack
Copy link

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.

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.

Isn't this a best practice analyzer? Those all sound like hard to maintain or "bad" practices.

@twirlse
Copy link

twirlse commented Jun 20, 2020

I'd love to see this rule too.

@bergmeister
Copy link
Collaborator

bergmeister commented Jun 20, 2020

When you guys say you want this rule, how are you using PSSA?

  • Via Invoke-ScriptAnalyzer
  • Only in VS-Code

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

@twirlse
Copy link

twirlse commented Jun 20, 2020

Thanks for a quick reply!
I'm using PSScriptAnalyzer mainly in vscode. I usually have a setting file per project.
I think that for me it'd be an opt-in feature, not something that I want to have on by default if it too noisy.

@bergmeister bergmeister reopened this Jun 20, 2020
@StingyJack
Copy link

@bergmeister - I do some in Visual Studio via an External Tool I hooked into the menus, but mostly in vscode now.

@simonsabin
Copy link

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.
Agree not by default but I'd like to be able to use it

@kilasuit
Copy link
Contributor

kilasuit commented Feb 3, 2021

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

@cutecycle
Copy link

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?

@sdwheeler
Copy link
Collaborator

@rjmholt Has a great explanation about why this is a hard rule to get right.

@SeeminglyScience
Copy link
Contributor

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.

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

No branches or pull requests