Skip to content

Commit 3e546a4

Browse files
committed
Made rule configurable (disabled) and updated documentation and tests accordingly. Increased number of information tests to 20 in Get-ScriptAnalyzerRule tests.
1 parent a20bdf5 commit 3e546a4

5 files changed

Lines changed: 139 additions & 38 deletions

Rules/AvoidDynamicallyCreatingVariableNames.cs

Lines changed: 69 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,32 @@
1414

1515
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
1616
{
17-
#if !CORECLR
18-
[Export(typeof(IScriptRule))]
19-
#endif
20-
2117
/// <summary>
2218
/// Rule that informs the user when they create variables with dynamic names in the general variable scope.
2319
/// This might lead to conflicts with other variables.
2420
/// </summary>
25-
public class AvoidDynamicallyCreatingVariableNames : IScriptRule
21+
#if !CORECLR
22+
[Export(typeof(IScriptRule))]
23+
#endif
24+
public class AvoidDynamicallyCreatingVariableNames : ConfigurableRule
2625
{
26+
2727
/// <summary>
28-
/// Analyzes the PowerShell AST for uses of "New-Variable" command with a dynamic name argument.
28+
/// Construct an object of AvoidDynamicallyCreatingVariableNames type.
2929
/// </summary>
30-
/// <param name="ast">The PowerShell Abstract Syntax Tree to analyze.</param>
31-
/// <param name="fileName">The name of the file being analyzed (for diagnostic reporting).</param>
32-
/// <returns>A collection of diagnostic records for each violation.</returns>
30+
public AvoidDynamicallyCreatingVariableNames() {
31+
Enable = false;
32+
}
3333

3434
readonly HashSet<string> cmdList = new HashSet<string>(Helper.Instance.CmdletNameAndAliases("New-Variable"), StringComparer.OrdinalIgnoreCase);
35-
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
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)
3643
{
3744
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
3845

@@ -70,20 +77,65 @@ testAst is CommandAst cmdAst &&
7077
}
7178
}
7279

73-
public string GetCommonName() => Strings.AvoidDynamicallyCreatingVariableNamesCommonName;
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+
}
7487

75-
public string GetDescription() => Strings.AvoidDynamicallyCreatingVariableNamesDescription;
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+
}
7695

77-
public string GetName() => string.Format(
96+
/// <summary>
97+
/// Retrieves the name of this rule.
98+
/// </summary>
99+
public override string GetName()
100+
{
101+
return string.Format(
78102
CultureInfo.CurrentCulture,
79103
Strings.NameSpaceFormat,
80104
GetSourceName(),
81105
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+
}
82115

83-
public RuleSeverity GetSeverity() => RuleSeverity.Information;
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+
}
84124

85-
public string GetSourceName() => Strings.SourceName;
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+
}
86132

87-
public SourceType GetSourceType() => SourceType.Builtin;
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+
}
88140
}
89-
}
141+
}

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: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,18 @@ BeforeAll {
1111
}
1212

1313
Describe "AvoidDynamicallyCreatingVariableNames" {
14+
15+
BeforeAll {
16+
$Settings = @{
17+
IncludeRules = @($ruleName)
18+
Rules = @{ $ruleName = @{ Enable = $true } }
19+
}
20+
}
21+
1422
Context "Violates" {
1523
It "Basic dynamic variable name" {
1624
$scriptDefinition = { New-Variable -Name $Test }.ToString()
17-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
25+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
1826
$violations.Count | Should -Be 1
1927
$violations.Severity | Should -Be Information
2028
$violations.Extent.Text | Should -Be {New-Variable -Name $Test}.ToString()
@@ -23,7 +31,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
2331

2432
It "Using alias" {
2533
$scriptDefinition = { nv -Name $Test }.ToString()
26-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
34+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
2735
$violations.Count | Should -Be 1
2836
$violations.Severity | Should -Be Information
2937
$violations.Extent.Text | Should -Be {nv -Name $Test}.ToString()
@@ -32,7 +40,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
3240

3341
It "Using uppercase" {
3442
$scriptDefinition = { NEW-VARIABLE -Name $Test }.ToString()
35-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
43+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
3644
$violations.Count | Should -Be 1
3745
$violations.Severity | Should -Be Information
3846
$violations.Extent.Text | Should -Be {NEW-VARIABLE -Name $Test}.ToString()
@@ -46,7 +54,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
4654
}
4755
$MyTwo # returns 2
4856
}.ToString()
49-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
57+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
5058
$violations.Count | Should -Be 1
5159
$violations.Severity | Should -Be Information
5260
$violations.Extent.Text | Should -Be {New-Variable -Name "My$_" -Value ($i++)}.ToString()
@@ -58,7 +66,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
5866
$myVarName = 'foo'
5967
New-Variable $myVarName
6068
}.ToString()
61-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
69+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
6270
$violations.Count | Should -Be 1
6371
$violations.Severity | Should -Be Information
6472
$violations.Extent.Text | Should -Be {New-Variable $myVarName}.ToString()
@@ -72,7 +80,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
7280
}
7381
$MyTwo # returns 2
7482
}.ToString()
75-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
83+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
7684
$violations.Count | Should -Be 1
7785
$violations.Severity | Should -Be Information
7886
$violations.Extent.Text | Should -Be {New-Variable "My$_" ($i++)}.ToString()
@@ -89,7 +97,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
8997
}
9098
$My.Two # returns 2
9199
}.ToString()
92-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
100+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
93101
$violations | Should -BeNullOrEmpty
94102
}
95103

