Add new AvoidDynamicallyCreatingVariableNames rule - #2178

👋

I get that this rule is warning against using New-\Set-Variable with a non-constant name. What's the issue with dynamic variable names? Could someone use the variable drive and Set-Item to get around this? Something like: $name='foo'; Set-Item -Path "Variable:$name" -Value 1?

I think I have one bit of code in an internal module that this rule would flag. It basically hoists a bunch of variables into a caller-visible scope. I think it's perfectly legitimate? The gist of it is:

foreach ($name in $NamesToExpose) {
    Set-Variable -Scope Script -Name $name -Value $data[$name]
}

That aside...

This misses alias-based creation and update calls. e.g. nv -> New-Variable.

There's a helper function for this:

And a sample usage here:

You have the same copy-pasta in the class and AnalyzeScript doc-comment (references to reserved words as function names). There's also references to finding all of the FunctionDefinitionAst - something this rule isn't concerned with.

I know it works as you're statically invoking parameter binding - but perhaps a test which uses positional binding $myVarName = 'foo'; New-Variable $myVarName - just to future proof it against regressions outside of your code.

Liam Peters (@liamjpeters),

Thank you for your feedback,
I will investigate your finding and further reply tomorrow.

Liam Peters (@liamjpeters),

What's the issue with dynamic variable names?

It is meant as a best practice guidance for users that are not known with hash tables, as in e.g. https://stackoverflow.com/q/68827910/1701026. There are probably a lot of ways around it but I am not trying to completely close the door on dynamically creating variable names, but only want to prevent beginners from unknowingly choosing the wrong direction and possibly run into a pitfall. I don't think that the audience I am trying to reach with this rule will use something like Set-Item -Path "Variable:$name" for this.

I think it's perfectly legitimate?

Yes, your example of Set-Variable is probably perfectly legitimate (I don't have the complete background). The problem here is that you can create a "New-Variable" with Set-Variable without knowing it (imho, I think this should actually return an error like "the variable doesn't exist" or require a Set-Variable -force parameter)
Anyways, I:

• removed the Set-Variable from the scope as it might indeed lead to false positives (just as Set-Item might result in unforeseen false positives)
• renamed the rule to AvoidDynamicallyCreatingVariableNames (I hope I have covered all references)
• lowered the severity to Information

This misses alias-based creation and update calls

Implemented

You have the same copy-pasta in the class and AnalyzeScript doc-comment

Changed

but perhaps a test which uses positional binding

Added

And now Copilot also kicks in with suggestions... will I ever be able to finish this? 😆

Andy Jordan (@andyleejordan)

Opt-in by default.
...
our convention is to ship it opt-in via ConfigurableRule with Enable = false.

This morning, I have also added an additional #2183 PR.
Despite the fact that I based that one on the ConfigurableRule class, I also enabled it by default.
Now going though each PR feedback one-by-one, it gets clear that the general policy is to avoid the IScriptRule interface.
If this is a convention, I would certainly consider to put that in one of the general test so that authors do not have to revise their cs rules (and everything that comes with that, rule.md, README.md and related tests)
(and, Liam Peters (@liamjpeters), you might want to make that clear in your blogpost too.)

Although, I see the value of preferring a ConfigurableRule above a IScriptRule, I don't like the idea of disabling all new rules by default. My effort (which definitely has taken more time than anticipated at first) in writing these rules was intended to reach the PowerShell(starter) audience and prevent pitfalls (mainly by using VSCode). If those rules are disabled by default, I find it unlikely to believe that they will actually reach this audience and wonder whether this whole exercise has been a waste of time for me.
Btw, I suspect that the author of #2109 also lost his interest due to amount of effort it takes to get a formal rule PR through the process. Yet, the PSAvoidUsingNewObject rule is a perfect example where you want to wake-up the community in improving the PowerShell syntax and performance.
I was planned to finish this rule and I do see the advantage of making it configurable (I was even thinking of implementing an automated corrective action/fix), but if it also needs to be disabled by default, I won't put any effort in it...

Maybe a general solution (e.g. a setting like DisableRulesNewerThan = <date>) could supply a way out of this...

Christoph Bergmeister (@bergmeister),

I am not sure we need a rule for this to warn when using New-Variable

This https://stackoverflow.com/q/68827910/1701026 StackOverflow question, shows where a novice scripter could be prevented to run into the wrong direction. But, I agree, even experienced PowerShell programmers might choose to shoot in their own foot 🤪. See: PowerShell/PowerShell#27023