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

PSAvoidUninitializedVariable rule has very questionable usefulness #189

Closed
KirkMunro opened this Issue May 21, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@KirkMunro
Contributor

KirkMunro commented May 21, 2015

PSAvoidUninitializedVariable is currently causing more harm than good because it is trying to overreach its bounds. It generates a warning for variables that are properly initialized in other files (this happens when I use snippets). It generates warnings for function parameters that are not initialized (these should not be initialized as a best practice). It generates warnings if you use -ov to store the results from a command in a variable (although that should generate a warning about the -ov parameter alias instead). It generates warnings if you use Set-Variable to set the value of a variable in a scope other than the current scope. Each of these scenarios can and do happen in scripts, and none of them should generate warnings.

To further complicate the issue, if you want to suppress the warning in a script module (psm1) file, you have no choice but to suppress all instances of that warning. As a best practice, that would be the wrong thing to do. You should suppress only the specific instances where this rule fails to identify that a variable is initialized, or when this rule identifies that having a variable that is uninitialized is a bad thing when in fact it is not (parameters).

I really dislike this rule as a result of these issues, and seriously question its usefulness. I feel there are too many scenarios where this rule will show up when you don't want it. This rule by far generates the most noise across all of my modules that I am performing analysis on as well.

All of this raises an important question: where does PSScriptAnalyzer start overstepping its bounds? For example, this rule is about identifying variables that are used when they are uninitialized. But Set-StrictMode -Version Latest does that very thing, and it does it very well, at runtime. When you use strict mode, you will get an error if you try to reference a property or method on an uninitialized variable. I use strict mode as a best practice. That, combined with a proper test harness (Pester or something else) is a better solution to this problem than this rule, because those solutions will properly and accurately identify when a variable is used that is not initialized. And since they exist, why duplicate that effort in PSScriptAnalyzer in a semi-functional, noisy way where it shows up in places where you don't want it?

That said, I get the value of identifying these issues through static analysis. If you keep this rule around (which I suspect you will), you absolutely have to fix it so that it doesn't generate warnings on parameters, and you need a better suppression mechanism so that you can suppress it with the appropriate restrictions (for example, I would suppress it broadly for all use of PSModule and PSModulePath for my modules, because I initialize those in an external snippet. The current module does not seem to support a suppression like this (for specific variable names across any analysis I run), nor does it support suppression of this for specific variables in a script module file.

I felt this needed to be discussed with the community, so please share your thoughts if you have any to share.

@quoctruong

This comment has been minimized.

Show comment
Hide comment
@quoctruong

quoctruong May 21, 2015

Hi Kirk,

We will spend some time discussing this rule. In the meantime, to suppress this rule for a specific variable, you can do this:

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable", "variablename")].

This will suppress the rule for all variables with the name variablename.

Hi Kirk,

We will spend some time discussing this rule. In the meantime, to suppress this rule for a specific variable, you can do this:

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable", "variablename")].

This will suppress the rule for all variables with the name variablename.

@KirkMunro

This comment has been minimized.

Show comment
Hide comment
@KirkMunro

KirkMunro May 22, 2015

Contributor

That works for a single variable, but only a single variable. If I enter
multiple SuppressMessageAttributes, each one for a different variable for
which I want to suppress this rule, only the last one is respected.

e.g. If I enter this at the top of my module:

[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUninitializedVariable','PSModuleRoot')]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUninitializedVariable','PSModule')]
param()

The only variable for which this rule will be suppressed is PSModule.

There really needs to be a way to suppress rules like this for multiple
modules.

*Kirk Munro *
Poshoholic, Microsoft MVP
Blog http://poshoholic.com/ | Twitter http://twitter.com/poshoholic |
LinkedIn http://ca.linkedin.com/in/kirkmunro | GitHub
http://github.com/KirkMunro | Facebook
http://www.facebook.com/#%21/kirk.munro

Need a PowerShell SME for a project at work? Contact me
http://poshoholic.com/contact-me/!

On Thu, May 21, 2015 at 5:40 PM, Quoc Truong notifications@github.com
wrote:

Hi Kirk,

We will spend some time discussing this rule. In the meantime, to suppress
this rule for a specific variable, you can do this:

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable",
"variablename")].


Reply to this email directly or view it on GitHub
#189 (comment)
.

Contributor

KirkMunro commented May 22, 2015

That works for a single variable, but only a single variable. If I enter
multiple SuppressMessageAttributes, each one for a different variable for
which I want to suppress this rule, only the last one is respected.

e.g. If I enter this at the top of my module:

[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUninitializedVariable','PSModuleRoot')]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUninitializedVariable','PSModule')]
param()

The only variable for which this rule will be suppressed is PSModule.

There really needs to be a way to suppress rules like this for multiple
modules.

*Kirk Munro *
Poshoholic, Microsoft MVP
Blog http://poshoholic.com/ | Twitter http://twitter.com/poshoholic |
LinkedIn http://ca.linkedin.com/in/kirkmunro | GitHub
http://github.com/KirkMunro | Facebook
http://www.facebook.com/#%21/kirk.munro

Need a PowerShell SME for a project at work? Contact me
http://poshoholic.com/contact-me/!

On Thu, May 21, 2015 at 5:40 PM, Quoc Truong notifications@github.com
wrote:

Hi Kirk,

We will spend some time discussing this rule. In the meantime, to suppress
this rule for a specific variable, you can do this:

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable",
"variablename")].


Reply to this email directly or view it on GitHub
#189 (comment)
.

@quoctruong

This comment has been minimized.

Show comment
Hide comment
@quoctruong

quoctruong May 29, 2015

This is actually a bug with the suppression feature. This is fixed with #231

This is actually a bug with the suppression feature. This is fixed with #231

@raghushantha

This comment has been minimized.

Show comment
Hide comment
@raghushantha

raghushantha Aug 26, 2015

Member

As per community FB, this rule has been deprecated.
#296

Member

raghushantha commented Aug 26, 2015

As per community FB, this rule has been deprecated.
#296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment