Skip to content

Commit 4b0117c

Browse files
iRon7CopilotCopilot
authored
Add new AvoidDynamicallyCreatingVariableNames rule (#2178)
* 1st commit * Avoid dynamic variable names rule implementation and tests * Removed `using System.Linq;` and added some tests * Covering Liam's feedback Co-authored-by: Copilot <copilot@github.com> * Update docs/Rules/AvoidDynamicallyCreatingVariableNames.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update docs/Rules/AvoidDynamicallyCreatingVariableNames.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Rules/Strings.resx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Corrected alphabetical order of rules in README.md * Corrected $ruleMessage in Test Co-authored-by: Copilot <copilot@github.com> * Changed newVariableAst.Parent.Extent to newVariableAst.Extent * Made rule configurable (disabled) and updated documentation and tests accordingly. Increased number of information tests to 20 in Get-ScriptAnalyzerRule tests. * Change rule name from PSAvoidExclaimOperator to PSAvoidDynamicallyCreatingVariableNames in documentation. --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent eb6cebe commit 4b0117c

6 files changed

Lines changed: 396 additions & 1 deletion

‎Rules/AvoidDynamicallyCreatingVariableNames.cs‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
5+
using System;
6+
using System.Collections.Generic;
7+
using System.Globalization;
8+
using System.Linq;
9+
using System.Management.Automation.Language;
10+
11+
#if !CORECLR
12+
using System.ComponentModel.Composition;
13+
#endif
14+
15+
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
16+
{
17+
/// <summary>
18+
/// Rule that informs the user when they create variables with dynamic names in the general variable scope.
19+
/// This might lead to conflicts with other variables.
20+
/// </summary>
21+
#if !CORECLR
22+
[Export(typeof(IScriptRule))]
23+
#endif
24+
public class AvoidDynamicallyCreatingVariableNames : ConfigurableRule
25+
{
26+
27+
/// <summary>
28+
/// Construct an object of AvoidDynamicallyCreatingVariableNames type.
29+
/// </summary>
30+
public AvoidDynamicallyCreatingVariableNames() {
31+
Enable = false;
32+
}
33+
34+
readonly HashSet<string> cmdList = new HashSet<string>(Helper.Instance.CmdletNameAndAliases("New-Variable"), StringComparer.OrdinalIgnoreCase);
35+
36+
/// <summary>
37+
/// Analyzes the given ast to find the [violation]
38+
/// </summary>
39+
/// <param name="ast">AST to be analyzed. This should be non-null</param>
40+
/// <param name="fileName">Name of file that corresponds to the input AST.</param>
41+
/// <returns>A an enumerable type containing the violations</returns>
42+
public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
43+
{
44+
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
45+
46+
// Find all "New-Variable" commands in the Ast
47+
IEnumerable<CommandAst> newVariableAsts = ast.FindAll(testAst =>
48+
testAst is CommandAst cmdAst &&
49+
cmdList.Contains(cmdAst.GetCommandName()),
50+
true
51+
).Cast<CommandAst>();
52+
53+
foreach (CommandAst newVariableAst in newVariableAsts)
54+
{
55+
// Use StaticParameterBinder to reliably get parameter values
56+
var bindingResult = StaticParameterBinder.BindCommand(newVariableAst, true);
57+
if (!bindingResult.BoundParameters.ContainsKey("Name")) { continue; }
58+
var nameBindingResult = bindingResult.BoundParameters["Name"];
59+
// Dynamic parameters return null for the ConstantValue property
60+
if (nameBindingResult.ConstantValue != null) { continue; }
61+
string variableName = nameBindingResult.Value.ToString();
62+
if (variableName.StartsWith("\"") && variableName.EndsWith("\""))
63+
{
64+
variableName = variableName.Substring(1, variableName.Length - 2);
65+
}
66+
yield return new DiagnosticRecord(
67+
string.Format(
68+
CultureInfo.CurrentCulture,
69+
Strings.AvoidDynamicallyCreatingVariableNamesError,
70+
variableName),
71+
newVariableAst.Extent,
72+
GetName(),
73+
DiagnosticSeverity.Information,
74+
fileName,
75+
variableName
76+
);
77+
}
78+
}
79+
80+
/// <summary>
81+
/// Retrieves the common name of this rule.
82+
/// </summary>
83+
public override string GetCommonName()
84+
{
85+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDynamicallyCreatingVariableNamesCommonName);
86+
}
87+
88+
/// <summary>
89+
/// Retrieves the description of this rule.
90+
/// </summary>
91+
public override string GetDescription()
92+
{
93+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDynamicallyCreatingVariableNamesDescription);
94+
}
95+
96+
/// <summary>
97+
/// Retrieves the name of this rule.
98+
/// </summary>
99+
public override string GetName()
100+
{
101+
return string.Format(
102+
CultureInfo.CurrentCulture,
103+
Strings.NameSpaceFormat,
104+
GetSourceName(),
105+
Strings.AvoidDynamicallyCreatingVariableNamesName);
106+
}
107+
108+
/// <summary>
109+
/// Retrieves the severity of the rule: error, warning or information.
110+
/// </summary>
111+
public override RuleSeverity GetSeverity()
112+
{
113+
return RuleSeverity.Information;
114+
}
115+
116+
/// <summary>
117+
/// Gets the severity of the returned diagnostic record: error, warning, or information.
118+
/// </summary>
119+
/// <returns></returns>
120+
public DiagnosticSeverity GetDiagnosticSeverity()
121+
{
122+
return DiagnosticSeverity.Information;
123+
}
124+
125+
/// <summary>
126+
/// Retrieves the name of the module/assembly the rule is from.
127+
/// </summary>
128+
public override string GetSourceName()
129+
{
130+
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
131+
}
132+
133+
/// <summary>
134+
/// Retrieves the type of the rule, Builtin, Managed or Module.
135+
/// </summary>
136+
public override SourceType GetSourceType()
137+
{
138+
return SourceType.Builtin;
139+
}
140+
}
141+
}

