diff --git a/Rules/AvoidDynamicallyCreatingVariableNames.cs b/Rules/AvoidDynamicallyCreatingVariableNames.cs new file mode 100644 index 000000000..5e5df7566 --- /dev/null +++ b/Rules/AvoidDynamicallyCreatingVariableNames.cs @@ -0,0 +1,141 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Management.Automation.Language; + +#if !CORECLR +using System.ComponentModel.Composition; +#endif + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// Rule that informs the user when they create variables with dynamic names in the general variable scope. + /// This might lead to conflicts with other variables. + /// +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + public class AvoidDynamicallyCreatingVariableNames : ConfigurableRule + { + + /// + /// Construct an object of AvoidDynamicallyCreatingVariableNames type. + /// + public AvoidDynamicallyCreatingVariableNames() { + Enable = false; + } + + readonly HashSet cmdList = new HashSet(Helper.Instance.CmdletNameAndAliases("New-Variable"), StringComparer.OrdinalIgnoreCase); + + /// + /// Analyzes the given ast to find the [violation] + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations + public override IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + // Find all "New-Variable" commands in the Ast + IEnumerable newVariableAsts = ast.FindAll(testAst => + testAst is CommandAst cmdAst && + cmdList.Contains(cmdAst.GetCommandName()), + true + ).Cast(); + + foreach (CommandAst newVariableAst in newVariableAsts) + { + // Use StaticParameterBinder to reliably get parameter values + var bindingResult = StaticParameterBinder.BindCommand(newVariableAst, true); + if (!bindingResult.BoundParameters.ContainsKey("Name")) { continue; } + var nameBindingResult = bindingResult.BoundParameters["Name"]; + // Dynamic parameters return null for the ConstantValue property + if (nameBindingResult.ConstantValue != null) { continue; } + string variableName = nameBindingResult.Value.ToString(); + if (variableName.StartsWith("\"") && variableName.EndsWith("\"")) + { + variableName = variableName.Substring(1, variableName.Length - 2); + } + yield return new DiagnosticRecord( + string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidDynamicallyCreatingVariableNamesError, + variableName), + newVariableAst.Extent, + GetName(), + DiagnosticSeverity.Information, + fileName, + variableName + ); + } + } + + /// + /// Retrieves the common name of this rule. + /// + public override string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDynamicallyCreatingVariableNamesCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public override string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDynamicallyCreatingVariableNamesDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public override string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidDynamicallyCreatingVariableNamesName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public override RuleSeverity GetSeverity() + { + return RuleSeverity.Information; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Information; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public override string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public override SourceType GetSourceType() + { + return SourceType.Builtin; + } + } +} diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 2a04fd759..4b0ab5aa5 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -873,6 +873,18 @@ The type accelerator '{0}' is not available by default in PowerShell version '{1}' on platform '{2}' + + AvoidDynamicallyCreatingVariableNames + + + Avoid dynamically creating variable names + + + Do not create variables with a dynamic name, as this might introduce conflicts with other variables and is difficult to maintain. + + + '{0}' is a dynamic variable name. Please avoid creating variables with a dynamic name + Avoid global functions and aliases diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 422b585bf..789bc3548 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -152,7 +152,7 @@ Describe "TestSeverity" { It "filters rules based on multiple severity inputs"{ $rules = Get-ScriptAnalyzerRule -Severity Error,Information - $rules.Count | Should -Be 19 + $rules.Count | Should -Be 20 } It "takes lower case inputs" { diff --git a/Tests/Rules/AvoidDynamicallyCreatingVariableNames.tests.ps1 b/Tests/Rules/AvoidDynamicallyCreatingVariableNames.tests.ps1 new file mode 100644 index 000000000..1a47abbac --- /dev/null +++ b/Tests/Rules/AvoidDynamicallyCreatingVariableNames.tests.ps1 @@ -0,0 +1,164 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +[Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'False positive')] +[Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidUsingCmdletAliases', 'nv', Justification = 'For test purposes')] +param() + +BeforeAll { + $ruleName = "PSAvoidDynamicallyCreatingVariableNames" + $ruleMessage = "'{0}' is a dynamic variable name. Please avoid creating variables with a dynamic name" +} + +Describe "AvoidDynamicallyCreatingVariableNames" { + + BeforeAll { + $Settings = @{ + IncludeRules = @($ruleName) + Rules = @{ $ruleName = @{ Enable = $true } } + } + } + + Context "Violates" { + It "Basic dynamic variable name" { + $scriptDefinition = { New-Variable -Name $Test }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Information + $violations.Extent.Text | Should -Be {New-Variable -Name $Test}.ToString() + $violations.Message | Should -Be ($ruleMessage -f '$Test') + } + + It "Using alias" { + $scriptDefinition = { nv -Name $Test }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Information + $violations.Extent.Text | Should -Be {nv -Name $Test}.ToString() + $violations.Message | Should -Be ($ruleMessage -f '$Test') + } + + It "Using uppercase" { + $scriptDefinition = { NEW-VARIABLE -Name $Test }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Information + $violations.Extent.Text | Should -Be {NEW-VARIABLE -Name $Test}.ToString() + $violations.Message | Should -Be ($ruleMessage -f '$Test') + } + + It "Common dynamic variable iteration" { + $scriptDefinition = { + 'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + New-Variable -Name "My$_" -Value ($i++) + } + $MyTwo # returns 2 + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Information + $violations.Extent.Text | Should -Be {New-Variable -Name "My$_" -Value ($i++)}.ToString() + $violations.Message | Should -Be ($ruleMessage -f 'My$_') + } + + It "Unquoted positional binding" { + $scriptDefinition = { + $myVarName = 'foo' + New-Variable $myVarName + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Information + $violations.Extent.Text | Should -Be {New-Variable $myVarName}.ToString() + $violations.Message | Should -Be ($ruleMessage -f '$myVarName') + } + + It "Quoted positional binding" { + $scriptDefinition = { + 'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + New-Variable "My$_" ($i++) + } + $MyTwo # returns 2 + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Information + $violations.Extent.Text | Should -Be {New-Variable "My$_" ($i++)}.ToString() + $violations.Message | Should -Be ($ruleMessage -f 'My$_') + } + } + + Context "Compliant" { + It "Common hash table population" { + $scriptDefinition = { + $My = @{} + 'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + $My[$_] = $i++ + } + $My.Two # returns 2 + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "Scoped hash table population" { + $scriptDefinition = { + New-Variable -Name My -Value @{} -Option ReadOnly -Scope Script + 'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + $Script:My[$_] = $i++ + } + $Script:My.Two # returns 2 + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "Verbatim (single quoted) name with dollar sign" { + $scriptDefinition = { + New-Variable -Name '$Sign1' + New-Variable -Name '$Sign2' -Value 'Dollar' + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + } + + Context "Disabled" { + BeforeAll { + $Settings = @{ + IncludeRules = @($ruleName) + Rules = @{ $ruleName = @{ Enable = $false } } + } + } + + It "ConvertFrom-SecureString -AsPlainText" { + $scriptDefinition = { New-Variable -Name $Test }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + } + + Context "Suppressed" { + It "Basic dynamic variable name" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicallyCreatingVariableNames', '$Test', Justification = 'Test')] + Param() + New-Variable -Name $Test + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + It "Common dynamic variable iteration" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicallyCreatingVariableNames', 'My$_', Justification = 'Test')] + Param() + 'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + New-Variable -Name "My$_" -Value ($i++) + } + $MyTwo # returns 2 + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + } +} \ No newline at end of file diff --git a/docs/Rules/AvoidDynamicallyCreatingVariableNames.md b/docs/Rules/AvoidDynamicallyCreatingVariableNames.md new file mode 100644 index 000000000..22183332f --- /dev/null +++ b/docs/Rules/AvoidDynamicallyCreatingVariableNames.md @@ -0,0 +1,77 @@ +--- +description: Avoid dynamic variable names, instead use a hash table or similar dictionary type. +ms.date: 04/21/2026 +ms.topic: reference +title: AvoidDynamicallyCreatingVariableNames +--- +# AvoidDynamicallyCreatingVariableNames + +**Severity Level: Information** + +## Description + +Don't create variables with dynamic names. It also makes the code difficult to understand and can +lead to unexpected behavior if the variable names are not unique or if they collide with existing +variables. A dynamic name is a name constructed using string concatenation or interpolation. +This rule checks for the use of `New-Variable` with a dynamic name. + +> [!NOTE] +> This rule is not enabled by default. The user needs to enable it through settings. + +## How to Fix + +Use a hash table or similar dictionary type to store values with dynamic keys. When you require a +specific scope, option, or visibility, put the dictionary (hashtable) in that scope and apply the +appropriate option or visibility. + +## Example + +### Wrong + +```powershell +'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + New-Variable -Name "My$_" -Value ($i++) +} +$MyTwo # returns 2 +``` + +### Correct + +```powershell +$My = @{} +'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + $My[$_] = $i++ +} +$My.Two # returns 2 +``` + +In this example, you want the values to be read-only and available in the script scope. +Put the hashtable in the script scope and make it read-only. + +```powershell +New-Variable -Name My -Value @{} -Option ReadOnly -Scope Script +'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process { + $Script:My[$_] = $i++ +} +$Script:My.Two # returns 2 +``` + +## Configuration + +```powershell +Rules = @{ + PSAvoidExclaimOperator = @{ + Enable = $true + } +} +``` + +### Parameters + +- `Enable`: **bool** (Default value is `$false`) + + Enable or disable the rule during ScriptAnalyzer invocation. + +## References +- [New-Variable](xref:Microsoft.PowerShell.Utility.New-Variable) + diff --git a/docs/Rules/README.md b/docs/Rules/README.md index fca031e33..5fef48f65 100644 --- a/docs/Rules/README.md +++ b/docs/Rules/README.md @@ -14,6 +14,7 @@ The PSScriptAnalyzer contains the following rule definitions. | [AvoidAssignmentToAutomaticVariable](./AvoidAssignmentToAutomaticVariable.md) | Warning | Yes | | | [AvoidDefaultValueForMandatoryParameter](./AvoidDefaultValueForMandatoryParameter.md) | Warning | Yes | | | [AvoidDefaultValueSwitchParameter](./AvoidDefaultValueSwitchParameter.md) | Warning | Yes | | +| [AvoidDynamicallyCreatingVariableNames](./AvoidDynamicallyCreatingVariableNames.md) | Information | No | Yes | | [AvoidExclaimOperator](./AvoidExclaimOperator.md) | Warning | No | | | [AvoidGlobalAliases1](./AvoidGlobalAliases.md) | Warning | Yes | | | [AvoidGlobalFunctions](./AvoidGlobalFunctions.md) | Warning | Yes | |