Add new AvoidDynamicallyCreatingVariableNames rule

[iRon7]

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

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

[liamjpeters]

👋

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

	/// <summary>
	/// Given a cmdlet, return the list of all the aliases.
	/// Also include the original name in the list.
	/// </summary>
	/// <param name="Cmdlet">Name of the cmdlet</param>
	/// <returns></returns>
	public List<String> CmdletNameAndAliases(String Cmdlet)
	{
	List<String> results = new List<String>();
	results.Add(Cmdlet);
	if (CmdletToAliasDictionary.ContainsKey(Cmdlet))
	{
	results.AddRange(CmdletToAliasDictionary[Cmdlet]);
	}
	return results;
	}

And a sample usage here:

PSScriptAnalyzer/Rules/AvoidGlobalAliases.cs

Lines 95 to 96 in a143b9f

	var AliasList = Helper.Instance.CmdletNameAndAliases("New-Alias");
	if (AliasList.Contains(commandAst.GetCommandName()))

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.

[iRon7]

@liamjpeters,

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

[Copilot]

Pull request overview

Adds a new built-in PSScriptAnalyzer rule intended to discourage dynamically created variable names (favoring hashtables/dictionaries), along with documentation and Pester coverage.

Changes:

• Add new AvoidDynamicallyCreatingVariableNames rule implementation and localized strings.
• Add rule documentation page and include it in the rules index.
• Add Pester tests covering violations, compliant cases, and suppression.

Reviewed 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.

[iRon7]

@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? 😆