From 72e0dac3837b07625f162813ea4c8ccaae986422 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 13 Jan 2020 08:31:20 +0000 Subject: [PATCH 1/3] Add more automatic variables to PSAvoidAssignmentToAutomaticVariables that are not read-only but should still not be assigned to in most cases --- Rules/AvoidAssignmentToAutomaticVariable.cs | 23 +++++ Rules/Strings.Designer.cs | 89 ++++++++++--------- Rules/Strings.resx | 5 +- ...oidAssignmentToAutomaticVariable.tests.ps1 | 69 +++++++++----- 4 files changed, 119 insertions(+), 67 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index b6b427440..6bf618a56 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -33,6 +33,16 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule "IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows" }; + private static readonly IList _automaticVariablesThatCouldBeProblematicToAssignTo = new List() + { + // 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" + }; + /// /// Checks for assignment to automatic variables. /// The script's ast @@ -61,6 +71,12 @@ public IEnumerable 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 parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true); @@ -79,12 +95,19 @@ public IEnumerable 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); + } } } diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 54cba38eb..1134d2f69 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -150,6 +150,15 @@ internal static string AvoidAssignmentToReadOnlyAutomaticVariableIntroducedInPow } } + /// + /// Looks up a localized string similar to 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.. + /// + internal static string AvoidAssignmentToWritableAutomaticVariableError { + get { + return ResourceManager.GetString("AvoidAssignmentToWritableAutomaticVariableError", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using ComputerName Hardcoded. /// @@ -492,6 +501,42 @@ internal static string AvoidNullOrEmptyHelpMessageAttributeName { } } + /// + /// Looks up a localized string similar to Avoid overwriting built in cmdlets. + /// + internal static string AvoidOverwritingBuiltInCmdletsCommonName { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Do not overwrite the definition of a cmdlet that is included with PowerShell. + /// + internal static string AvoidOverwritingBuiltInCmdletsDescription { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to '{0}' is a cmdlet that is included with PowerShell (version {1}) whose definition should not be overridden. + /// + internal static string AvoidOverwritingBuiltInCmdletsError { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidOverwritingBuiltInCmdlets. + /// + internal static string AvoidOverwritingBuiltInCmdletsName { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using ShouldContinue Without Boolean Force Parameter. /// @@ -2084,50 +2129,6 @@ internal static string UseCompatibleCmdletsName { return ResourceManager.GetString("UseCompatibleCmdletsName", resourceCulture); } } - - /// - /// Looks up a localized string similar to Avoid overwriting built in cmdlets. - /// - internal static string AvoidOverwritingBuiltInCmdletsCommonName - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsCommonName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Avoid overwriting built in cmdlets. - /// - internal static string AvoidOverwritingBuiltInCmdletsDescription - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsDescription", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to '{0}' is a cmdlet that is included with PowerShell whose definition should not be overridden. - /// - internal static string AvoidOverwritingBuiltInCmdletsError - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsError", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to AvoidOverwritingBuiltInCmdlets. - /// - internal static string AvoidOverwritingBuiltInCmdletsName - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsName", resourceCulture); - } - } /// /// Looks up a localized string similar to The command '{0}' is not available by default in PowerShell version '{1}' on platform '{2}'. diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 50362080f..d8ca0a3a9 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1104,4 +1104,7 @@ UseProcessBlockForPipelineCommand - + + 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. + + \ No newline at end of file diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index e2a363da5..caf207b8c 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -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 produces warning of Severity " -TestCases $testCases_ReadOnlyVariables { + $testCases_ReadOnlyAutomaticVariables = $testCases_AutomaticVariables | Where-Object { $_.IsReadonly } + + It "Variable produces warning of Severity " -TestCases $testCases_AutomaticVariables { param ($VariableName, $ExpectedSeverity) $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$${VariableName} = 'foo'" -ExcludeRule PSUseDeclaredVarsMoreThanAssignments @@ -40,7 +65,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { $warnings.RuleName | Should -Be $ruleName } - It "Using Variable as parameter name produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables { + It "Using Variable as parameter name produces warning of Severity error" -TestCases $testCases_AutomaticVariables { param ($VariableName, $ExpectedSeverity) [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" @@ -49,7 +74,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { $warnings.RuleName | Should -Be $ruleName } - It "Using Variable as parameter name in param block produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables { + It "Using Variable 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){}" @@ -77,8 +102,8 @@ Describe "AvoidAssignmentToAutomaticVariables" { $warnings.Count | Should -Be 0 } - It "Setting Variable throws exception in applicable PowerShell version to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { - param ($VariableName, $ExpectedSeverity, $OnlyPresentInCoreClr) + It "Setting Variable 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) { @@ -86,7 +111,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { 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)) From 2a91c87e5b97109a6355efc27939d7f853c9bebd Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister [MVP]" Date: Tue, 14 Jan 2020 09:09:12 +0000 Subject: [PATCH 2/3] Update Rules/AvoidAssignmentToAutomaticVariable.cs Co-Authored-By: Robert Holt --- Rules/AvoidAssignmentToAutomaticVariable.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index 6bf618a56..aae057b05 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -33,7 +33,7 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule "IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows" }; - private static readonly IList _automaticVariablesThatCouldBeProblematicToAssignTo = new List() + private static readonly IReadOnlyList _automaticVariablesThatCouldBeProblematicToAssignTo = new List() { // 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", From 09ec140fcff0e0763220eeec31d3c9c0f4f63442 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Wed, 15 Jan 2020 08:58:31 +0000 Subject: [PATCH 3/3] rename variable to _writableAutomaticVariables --- Rules/AvoidAssignmentToAutomaticVariable.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index aae057b05..1f2a784dd 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -33,7 +33,7 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule "IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows" }; - private static readonly IReadOnlyList _automaticVariablesThatCouldBeProblematicToAssignTo = new List() + private static readonly IReadOnlyList _writableAutomaticVariables = new List() { // 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", @@ -72,7 +72,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) variableExpressionAst.Extent, GetName(), severity, fileName); } - if (_automaticVariablesThatCouldBeProblematicToAssignTo.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + if (_writableAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) { yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToWritableAutomaticVariableError, variableName), variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); @@ -103,7 +103,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) variableExpressionAst.Extent, GetName(), severity, fileName); } - if (_automaticVariablesThatCouldBeProblematicToAssignTo.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + if (_writableAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) { yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToWritableAutomaticVariableError, variableName), variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);