diff --git a/Rules/AvoidDynamicVariableNames.cs b/Rules/AvoidDynamicVariableNames.cs new file mode 100644 index 000000000..716069eb8 --- /dev/null +++ b/Rules/AvoidDynamicVariableNames.cs @@ -0,0 +1,89 @@ +// 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.Management.Automation.Language; +using System.Linq; + +#if !CORECLR +using System.ComponentModel.Composition; +#endif + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + + /// + /// Rule that warns when reserved words are used as function names + /// + public class AvoidDynamicVariableNames : IScriptRule + { + /// + /// Analyzes the PowerShell AST for uses of reserved words as function names. + /// + /// The PowerShell Abstract Syntax Tree to analyze. + /// The name of the file being analyzed (for diagnostic reporting). + /// A collection of diagnostic records for each violation. + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + // Find all FunctionDefinitionAst in the Ast + var newVariableAsts = ast.FindAll(testAst => + testAst is CommandAst cmdAst && + ( + String.Equals(cmdAst.GetCommandName(), "New-Variable", StringComparison.OrdinalIgnoreCase) || + String.Equals(cmdAst.GetCommandName(), "Set-Variable", StringComparison.OrdinalIgnoreCase) + ), + true + ); + + 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.AvoidDynamicVariableNamesError, + variableName), + newVariableAst.Parent.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + variableName + ); + } + } + + public string GetCommonName() => Strings.AvoidDynamicVariableNamesCommonName; + + public string GetDescription() => Strings.AvoidDynamicVariableNamesDescription; + + public string GetName() => string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidDynamicVariableNamesName); + + public RuleSeverity GetSeverity() => RuleSeverity.Warning; + + public string GetSourceName() => Strings.SourceName; + + public SourceType GetSourceType() => SourceType.Builtin; + } +} \ No newline at end of file diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 2a04fd759..e493cfd91 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}' + + AvoidDynamicVariableNames + + + Avoid dynamic variable names + + + Do not create variables with a dynamic name, 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/Rules/AvoidDynamicVariableNames.tests.ps1 b/Tests/Rules/AvoidDynamicVariableNames.tests.ps1 new file mode 100644 index 000000000..19e0f33c6 --- /dev/null +++ b/Tests/Rules/AvoidDynamicVariableNames.tests.ps1 @@ -0,0 +1,86 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +[Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'False positive')] +param() + +BeforeAll { + $ruleName = "PSAvoidDynamicVariableNames" + $ruleMessage = "'{0}' is a dynamic variable name. Please, avoid creating variables with a dynamic name" +} + +Describe "AvoidDynamicVariableNames" { + Context "Violates" { + It "Basic dynamic variable name" { + $scriptDefinition = { New-Variable -Name $Test }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $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 -IncludeRule @($ruleName) + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {New-Variable -Name "My$_" -Value ($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 -IncludeRule @($ruleName) + $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 -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + } + + Context "Suppressed" { + It "Basic dynamic variable name" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicVariableNames', '$Test', Justification = 'Test')] + Param() + New-Variable -Name $Test + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + It "Common dynamic variable iteration" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicVariableNames', '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 -IncludeRule @($ruleName) + $violations | Should -BeNullOrEmpty + } + } +} \ No newline at end of file diff --git a/docs/Rules/AvoidDynamicVariableNames.md b/docs/Rules/AvoidDynamicVariableNames.md new file mode 100644 index 000000000..05086fd6a --- /dev/null +++ b/docs/Rules/AvoidDynamicVariableNames.md @@ -0,0 +1,51 @@ +--- +description: Avoid dynamic variable names, instead use a hash table or similar dictionary type. +ms.date: 04/21/2026 +ms.topic: reference +title: AvoidDynamicVariableNames +--- +# AvoidDynamicVariableNames + +**Severity Level: Warning** + +## Description + +Do not create variables with a dynamic name, this might introduce conflicts with +other variables and is difficult to maintain. + +## How + +Use a hash table or similar dictionary type to store values with dynamic keys. + +## 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 +``` + +When it concerns a specific scope, option or visibility, put the concerned dictionary (hash table) in that +scope, option or visibility. In example, if the values should be read only and available in the script scope, +put the _hash table_ 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 +``` \ No newline at end of file diff --git a/docs/Rules/README.md b/docs/Rules/README.md index fca031e33..ee82f511f 100644 --- a/docs/Rules/README.md +++ b/docs/Rules/README.md @@ -34,6 +34,7 @@ The PSScriptAnalyzer contains the following rule definitions. | [AvoidUsingConvertToSecureStringWithPlainText](./AvoidUsingConvertToSecureStringWithPlainText.md) | Error | Yes | | | [AvoidUsingDeprecatedManifestFields](./AvoidUsingDeprecatedManifestFields.md) | Warning | Yes | | | [AvoidUsingDoubleQuotesForConstantString](./AvoidUsingDoubleQuotesForConstantString.md) | Information | No | | +| [AvoidUsingDynamicVariableNames](./AvoidUsingDynamicVariableNames.md) | Warning | Yes | | | [AvoidUsingEmptyCatchBlock](./AvoidUsingEmptyCatchBlock.md) | Warning | Yes | | | [AvoidUsingInvokeExpression](./AvoidUsingInvokeExpression.md) | Warning | Yes | | | [AvoidUsingPlainTextForPassword](./AvoidUsingPlainTextForPassword.md) | Warning | Yes | |