Skip to content

Commit d432e00

Browse files
bergmeisterJamesWTruher
bergmeister
authored and
JamesWTruher
committed
Warn against assignment to read-only automatic variables (#864)
* add simple prototype to warn against some automatic variables * test against variables used in parameters as well and apply it to read-only variables * add first tests * parameterise tests * test that setting the variable actually throws an error * add documentation * fix test due to the addition of a new rule * Fix false positive in parameter attributes and add tests for it. * improve and simplify testing for exception * exclude PSEdition from test in wmf 4 * Address PR comments * trivial test fix due to merge with upstream branch
1 parent 539e28e commit d432e00

7 files changed

+309
-1
lines changed

‎Engine/Generic/DiagnosticRecordHelper.cs

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
using System;
2+
using System.Globalization;
3+
4+
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
5+
{
6+
public static class DiagnosticRecordHelper
7+
{
8+
public static string FormatError(string format, params object[] args)
9+
{
10+
return String.Format(CultureInfo.CurrentCulture, format, args);
11+
}
12+
}
13+
}

‎RuleDocumentation/AvoidAssignmentToAutomaticVariable.md

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
# AvoidAssignmentToAutomaticVariable
2+
3+
**Severity Level: Warning**
4+
5+
## Description
6+
7+
`PowerShell` exposes some of its built-in variables that are known as automatic variables. Many of them are read-only and PowerShell would throw an error when trying to assign an value on those. Other automatic variables should only be assigned to in certain special cases to achieve a certain effect as a special technique.
8+
9+
To understand more about automatic variables, see ```Get-Help about_Automatic_Variables```.
10+
11+
## How
12+
13+
Use variable names in functions or their parameters that do not conflict with automatic variables.
14+
15+
## Example
16+
17+
### Wrong
18+
19+
The variable `$Error` is an automatic variables that exists in the global scope and should therefore never be used as a variable or parameter name.
20+
21+
``` PowerShell
22+
function foo($Error){ }
23+
```
24+
25+
``` PowerShell
26+
function Get-CustomErrorMessage($ErrorMessage){ $Error = "Error occurred: $ErrorMessage" }
27+
```
28+
29+
### Correct
30+
31+
``` PowerShell
32+
function Get-CustomErrorMessage($ErrorMessage){ $FinalErrorMessage = "Error occurred: $ErrorMessage" }
33+
```

‎Rules/AvoidAssignmentToAutomaticVariable.cs

+128
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
//
2+
// Copyright (c) Microsoft Corporation.
3+
//
4+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
5+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
6+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
7+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
8+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
9+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
10+
// THE SOFTWARE.
11+
//
12+
13+
using System;
14+
using System.Linq;
15+
using System.Collections.Generic;
16+
using System.Management.Automation.Language;
17+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
18+
#if !CORECLR
19+
using System.ComponentModel.Composition;
20+
#endif
21+
using System.Globalization;
22+
23+
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
24+
{
25+
/// <summary>
26+
/// AvoidAssignmentToAutomaticVariable:
27+
/// </summary>
28+
#if !CORECLR
29+
[Export(typeof(IScriptRule))]
30+
#endif
31+
public class AvoidAssignmentToAutomaticVariable : IScriptRule
32+
{
33+
private static readonly IList<string> _readOnlyAutomaticVariables = new List<string>()
34+
{
35+
// Attempting to assign to any of those read-only variable would result in an error at runtime
36+
"?", "true", "false", "Host", "PSCulture", "Error", "ExecutionContext", "Home", "PID", "PSEdition", "PSHome", "PSUICulture", "PSVersionTable", "ShellId"
37+
};
38+
39+
/// <summary>
40+
/// Checks for assignment to automatic variables.
41+
/// <param name="ast">The script's ast</param>
42+
/// <param name="fileName">The script's file name</param>
43+
/// <returns>The diagnostic results of this rule</returns>
44+
/// </summary>
45+
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
46+
{
47+
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
48+
49+
IEnumerable<Ast> assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true);
50+
foreach (var assignmentStatementAst in assignmentStatementAsts)
51+
{
52+
var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
53+
var variableName = variableExpressionAst.VariablePath.UserPath;
54+
if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase))
55+
{
56+
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName),
57+
variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName);
58+
}
59+
}
60+
61+
IEnumerable<Ast> parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true);
62+
foreach (ParameterAst parameterAst in parameterAsts)
63+
{
64+
var variableExpressionAst = parameterAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
65+
var variableName = variableExpressionAst.VariablePath.UserPath;
66+
// also check the parent to exclude parameter attributes such as '[Parameter(Mandatory=$true)]' where the read-only variable $true appears.
67+
if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase) && !(variableExpressionAst.Parent is NamedAttributeArgumentAst))
68+
{
69+
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName),
70+
variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName);
71+
}
72+
}
73+
}
74+
75+
/// <summary>
76+
/// GetName: Retrieves the name of this rule.
77+
/// </summary>
78+
/// <returns>The name of this rule</returns>
79+
public string GetName()
80+
{
81+
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidAssignmentToAutomaticVariableName);
82+
}
83+
84+
/// <summary>
85+
/// GetCommonName: Retrieves the common name of this rule.
86+
/// </summary>
87+
/// <returns>The common name of this rule</returns>
88+
public string GetCommonName()
89+
{
90+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableCommonName);
91+
}
92+
93+
/// <summary>
94+
/// GetDescription: Retrieves the description of this rule.
95+
/// </summary>
96+
/// <returns>The description of this rule</returns>
97+
public string GetDescription()
98+
{
99+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableDescription);
100+
}
101+
102+
/// <summary>
103+
/// GetSourceType: Retrieves the type of the rule: builtin, managed or module.
104+
/// </summary>
105+
public SourceType GetSourceType()
106+
{
107+
return SourceType.Builtin;
108+
}
109+
110+
/// <summary>
111+
/// GetSeverity: Retrieves the severity of the rule: error, warning of information.
112+
/// </summary>
113+
/// <returns></returns>
114+
public RuleSeverity GetSeverity()
115+
{
116+
return RuleSeverity.Warning;
117+
}
118+
119+
/// <summary>
120+
/// GetSourceName: Retrieves the module/assembly name the rule is from.
121+
/// </summary>
122+
public string GetSourceName()
123+
{
124+
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
125+
}
126+
}
127+
128+
}

