Skip to content

Add warnings about read-only automatic variables introduced in PowerShell 6.0 #917

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 9, 2018
Merged

Add warnings about read-only automatic variables introduced in PowerShell 6.0 #917

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eb73985
Add warnings about readonly variables introduced in PowerShell 6.0
bergmeister Mar 3, 2018
178711c
use custom message for variables that only apply to ps 6.0
bergmeister Mar 3, 2018
c55f39c
Remove redundant quotes in IT block because Pester adds them now auto…
bergmeister Mar 3, 2018
5fda73d
Make it throw an Error when PSSA is executed on PSCore
bergmeister Mar 5, 2018
bbc8f3d
address pr comments
bergmeister Mar 23, 2018
abd4f61
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Mar 23, 2018
d81a5a8
exclude PSUseDeclaredVarsMoreThanAssignments rule for tests
bergmeister Mar 23, 2018
a5c023a
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Mar 30, 2018
ad56c52
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions Rules/AvoidAssignmentToAutomaticVariable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,16 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
public class AvoidAssignmentToAutomaticVariable : IScriptRule
{
private static readonly IList<string> _readOnlyAutomaticVariables = new List<string>()
{
// Attempting to assign to any of those read-only variable would result in an error at runtime
"?", "true", "false", "Host", "PSCulture", "Error", "ExecutionContext", "Home", "PID", "PSEdition", "PSHome", "PSUICulture", "PSVersionTable", "ShellId"
};
{
// Attempting to assign to any of those read-only variable would result in an error at runtime
"?", "true", "false", "Host", "PSCulture", "Error", "ExecutionContext", "Home", "PID", "PSEdition", "PSHome", "PSUICulture", "PSVersionTable", "ShellId"
};

private static readonly IList<string> _readOnlyAutomaticVariablesIntroducedInVersion6_0 = new List<string>()
{
// Attempting to assign to any of those read-only variable will result in an error at runtime
"IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows"
};

/// <summary>
/// Checks for assignment to automatic variables.
Expand All @@ -56,6 +62,12 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName),
variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName);
}

if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase))
{
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName),
variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
}
}

IEnumerable<Ast> parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true);
Expand All @@ -64,11 +76,22 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
var variableExpressionAst = parameterAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
var variableName = variableExpressionAst.VariablePath.UserPath;
// also check the parent to exclude parameter attributes such as '[Parameter(Mandatory=$true)]' where the read-only variable $true appears.
if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase) && !(variableExpressionAst.Parent is NamedAttributeArgumentAst))
if (variableExpressionAst.Parent is NamedAttributeArgumentAst)
{
continue;
}

if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase))
{
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName),
variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName);
}

if (_readOnlyAutomaticVariablesIntroducedInVersion6_0.Contains(variableName, StringComparer.OrdinalIgnoreCase))
{
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName),
variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
}
}
}

