Skip to content
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

Add more automatic variables to PSAvoidAssignmentToAutomaticVariables that are not read-only but should still not be assigned to in most cases #1394

Merged
merged 4 commits into from
Jan 16, 2020
Merged

Add more automatic variables to PSAvoidAssignmentToAutomaticVariables that are not read-only but should still not be assigned to in most cases #1394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72e0dac
Add more automatic variables to PSAvoidAssignmentToAutomaticVariables…
Jan 13, 2020
af5734d
Merge branch 'master' of https://github.com/powershell/psscriptanalyz…
Jan 13, 2020
2a91c87
Update Rules/AvoidAssignmentToAutomaticVariable.cs
bergmeister Jan 14, 2020
09ec140
rename variable to _writableAutomaticVariables
Jan 15, 2020
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
23 changes: 23 additions & 0 deletions Rules/AvoidAssignmentToAutomaticVariable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule
"IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows"
};

private static readonly IReadOnlyList<string> _writableAutomaticVariables = new List<string>()
{
// Attempting to assign to any of those could cause issues, only in some special cases could assignment be by design
"_", "AllNodes", "Args", "ConsoleFilename", "Event", "EventArgs", "EventSubscriber", "ForEach", "Input", "Matches", "MyInvocation",
"NestedPromptLevel", "Profile", "PSBoundParameters", "PsCmdlet", "PSCommandPath", "PSDebugContext",
"PSItem", "PSScriptRoot", "PSSenderInfo", "Pwd", "PSCommandPath", "ReportErrorShowExceptionClass",
"ReportErrorShowInnerException", "ReportErrorShowSource", "ReportErrorShowStackTrace", "Sender",
"StackTrace", "This"
};

/// <summary>
/// Checks for assignment to automatic variables.
/// <param name="ast">The script's ast</param>
Expand Down Expand Up @@ -61,6 +71,12 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName),
variableExpressionAst.Extent, GetName(), severity, fileName);
}

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

IEnumerable<Ast> parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true);
Expand All @@ -79,12 +95,19 @@ 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))
{
var severity = IsPowerShellVersion6OrGreater() ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning;
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPowerShell6_0Error, variableName),
variableExpressionAst.Extent, GetName(), severity, fileName);
}

if (_writableAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase))
{
yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToWritableAutomaticVariableError, 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 @@ -1104,6 +1104,9 @@
<data name="UseProcessBlockForPipelineCommandName" xml:space="preserve">
<value>UseProcessBlockForPipelineCommand</value>
</data>
<data name="AvoidAssignmentToWritableAutomaticVariableError" xml:space="preserve">
<value>The Variable '{0}' is an automatic variable that is built into PowerShell, assigning to it might have undesired side effects. If assignment is not by design, please use a different name.</value>
</data>
<data name="UseConsistentWhitespaceErrorSpaceBetweenParameter" xml:space="preserve">
<value>Use only 1 whitespace between parameter names or values.</value>
</data>
Expand Down
69 changes: 47 additions & 22 deletions Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,54 @@ Describe "AvoidAssignmentToAutomaticVariables" {
$excpectedSeverityForAutomaticVariablesInPowerShell6 = 'Error'
}

$testCases_ReadOnlyVariables = @(
@{ 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
$testCases_AutomaticVariables = @(
@{ VariableName = '?'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'Error' ; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'ExecutionContext'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'false'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'Home'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'Host'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'PID'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'PSCulture'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'PSEdition'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'PSHome'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'PSUICulture'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'PSVersionTable'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'ShellId'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
@{ VariableName = 'true'; ExpectedSeverity = 'Error'; IsReadOnly = $true }
# Variables introduced only in PowerShell 6+ have a Severity of Warning only
@{ VariableName = 'IsCoreCLR'; ExpectedSeverity = $excpectedSeverityForAutomaticVariablesInPowerShell6; OnlyPresentInCoreClr = $true }
@{ VariableName = 'IsLinux'; ExpectedSeverity = $excpectedSeverityForAutomaticVariablesInPowerShell6; OnlyPresentInCoreClr = $true }
@{ VariableName = 'IsMacOS'; ExpectedSeverity = $excpectedSeverityForAutomaticVariablesInPowerShell6; OnlyPresentInCoreClr = $true }
@{ VariableName = 'IsWindows'; ExpectedSeverity = $excpectedSeverityForAutomaticVariablesInPowerShell6; OnlyPresentInCoreClr = $true }
@{ VariableName = '_'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'AllNodes'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'Args'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'ConsoleFilename'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'Event'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'EventArgs'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'EventSubscriber'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'ForEach'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'Input'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'Matches'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'MyInvocation'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'NestedPromptLevel'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'Profile'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'PSBoundParameters'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'PsCmdlet'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'PSCommandPath'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'ReportErrorShowExceptionClass'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'ReportErrorShowInnerException'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'ReportErrorShowSource'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'ReportErrorShowStackTrace'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'Sender'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'StackTrace'; ExpectedSeverity = 'Warning' }
@{ VariableName = 'This'; ExpectedSeverity = 'Warning' }
)

It "Variable <VariableName> produces warning of Severity <ExpectedSeverity>" -TestCases $testCases_ReadOnlyVariables {
$testCases_ReadOnlyAutomaticVariables = $testCases_AutomaticVariables | Where-Object { $_.IsReadonly }

It "Variable <VariableName> produces warning of Severity <ExpectedSeverity>" -TestCases $testCases_AutomaticVariables {
param ($VariableName, $ExpectedSeverity)

$warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$${VariableName} = 'foo'" -ExcludeRule PSUseDeclaredVarsMoreThanAssignments
Expand All @@ -40,7 +65,7 @@ Describe "AvoidAssignmentToAutomaticVariables" {
$warnings.RuleName | Should -Be $ruleName
}

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

[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}"
Expand All @@ -49,7 +74,7 @@ Describe "AvoidAssignmentToAutomaticVariables" {
$warnings.RuleName | Should -Be $ruleName
}

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

[System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}"
Expand Down Expand Up @@ -77,16 +102,16 @@ Describe "AvoidAssignmentToAutomaticVariables" {
$warnings.Count | Should -Be 0
}

It "Setting Variable <VariableName> throws exception in applicable PowerShell version to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables {
param ($VariableName, $ExpectedSeverity, $OnlyPresentInCoreClr)
It "Setting Variable <VariableName> throws exception in applicable PowerShell version to verify the variables is read-only" -TestCases $testCases_ReadOnlyAutomaticVariables {
param ($VariableName, $ExpectedSeverity, $OnlyPresentInCoreClr, [bool] $IsReadOnly)

if ($OnlyPresentInCoreClr -and !$IsCoreCLR)
{
# In this special case we expect it to not throw
Set-Variable -Name $VariableName -Value 'foo'
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.
if ($VariableName -ne 'Error' -and ($VariableName -ne 'PSEdition' -and $PSVersionTable.PSVersion.Major -ne 4))
Expand Down