@@ -101,7 +109,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
101109
}
102110
$Script:My.Two # returns 2
103111
}.ToString()
104-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
112+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
105113
$violations | Should -BeNullOrEmpty
106114
}
107115

@@ -110,7 +118,22 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
110118
New-Variable -Name '$Sign1'
111119
New-Variable -Name '$Sign2' -Value 'Dollar'
112120
}.ToString()
113-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
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
114137
$violations | Should -BeNullOrEmpty
115138
}
116139
}
@@ -122,7 +145,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
122145
Param()
123146
New-Variable -Name $Test
124147
}.ToString()
125-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
148+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
126149
$violations | Should -BeNullOrEmpty
127150
}
128151
It "Common dynamic variable iteration" {
@@ -134,7 +157,7 @@ Describe "AvoidDynamicallyCreatingVariableNames" {
134157
}
135158
$MyTwo # returns 2
136159
}.ToString()
137-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
160+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
138161
$violations | Should -BeNullOrEmpty
139162
}
140163
}

docs/Rules/AvoidDynamicallyCreatingVariableNames.md

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,19 @@ title: AvoidDynamicallyCreatingVariableNames
1010

1111
## Description
1212

13-
Do not create variables with a dynamic name, this might introduce conflicts with
14-
other variables and is difficult to maintain.
13+
Don't create variables with dynamic names. It also makes the code difficult to understand and can
14+
lead to unexpected behavior if the variable names are not unique or if they collide with existing
15+
variables. A dynamic name is a name constructed using string concatenation or interpolation.
16+
This rule checks for the use of `New-Variable` with a dynamic name.
1517

16-
## How
18+
> [!NOTE]
19+
> This rule is not enabled by default. The user needs to enable it through settings.
1720
18-
Use a hash table or similar dictionary type to store values with dynamic keys.
21+
## How to Fix
22+
23+
Use a hash table or similar dictionary type to store values with dynamic keys. When you require a
24+
specific scope, option, or visibility, put the dictionary (hashtable) in that scope and apply the
25+
appropriate option or visibility.
1926

2027
## Example
2128

@@ -38,14 +45,33 @@ $My = @{}
3845
$My.Two # returns 2
3946
```
4047

41-
When a specific scope, option, or visibility is required, put the dictionary (hash table) in that
42-
scope and apply the appropriate option or visibility. For example, if the values should be read-only and
43-
available in the script scope, put the _hash table_ in the script scope and make it read-only.
48+
In this example, you want the values to be read-only and available in the script scope.
49+
Put the hashtable in the script scope and make it read-only.
4450

4551
```powershell
4652
New-Variable -Name My -Value @{} -Option ReadOnly -Scope Script
4753
'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
4854
$Script:My[$_] = $i++
4955
}
5056
$Script:My.Two # returns 2
51-
```
57+
```
58+
59+
## Configuration
60+
61+
```powershell
62+
Rules = @{
63+
PSAvoidExclaimOperator = @{
64+
Enable = $true
65+
}
66+
}
67+
```
68+
69+
### Parameters
70+
71+
- `Enable`: **bool** (Default value is `$false`)
72+
73+
Enable or disable the rule during ScriptAnalyzer invocation.
74+
75+
## References
76+
- [New-Variable](xref:Microsoft.PowerShell.Utility.New-Variable)
77+

docs/Rules/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ The PSScriptAnalyzer contains the following rule definitions.
1414
| [AvoidAssignmentToAutomaticVariable](./AvoidAssignmentToAutomaticVariable.md) | Warning | Yes | |
1515
| [AvoidDefaultValueForMandatoryParameter](./AvoidDefaultValueForMandatoryParameter.md) | Warning | Yes | |
1616
| [AvoidDefaultValueSwitchParameter](./AvoidDefaultValueSwitchParameter.md) | Warning | Yes | |
17-
| [AvoidDynamicallyCreatingVariableNames](./AvoidDynamicallyCreatingVariableNames.md) | Information | Yes | |
17+
| [AvoidDynamicallyCreatingVariableNames](./AvoidDynamicallyCreatingVariableNames.md) | Information | No | Yes |
1818
| [AvoidExclaimOperator](./AvoidExclaimOperator.md) | Warning | No | |
1919
| [AvoidGlobalAliases<sup>1</sup>](./AvoidGlobalAliases.md) | Warning | Yes | |
2020
| [AvoidGlobalFunctions](./AvoidGlobalFunctions.md) | Warning | Yes | |

0 commit comments

Comments
 (0)