Expand Down
9 changes: 9 additions & 0 deletions Rules/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Rules/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,4 +1005,7 @@
<data name="AvoidAssignmentToAutomaticVariableName" xml:space="preserve">
<value>AvoidAssignmentToAutomaticVariable</value>
</data>
<data name="AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error" xml:space="preserve">
<value>Starting from PowerShell 6.0, the Variable '{0}' cannot be assigned any more since it is a readonly automatic variable that is built into PowerShell, please use a different name.</value>
</data>
</root>
61 changes: 35 additions & 26 deletions Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,55 +4,64 @@ $ruleName = "PSAvoidAssignmentToAutomaticVariable"
Describe "AvoidAssignmentToAutomaticVariables" {
Context "ReadOnly Variables" {

$readOnlyVariableSeverity = "Error"
$testCases_ReadOnlyVariables = @(
@{ VariableName = '?' }
@{ VariableName = 'Error' }
@{ VariableName = 'ExecutionContext' }
@{ VariableName = 'false' }
@{ VariableName = 'Home' }
@{ VariableName = 'Host' }
@{ VariableName = 'PID' }
@{ VariableName = 'PSCulture' }
@{ VariableName = 'PSEdition' }
@{ VariableName = 'PSHome' }
@{ VariableName = 'PSUICulture' }
@{ VariableName = 'PSVersionTable' }
@{ VariableName = 'ShellId' }
@{ VariableName = 'true' }
@{ VariableName = '?'; ExpectedSeverity = 'Error'; }
@{ VariableName = 'Error' ; ExpectedSeverity = 'Error' }
@{ VariableName = 'ExecutionContext'; ExpectedSeverity = 'Error' }
@{ VariableName = 'false'; ExpectedSeverity = 'Error' }
@{ VariableName = 'Home'; ExpectedSeverity = 'Error' }
@{ VariableName = 'Host'; ExpectedSeverity = 'Error' }
@{ VariableName = 'PID'; ExpectedSeverity = 'Error' }
@{ VariableName = 'PSCulture'; ExpectedSeverity = 'Error' }
@{ VariableName = 'PSEdition'; ExpectedSeverity = 'Error' }
@{ VariableName = 'PSHome'; ExpectedSeverity = 'Error' }
@{ VariableName = 'PSUICulture'; ExpectedSeverity = 'Error' }
@{ VariableName = 'PSVersionTable'; ExpectedSeverity = 'Error' }
@{ VariableName = 'ShellId'; ExpectedSeverity = 'Error' }
@{ VariableName = 'true'; ExpectedSeverity = 'Error' }
# Variables introuced only in PowerShell 6.0 have a Severity of Warning only
@{ VariableName = 'IsCoreCLR'; ExpectedSeverity = 'Warning'; OnlyPresentInCoreClr = $true }
@{ VariableName = 'IsLinux'; ExpectedSeverity = 'Warning'; OnlyPresentInCoreClr = $true }
@{ VariableName = 'IsMacOS'; ExpectedSeverity = 'Warning'; OnlyPresentInCoreClr = $true }
@{ VariableName = 'IsWindows'; ExpectedSeverity = 'Warning'; OnlyPresentInCoreClr = $true }
)

It "Variable '<VariableName>' produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName)
It "Variable <VariableName> produces warning of Severity <ExpectedSeverity>" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName, $ExpectedSeverity)

$warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$${VariableName} = 'foo'" | Where-Object { $_.RuleName -eq $ruleName }
$warnings.Count | Should -Be 1
$warnings.Severity | Should -Be $readOnlyVariableSeverity
$warnings.Severity | Should -Be $ExpectedSeverity
}

It "Using Variable '<VariableName>' as parameter name produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName)
It "Using Variable <VariableName> as parameter name produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName, $ExpectedSeverity)

[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" | Where-Object {$_.RuleName -eq $ruleName }
$warnings.Count | Should -Be 1
$warnings.Severity | Should -Be $readOnlyVariableSeverity
$warnings.Severity | Should -Be $ExpectedSeverity
}

It "Using Variable '<VariableName>' as parameter name in param block produces warning of severity error" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName)
It "Using Variable <VariableName> as parameter name in param block produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName, $ExpectedSeverity)

[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" | Where-Object {$_.RuleName -eq $ruleName }
$warnings.Count | Should -Be 1
$warnings.Severity | Should -Be $readOnlyVariableSeverity
$warnings.Severity | Should -Be $ExpectedSeverity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the above pattern a lot, but it worries me. I would think that the Where-Object clause would obfuscate additional unexpected errors. Shouldn't we know the total amount of errors we expect and not have to filter for only what we expect?

Copy link
Collaborator Author

@bergmeister bergmeister Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no 'right' answer, I think. There is the approach of having either isolated unit tests (with the benefit of lower maintenance) or doing integration testing as part of the test. We are still running all rules against it, so we would probably still get a test failure if one rule unexpectedly threw an exception but we would not have to adapt tests too much if rules changed. I will leave this decision up to you and will therefore remove the Where-Object and replace it with $warnings.RuleName | Should -Be $ruleName'. In one test case I will use -ExcludeRule PSUseDeclaredVarsMoreThanAssignments`

}

It "Does not flag parameter attributes" {
[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'function foo{Param([Parameter(Mandatory=$true)]$param1)}' | Where-Object { $_.RuleName -eq $ruleName }
$warnings.Count | Should -Be 0
}

It "Setting Variable '<VariableName>' throws exception to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName)
It "Setting Variable <VariableName> throws exception to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName, $ExpectedSeverity, $OnlyPresentInCoreClr)

if ($OnlyPresentInCoreClr -and !$IsCoreCLR)
{
continue
}

# Setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string, therefore exclude this case.
# 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.
Expand Down