‎Rules/Strings.Designer.cs

+45
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Rules/Strings.resx

+15
Original file line numberDiff line numberDiff line change
@@ -990,4 +990,19 @@
990990
<data name="PossibleIncorrectUsageOfAssignmentOperatorName" xml:space="preserve">
991991
<value>PossibleIncorrectUsageOfAssignmentOperator</value>
992992
</data>
993+
<data name="AvoidAssignmentToReadOnlyAutomaticVariable" xml:space="preserve">
994+
<value>Use a different variable name</value>
995+
</data>
996+
<data name="AvoidAssignmentToReadOnlyAutomaticVariableCommonName" xml:space="preserve">
997+
<value>Changing automtic variables might have undesired side effects</value>
998+
</data>
999+
<data name="AvoidAssignmentToReadOnlyAutomaticVariableDescription" xml:space="preserve">
1000+
<value>This automatic variables is built into PowerShell and readonly.</value>
1001+
</data>
1002+
<data name="AvoidAssignmentToReadOnlyAutomaticVariableError" xml:space="preserve">
1003+
<value>The Variable '{0}' cannot be assigned since it is a readonly automatic variable that is built into PowerShell, please use a different name.</value>
1004+
</data>
1005+
<data name="AvoidAssignmentToAutomaticVariableName" xml:space="preserve">
1006+
<value>AvoidAssignmentToAutomaticVariable</value>
1007+
</data>
9931008
</root>

‎Tests/Engine/GetScriptAnalyzerRule.tests.ps1

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ Describe "Test Name parameters" {
6161

6262
It "get Rules with no parameters supplied" {
6363
$defaultRules = Get-ScriptAnalyzerRule
64-
$expectedNumRules = 53
64+
$expectedNumRules = 54
6565
if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4))
6666
{
6767
# for PSv3 PSAvoidGlobalAliases is not shipped because

‎Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1

+74
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
Import-Module PSScriptAnalyzer
2+
$ruleName = "PSAvoidAssignmentToAutomaticVariable"
3+
4+
Describe "AvoidAssignmentToAutomaticVariables" {
5+
Context "ReadOnly Variables" {
6+
7+
$readOnlyVariableSeverity = "Error"
8+
$testCases_ReadOnlyVariables = @(
9+
@{ VariableName = '?' }
10+
@{ VariableName = 'Error' }
11+
@{ VariableName = 'ExecutionContext' }
12+
@{ VariableName = 'false' }
13+
@{ VariableName = 'Home' }
14+
@{ VariableName = 'Host' }
15+
@{ VariableName = 'PID' }
16+
@{ VariableName = 'PSCulture' }
17+
@{ VariableName = 'PSEdition' }
18+
@{ VariableName = 'PSHome' }
19+
@{ VariableName = 'PSUICulture' }
20+
@{ VariableName = 'PSVersionTable' }
21+
@{ VariableName = 'ShellId' }
22+
@{ VariableName = 'true' }
23+
)
24+
25+
It "Variable '<VariableName>' produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
26+
param ($VariableName)
27+
28+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$${VariableName} = 'foo'" | Where-Object { $_.RuleName -eq $ruleName }
29+
$warnings.Count | Should Be 1
30+
$warnings.Severity | Should Be $readOnlyVariableSeverity
31+
}
32+
33+
It "Using Variable '<VariableName>' as parameter name produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
34+
param ($VariableName)
35+
36+
[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" | Where-Object {$_.RuleName -eq $ruleName }
37+
$warnings.Count | Should Be 1
38+
$warnings.Severity | Should Be $readOnlyVariableSeverity
39+
}
40+
41+
It "Using Variable '<VariableName>' as parameter name in param block produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
42+
param ($VariableName)
43+
44+
[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" | Where-Object {$_.RuleName -eq $ruleName }
45+
$warnings.Count | Should Be 1
46+
$warnings.Severity | Should Be $readOnlyVariableSeverity
47+
}
48+
49+
It "Does not flag parameter attributes" {
50+
[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'function foo{Param([Parameter(Mandatory=$true)]$param1)}' | Where-Object { $_.RuleName -eq $ruleName }
51+
$warnings.Count | Should Be 0
52+
}
53+
54+
It "Setting Variable '<VariableName>' throws exception to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables {
55+
param ($VariableName)
56+
57+
# Setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string, therefore exclude this case.
58+
# For the library test in WMF 4, assigning a value $PSEdition does not seem to throw an error, therefore this special case is excluded as well.
59+
if ($VariableName -ne 'Error' -and ($VariableName -ne 'PSEdition' -and $PSVersionTable.PSVersion.Major -ne 4))
60+
{
61+
try
62+
{
63+
Set-Variable -Name $VariableName -Value 'foo' -ErrorVariable errorVariable -ErrorAction Stop
64+
throw "Expected exception did not occur when assigning value to read-only variable '$VariableName'"
65+
}
66+
catch
67+
{
68+
$_.FullyQualifiedErrorId | Should Be 'VariableNotWritable,Microsoft.PowerShell.Commands.SetVariableCommand'
69+
}
70+
}
71+
}
72+
73+
}
74+
}

0 commit comments

Comments
 (0)