Skip to content

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

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 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 2 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 IList<string> _automaticVariablesThatCouldBeProblematicToAssignTo = new List<string>()
Copy link
Contributor

@rjmholt rjmholt Jan 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I feel we should avoid embedding clausal descriptions in variable names if we can -- and also I can't think of an automatic variable that's not problematic to assign to (since, being automatic, they could be reassigned unexpectedly)

Suggested change
private static readonly IList<string> _automaticVariablesThatCouldBeProblematicToAssignTo = new List<string>()
private static readonly IList<string> _automaticVariables = new List<string>()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to differentiate from the _readOnlyAutomaticVariables variable above where PowerShell actually itself throws an error when trying to assign, those variables don't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well one is readonly variables and the other is just automatic variables, no? So _readonlyVariables and _automaticVariables or _writeableAutomaticVariables maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just prefer not to call the variable something that embeds a sentence

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about _writeableAutomaticVariables and _readonlyAutomaticVariables?

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

{
// 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 (_automaticVariablesThatCouldBeProblematicToAssignTo.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 (_automaticVariablesThatCouldBeProblematicToAssignTo.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