‎Rules/Strings.resx‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -885,6 +885,18 @@
885885
<data name="UseCompatibleTypesTypeAcceleratorError" xml:space="preserve">
886886
<value>The type accelerator '{0}' is not available by default in PowerShell version '{1}' on platform '{2}'</value>
887887
</data>
888+
<data name="AvoidDynamicallyCreatingVariableNamesName" xml:space="preserve">
889+
<value>AvoidDynamicallyCreatingVariableNames</value>
890+
</data>
891+
<data name="AvoidDynamicallyCreatingVariableNamesCommonName" xml:space="preserve">
892+
<value>Avoid dynamically creating variable names</value>
893+
</data>
894+
<data name="AvoidDynamicallyCreatingVariableNamesDescription" xml:space="preserve">
895+
<value>Do not create variables with a dynamic name, as this might introduce conflicts with other variables and is difficult to maintain.</value>
896+
</data>
897+
<data name="AvoidDynamicallyCreatingVariableNamesError" xml:space="preserve">
898+
<value>'{0}' is a dynamic variable name. Please avoid creating variables with a dynamic name</value>
899+
</data>
888900
<data name="AvoidGlobalFunctionsCommonName" xml:space="preserve">
889901
<value>Avoid global functions and aliases</value>
890902
</data>

‎Tests/Engine/GetScriptAnalyzerRule.tests.ps1‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ Describe "TestSeverity" {
152152

153153
It "filters rules based on multiple severity inputs"{
154154
$rules = Get-ScriptAnalyzerRule -Severity Error,Information
155-
$rules.Count | Should -Be 19
155+
$rules.Count | Should -Be 20
156156
}
157157

158158
It "takes lower case inputs" {

‎Tests/Rules/AvoidDynamicallyCreatingVariableNames.tests.ps1‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
# Copyright (c) Microsoft Corporation. All rights reserved.
2+
# Licensed under the MIT License.
3+
4+
[Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'False positive')]
5+
[Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidUsingCmdletAliases', 'nv', Justification = 'For test purposes')]
6+
param()
7+
8+
BeforeAll {
9+
$ruleName = "PSAvoidDynamicallyCreatingVariableNames"
10+
$ruleMessage = "'{0}' is a dynamic variable name. Please avoid creating variables with a dynamic name"
11+
}
12+
13+
Describe "AvoidDynamicallyCreatingVariableNames" {
14+
15+
BeforeAll {
16+
$Settings = @{
17+
IncludeRules = @($ruleName)
18+
Rules = @{ $ruleName = @{ Enable = $true } }
19+
}
20+
}
21+
22+
Context "Violates" {
23+
It "Basic dynamic variable name" {
24+
$scriptDefinition = { New-Variable -Name $Test }.ToString()
25+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
26+
$violations.Count | Should -Be 1
27+
$violations.Severity | Should -Be Information
28+
$violations.Extent.Text | Should -Be {New-Variable -Name $Test}.ToString()
29+
$violations.Message | Should -Be ($ruleMessage -f '$Test')
30+
}
31+
32+
It "Using alias" {
33+
$scriptDefinition = { nv -Name $Test }.ToString()
34+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
35+
$violations.Count | Should -Be 1
36+
$violations.Severity | Should -Be Information
37+
$violations.Extent.Text | Should -Be {nv -Name $Test}.ToString()
38+
$violations.Message | Should -Be ($ruleMessage -f '$Test')
39+
}
40+
41+
It "Using uppercase" {
42+
$scriptDefinition = { NEW-VARIABLE -Name $Test }.ToString()
43+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
44+
$violations.Count | Should -Be 1
45+
$violations.Severity | Should -Be Information
46+
$violations.Extent.Text | Should -Be {NEW-VARIABLE -Name $Test}.ToString()
47+
$violations.Message | Should -Be ($ruleMessage -f '$Test')
48+
}
49+
50+
It "Common dynamic variable iteration" {
51+
$scriptDefinition = {
52+
'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
53+
New-Variable -Name "My$_" -Value ($i++)
54+
}
55+
$MyTwo # returns 2
56+
}.ToString()
57+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
58+
$violations.Count | Should -Be 1
59+
$violations.Severity | Should -Be Information
60+
$violations.Extent.Text | Should -Be {New-Variable -Name "My$_" -Value ($i++)}.ToString()
61+
$violations.Message | Should -Be ($ruleMessage -f 'My$_')
62+
}
63+
64+
It "Unquoted positional binding" {
65+
$scriptDefinition = {
66+
$myVarName = 'foo'
67+
New-Variable $myVarName
68+
}.ToString()
69+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
70+
$violations.Count | Should -Be 1
71+
$violations.Severity | Should -Be Information
72+
$violations.Extent.Text | Should -Be {New-Variable $myVarName}.ToString()
73+
$violations.Message | Should -Be ($ruleMessage -f '$myVarName')
74+
}
75+
76+
It "Quoted positional binding" {
77+
$scriptDefinition = {
78+
'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
79+
New-Variable "My$_" ($i++)
80+
}
81+
$MyTwo # returns 2
82+
}.ToString()
83+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
84+
$violations.Count | Should -Be 1
85+
$violations.Severity | Should -Be Information
86+
$violations.Extent.Text | Should -Be {New-Variable "My$_" ($i++)}.ToString()
87+
$violations.Message | Should -Be ($ruleMessage -f 'My$_')
88+
}
89+
}
90+
91+
Context "Compliant" {
92+
It "Common hash table population" {
93+
$scriptDefinition = {
94+
$My = @{}
95+
'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
96+
$My[$_] = $i++
97+
}
98+
$My.Two # returns 2
99+
}.ToString()
100+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
101+
$violations | Should -BeNullOrEmpty
102+
}
103+
104+
It "Scoped hash table population" {
105+
$scriptDefinition = {
106+
New-Variable -Name My -Value @{} -Option ReadOnly -Scope Script
107+
'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
108+
$Script:My[$_] = $i++
109+
}
110+
$Script:My.Two # returns 2
111+
}.ToString()
112+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
113+
$violations | Should -BeNullOrEmpty
114+
}
115+
116+
It "Verbatim (single quoted) name with dollar sign" {
117+
$scriptDefinition = {
118+
New-Variable -Name '$Sign1'
119+
New-Variable -Name '$Sign2' -Value 'Dollar'
120+
}.ToString()
121+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
122+
$violations | Should -BeNullOrEmpty
123+
}
124+
}
125+
126+
Context "Disabled" {
127+
BeforeAll {
128+
$Settings = @{
129+
IncludeRules = @($ruleName)
130+
Rules = @{ $ruleName = @{ Enable = $false } }
131+
}
132+
}
133+
134+
It "ConvertFrom-SecureString -AsPlainText" {
135+
$scriptDefinition = { New-Variable -Name $Test }.ToString()
136+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
137+
$violations | Should -BeNullOrEmpty
138+
}
139+
}
140+
141+
Context "Suppressed" {
142+
It "Basic dynamic variable name" {
143+
$scriptDefinition = {
144+
[Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicallyCreatingVariableNames', '$Test', Justification = 'Test')]
145+
Param()
146+
New-Variable -Name $Test
147+
}.ToString()
148+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
149+
$violations | Should -BeNullOrEmpty
150+
}
151+
It "Common dynamic variable iteration" {
152+
$scriptDefinition = {
153+
[Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicallyCreatingVariableNames', 'My$_', Justification = 'Test')]
154+
Param()
155+
'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
156+
New-Variable -Name "My$_" -Value ($i++)
157+
}
158+
$MyTwo # returns 2
159+
}.ToString()
160+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
161+
$violations | Should -BeNullOrEmpty
162+
}
163+
}
164+
}

0 commit comments

Comments
 (0)