| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
👋 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: PSScriptAnalyzer/Engine/Helper.cs Lines 216 to 233 in a143b9f And a sample usage here: PSScriptAnalyzer/Rules/AvoidGlobalAliases.cs Lines 95 to 96 in a143b9f 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. |
Sorry, something went wrong.
|
Thank you for your feedback, |
Sorry, something went wrong.
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Adds a new built-in PSScriptAnalyzer rule intended to discourage dynamically created variable names (favoring hashtables/dictionaries), along with documentation and Pester coverage.
Changes:
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file| File | Description |
|---|---|
| docs/Rules/README.md | Adds the new rule to the published rules list. |
| docs/Rules/AvoidDynamicallyCreatingVariableNames.md | Introduces end-user documentation and examples for the new rule. |
| Tests/Rules/AvoidDynamicallyCreatingVariableNames.tests.ps1 | Adds test coverage for rule detection and suppression behavior. |
| Rules/Strings.resx | Adds name/common name/description/error strings for the rule. |
| Rules/AvoidDynamicallyCreatingVariableNames.cs | Implements the new analyzer rule logic for detecting dynamic New-Variable -Name usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
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.
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)
Implemented
Changed
Added And now Copilot also kicks in with suggestions... will I ever be able to finish this? 😆 |
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Thanks iRon7, and thanks Liam Peters (@liamjpeters) for the deep review on this one — I think your back-and-forth genuinely improved the rule (narrowing scope to New-Variable, picking up the alias coverage via Helper.CmdletNameAndAliases, dropping severity to Information). All of Liam's points look addressed.
Two follow-ups before I'm comfortable merging:
Opt-in by default. This rule is still derived from IScriptRule, which means it's enabled-by-default for everyone. For a new rule — especially one at Information severity that is going to flag any New-Variable -Name $name in dynamic-config / metaprogramming-style scripts — our convention is to ship it opt-in via ConfigurableRule with Enable = false. Same recipe as AvoidExclaimOperator. Could you switch the base class and update docs/Rules/README.md accordingly?
Second maintainer read. Now that the scope is narrowed to just New-Variable, I'd like Christoph Bergmeister (@bergmeister) or Mikey Lombardi (He/Him) (@michaeltlombardi) to weigh in on whether the rule still earns its keep at this scope, or whether it would be better recombined with the dynamic Set-Item Variable:\… form. Either answer is fine with me — just want a second pair of eyes on the design question before we land it.
Drafted by Copilot (Claude Opus 4.7)
Sorry, something went wrong.
This morning, I have also added an additional #2183 PR. 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. Maybe a general solution (e.g. a setting like DisableRulesNewerThan = <date>) could supply a way out of this... |
Sorry, something went wrong.
There was a problem hiding this comment.
I am not sure we need a rule for this to warn when using New-Variable. PowerShell's own variable nature is dynamic and it can sometimes be a blessing and sometimes unpredictable and hard for PSSA. But it's up to user, there are far more dynamic things in PowerShell that allows users to shoot into their own foot
Sorry, something went wrong.
|
Christoph Bergmeister (@bergmeister),
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 |
Sorry, something went wrong.
… accordingly. Increased number of information tests to 20 in Get-ScriptAnalyzerRule tests.
There was a problem hiding this comment.
Docs look good.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks iRon7 — the opt-in conversion (ConfigurableRule, Enable = false, README Information / No / Yes) and the bumped Information-rule count in the Get-ScriptAnalyzerRule test are in. Approving.
Weighing in on the design question: I agree this is a useful "best practice" rule — exactly the kind of thing I'd turn on for my own code. PowerShell's variable layer is dynamic by nature and there are other forms we don't catch (Set-Item Variable:\…, ${$name} = …, Get-Variable -Name $... | Set-…), but as an opt-in Information-severity rule narrowly scoped to New-Variable -Name $... the value/cost calculus is fine. We can extend the detection surface in follow-ups.
Heads-up: CI on the latest commit (3e546a48) is in action_required state — I'll approve the workflow run so we have a green run on file before merge.
Drafted by Copilot (Claude Opus 4.7)
Sorry, something went wrong.
…atingVariableNames in documentation.
| Back | FazBrowse Home | New Git URL |
PR Summary
New rule to push for the use of hash tables rather than dynamically creating vriables in the general variable pool.
See also: https://stackoverflow.com/a/68830451/1701026
PR Checklist