From 5c0cb374608669de50fd3fea827720546451dc62 Mon Sep 17 00:00:00 2001 From: Chris Bergmeister Date: Tue, 30 Jan 2018 23:06:48 +0000 Subject: [PATCH 01/12] add simple prototype to warn against some automatic variables --- Engine/Generic/DiagnosticRecordHelper.cs | 13 ++ Rules/AvoidAssignmentToAutomaticVariable.cs | 128 ++++++++++++++++++++ Rules/Strings.Designer.cs | 53 +++++++- Rules/Strings.resx | 72 ++++++----- 4 files changed, 233 insertions(+), 33 deletions(-) create mode 100644 Engine/Generic/DiagnosticRecordHelper.cs create mode 100644 Rules/AvoidAssignmentToAutomaticVariable.cs diff --git a/Engine/Generic/DiagnosticRecordHelper.cs b/Engine/Generic/DiagnosticRecordHelper.cs new file mode 100644 index 000000000..9302ec339 --- /dev/null +++ b/Engine/Generic/DiagnosticRecordHelper.cs @@ -0,0 +1,13 @@ +using System; +using System.Globalization; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic +{ + public static class DiagnosticRecordHelper + { + public static string FormatError(string format, params object[] args) + { + return String.Format(CultureInfo.CurrentCulture, format, args); + } + } +} diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs new file mode 100644 index 000000000..d2f05593b --- /dev/null +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -0,0 +1,128 @@ +// +// Copyright (c) Microsoft Corporation. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// + +using System; +using System.Linq; +using System.Collections.Generic; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using System.Management.Automation; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// AvoidAssignmentToAutomaticVariable: + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class AvoidAssignmentToAutomaticVariable : IScriptRule + { + /// + /// AnalyzeScript: Analyzes the ast to check that $null is on the left side of any equality comparisons. + /// The script's ast + /// The script's file name + /// The diagnostic results of this rule + /// + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + IEnumerable assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); + var automaticVariables = new List() + { + // TODO: fill in list from https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/SpecialVariables.cs + // and consider making the class above public + "_", "PSItem", "$", "?", "^", "ARGS", "CONSOLEFILENAME", "ERROR", "EVENT", + "EVENTARGS", "EVENTSUBSCRIBER", "EXECUTIONCONTEXT", "FALSE", + }; + + foreach (var assignmentStatementAst in assignmentStatementAsts) + { + var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; + var variableName = variableExpressionAst.VariablePath.UserPath; + if (automaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + { + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToAutomaticVariableError, variableName), + variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + } + } + +#if PSV3 + + IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true); + +#else + + IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true).Union(ast.FindAll(item => item is FunctionMemberAst, true)); + +#endif + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidAssignmentToAutomaticVariableName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToAutomaticVariableCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToAutomaticVariableDescription); + } + + /// + /// GetSourceType: Retrieves the type of the rule: builtin, managed or module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// GetSeverity: Retrieves the severity of the rule: error, warning of information. + /// + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// GetSourceName: Retrieves the module/assembly name the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } + +} diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 1d6f12a04..872955812 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -11,8 +11,8 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { using System; using System.Reflection; - - + + /// /// A strongly-typed resource class, for looking up localized strings, etc. /// @@ -97,6 +97,51 @@ internal static string AlignAssignmentStatementName { } } + /// + /// Looks up a localized string similar to Use a different variable name. + /// + internal static string AvoidAssignmentToAutomaticVariable { + get { + return ResourceManager.GetString("AvoidAssignmentToAutomaticVariable", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Changing automtic variables might have undesired side effects. + /// + internal static string AvoidAssignmentToAutomaticVariableCommonName { + get { + return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Automatic variables are built into PowerShell and are ony meant to be consumed but not changed.. + /// + internal static string AvoidAssignmentToAutomaticVariableDescription { + get { + return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Variable {0} should use a different name since it is an automatic variable.. + /// + internal static string AvoidAssignmentToAutomaticVariableError { + get { + return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidAssignmentToAutomaticVariable. + /// + internal static string AvoidAssignmentToAutomaticVariableName { + get { + return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using ComputerName Hardcoded. /// @@ -2366,7 +2411,7 @@ internal static string UseShouldProcessForStateChangingFunctionsDescrption { } /// - /// Looks up a localized string similar to Function '{0}' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.. + /// Looks up a localized string similar to Function '{0}' has verb that could change system state. Therefore, the function has to support 'ShouldProcess'.. /// internal static string UseShouldProcessForStateChangingFunctionsError { get { @@ -2636,7 +2681,7 @@ internal static string UseVerboseMessageInDSCResourceDescription { } /// - /// Looks up a localized string similar to There is no call to Write-Verbose in DSC function '{0}'. If you are using Write-Verbose in a helper function, suppress this rule application.. + /// Looks up a localized string similar to There is no call to Write-Verbose in DSC function '{0}'. If you are using Write-Verbose in a helper function, suppress this rule application.. /// internal static string UseVerboseMessageInDSCResourceErrorFunction { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index aa12ff43f..67fff8a19 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1,17 +1,17 @@  - @@ -957,7 +957,6 @@ Use space after a semicolon. - UseSupportsShouldProcess @@ -982,4 +981,19 @@ Assignment statements are not aligned - + + Use a different variable name + + + Changing automtic variables might have undesired side effects + + + Automatic variables are built into PowerShell and are ony meant to be consumed but not changed. + + + Variable {0} should use a different name since it is an automatic variable. + + + AvoidAssignmentToAutomaticVariable + + \ No newline at end of file From 94ee83b213cb3937848c3a975e17070a46f63611 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 16:28:51 +0000 Subject: [PATCH 02/12] test against variables used in parameters as well and apply it to read-only variables --- Rules/AvoidAssignmentToAutomaticVariable.cs | 42 +++++++++------------ Rules/Strings.Designer.cs | 30 +++++++-------- Rules/Strings.resx | 12 +++--- 3 files changed, 38 insertions(+), 46 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index d2f05593b..b7d98f72b 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -15,7 +15,6 @@ using System.Collections.Generic; using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; -using System.Management.Automation; #if !CORECLR using System.ComponentModel.Composition; #endif @@ -31,8 +30,14 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class AvoidAssignmentToAutomaticVariable : IScriptRule { + private static readonly IList _readOnlyAutomaticVariables = new List() + { + // 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" + }; + /// - /// AnalyzeScript: Analyzes the ast to check that $null is on the left side of any equality comparisons. + /// Checks for assignment to automatic variables. /// The script's ast /// The script's file name /// The diagnostic results of this rule @@ -41,35 +46,22 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + IEnumerable assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); - var automaticVariables = new List() - { - // TODO: fill in list from https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/SpecialVariables.cs - // and consider making the class above public - "_", "PSItem", "$", "?", "^", "ARGS", "CONSOLEFILENAME", "ERROR", "EVENT", - "EVENTARGS", "EVENTSUBSCRIBER", "EXECUTIONCONTEXT", "FALSE", - }; + IEnumerable parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true); + - foreach (var assignmentStatementAst in assignmentStatementAsts) + var assignmentStatementAndParamterAsts = assignmentStatementAsts.Concat(parameterAsts); + foreach (var assignmentStatementAst in assignmentStatementAndParamterAsts) { var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; var variableName = variableExpressionAst.VariablePath.UserPath; - if (automaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) { - yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToAutomaticVariableError, variableName), - variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), + variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); } } - -#if PSV3 - - IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true); - -#else - - IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true).Union(ast.FindAll(item => item is FunctionMemberAst, true)); - -#endif } /// @@ -87,7 +79,7 @@ public string GetName() /// The common name of this rule public string GetCommonName() { - return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToAutomaticVariableCommonName); + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableCommonName); } /// @@ -96,7 +88,7 @@ public string GetCommonName() /// The description of this rule public string GetDescription() { - return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToAutomaticVariableDescription); + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableDescription); } /// diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 872955812..aaedddce4 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -98,47 +98,47 @@ internal static string AlignAssignmentStatementName { } /// - /// Looks up a localized string similar to Use a different variable name. + /// Looks up a localized string similar to AvoidAssignmentToAutomaticVariable. /// - internal static string AvoidAssignmentToAutomaticVariable { + internal static string AvoidAssignmentToAutomaticVariableName { get { - return ResourceManager.GetString("AvoidAssignmentToAutomaticVariable", resourceCulture); + return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableName", resourceCulture); } } /// - /// Looks up a localized string similar to Changing automtic variables might have undesired side effects. + /// Looks up a localized string similar to Use a different variable name. /// - internal static string AvoidAssignmentToAutomaticVariableCommonName { + internal static string AvoidAssignmentToReadOnlyAutomaticVariable { get { - return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableCommonName", resourceCulture); + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariable", resourceCulture); } } /// - /// Looks up a localized string similar to Automatic variables are built into PowerShell and are ony meant to be consumed but not changed.. + /// Looks up a localized string similar to Changing automtic variables might have undesired side effects. /// - internal static string AvoidAssignmentToAutomaticVariableDescription { + internal static string AvoidAssignmentToReadOnlyAutomaticVariableCommonName { get { - return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableDescription", resourceCulture); + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariableCommonName", resourceCulture); } } /// - /// Looks up a localized string similar to Variable {0} should use a different name since it is an automatic variable.. + /// Looks up a localized string similar to This automatic variables is built into PowerShell and readonly.. /// - internal static string AvoidAssignmentToAutomaticVariableError { + internal static string AvoidAssignmentToReadOnlyAutomaticVariableDescription { get { - return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableError", resourceCulture); + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariableDescription", resourceCulture); } } /// - /// Looks up a localized string similar to AvoidAssignmentToAutomaticVariable. + /// Looks up a localized string similar to Variable {0} should use a different name since it is a readonly automatic variable.. /// - internal static string AvoidAssignmentToAutomaticVariableName { + internal static string AvoidAssignmentToReadOnlyAutomaticVariableError { get { - return ResourceManager.GetString("AvoidAssignmentToAutomaticVariableName", resourceCulture); + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariableError", resourceCulture); } } diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 67fff8a19..4aa651684 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -981,17 +981,17 @@ Assignment statements are not aligned - + Use a different variable name - + Changing automtic variables might have undesired side effects - - Automatic variables are built into PowerShell and are ony meant to be consumed but not changed. + + This automatic variables is built into PowerShell and readonly. - - Variable {0} should use a different name since it is an automatic variable. + + Variable {0} should use a different name since it is a readonly automatic variable. AvoidAssignmentToAutomaticVariable From c4c8c6e3576542aa2c536fc7a7bb3ab2d04168c9 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 17:27:30 +0000 Subject: [PATCH 03/12] add first tests --- .../AvoidAssignmentToAutomaticVariable.tests.ps1 | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 new file mode 100644 index 000000000..b28f6c129 --- /dev/null +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -0,0 +1,16 @@ +Import-Module PSScriptAnalyzer +$ruleName = "PSAvoidAssignmentToAutomaticVariable" + +Describe "AvoidAssignmentToAutomaticVariables" { + Context "ReadOnly Variables" { + It "'?' Variable" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '$? = Get-Alias' | Where-Object { $_.RuleName -eq $ruleName } + $warnings.Count | Should Be 1 + } + + It "True Variable" { + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '$true = Get-Alias' | Where-Object { $_.RuleName -eq $ruleName } + $warnings.Count | Should Be 1 + } + } +} \ No newline at end of file From 456902dfe0003658886ed3c2332718463fb21dee Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 17:38:36 +0000 Subject: [PATCH 04/12] parameterise tests --- Rules/AvoidAssignmentToAutomaticVariable.cs | 2 +- ...oidAssignmentToAutomaticVariable.tests.ps1 | 28 +++++++++++++------ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index b7d98f72b..066990fb2 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -33,7 +33,7 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule private static readonly IList _readOnlyAutomaticVariables = new List() { // 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" + "?", "true", "false", "Host", "PSCulture", "Error", "ExecutionContext", "Home", "PID", "PSEdition", "PSHome", "PSUICulture", "PSVersionTable", "ShellId" }; /// diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index b28f6c129..8bfcb2f67 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -2,15 +2,27 @@ $ruleName = "PSAvoidAssignmentToAutomaticVariable" Describe "AvoidAssignmentToAutomaticVariables" { - Context "ReadOnly Variables" { - It "'?' Variable" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '$? = Get-Alias' | Where-Object { $_.RuleName -eq $ruleName } - $warnings.Count | Should Be 1 - } - - It "True Variable" { - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition '$true = Get-Alias' | Where-Object { $_.RuleName -eq $ruleName } + Context "ReadOnly Variables produce warning of severity error" { + It "Variable ''" -TestCases @( + @{ 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' } + ) { + param ($VariableName) + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$$($VariableName) = Get-Alias" | Where-Object { $_.RuleName -eq $ruleName } $warnings.Count | Should Be 1 + $warnings.Severity | Should Be "Error" } } } \ No newline at end of file From 09e0692e78a0523f8ca892844210750a6bfdc7ad Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 18:13:32 +0000 Subject: [PATCH 05/12] test that setting the variable actually throws an error --- .../AvoidAssignmentToAutomaticVariable.tests.ps1 | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index 8bfcb2f67..80d9c43ca 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -3,7 +3,7 @@ $ruleName = "PSAvoidAssignmentToAutomaticVariable" Describe "AvoidAssignmentToAutomaticVariables" { Context "ReadOnly Variables produce warning of severity error" { - It "Variable ''" -TestCases @( + It "Variable '' produce warning of severity error and throw SessionStateUnauthorizedAccessException" -TestCases @( @{ VariableName = '?' } @{ VariableName = 'Error' } @{ VariableName = 'ExecutionContext' } @@ -20,9 +20,18 @@ Describe "AvoidAssignmentToAutomaticVariables" { @{ VariableName = 'true' } ) { param ($VariableName) - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$$($VariableName) = Get-Alias" | Where-Object { $_.RuleName -eq $ruleName } + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$$($VariableName) = 'foo'" | Where-Object { $_.RuleName -eq $ruleName } $warnings.Count | Should Be 1 $warnings.Severity | Should Be "Error" + + Set-Variable -Name $VariableName -Value 'foo' -ErrorVariable errorVariable -ErrorAction SilentlyContinue + $ErrorVariable | Should Not Be Null + $ErrorVariable.Exception | Should Not Be Null + if ($VariableName -ne 'Error') # setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string + { + $ErrorVariable.Exception.GetType() | Should Not Be Null + $ErrorVariable.Exception.GetType().FullName | Should Be 'System.Management.Automation.SessionStateUnauthorizedAccessException' + } } } } \ No newline at end of file From dc710a0549afb4e6e407d9adb9f03f157054587b Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 18:27:48 +0000 Subject: [PATCH 06/12] add documentation --- .../AvoidAssignmentToAutomaticVariable.md | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 RuleDocumentation/AvoidAssignmentToAutomaticVariable.md diff --git a/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md b/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md new file mode 100644 index 000000000..a4995b28a --- /dev/null +++ b/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md @@ -0,0 +1,33 @@ +# AvoidAssignmentToAutomaticVariable + +**Severity Level: Warning** + +## Description + +`PowerShell` exposes some of its build 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. The remaining automatic variables should only be assigned to in certain special cases to achieve a certain effect as a special technique. + +To understand more about automatic variables, see ```Get-Help about_Automatic_Variables```. + +## How + +Use variable names in functions or their parameters that do not conflict with automatic variables. + +## Example + +### Wrong + +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. + +``` PowerShell +function foo($Error){ } +``` + +``` PowerShell +function Get-CustomErrorMessage($ErrorMessage){ $Error = "Error occurred: $ErrorMessage" } +``` + +### Correct + +``` PowerShell +function Get-CustomErrorMessage($ErrorMessage){ $FinalErrorMessage = "Error occurred: $ErrorMessage" } +``` From b54f759941259e1d75b4d9676d6641f8345ab5e7 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 18:41:04 +0000 Subject: [PATCH 07/12] fix test due to the addition of a new rule --- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 28980e462..4a89ec5ce 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -61,7 +61,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 52 + $expectedNumRules = 53 if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because From bc5aa8d686377bee71798362e3dde3192e60c679 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 19:42:21 +0000 Subject: [PATCH 08/12] Fix false positive in parameter attributes and add tests for it. --- Rules/AvoidAssignmentToAutomaticVariable.cs | 20 +++++++--- ...oidAssignmentToAutomaticVariable.tests.ps1 | 40 ++++++++++++++++--- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs index 066990fb2..a474b4492 100644 --- a/Rules/AvoidAssignmentToAutomaticVariable.cs +++ b/Rules/AvoidAssignmentToAutomaticVariable.cs @@ -46,13 +46,8 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - IEnumerable assignmentStatementAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: true); - IEnumerable parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true); - - - var assignmentStatementAndParamterAsts = assignmentStatementAsts.Concat(parameterAsts); - foreach (var assignmentStatementAst in assignmentStatementAndParamterAsts) + foreach (var assignmentStatementAst in assignmentStatementAsts) { var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; var variableName = variableExpressionAst.VariablePath.UserPath; @@ -62,6 +57,19 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); } } + + IEnumerable parameterAsts = ast.FindAll(testAst => testAst is ParameterAst, searchNestedScriptBlocks: true); + foreach (ParameterAst parameterAst in parameterAsts) + { + 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)) + { + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), + variableExpressionAst.Extent, GetName(), DiagnosticSeverity.Error, fileName); + } + } } /// diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index 80d9c43ca..56ba92e5b 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -2,8 +2,10 @@ $ruleName = "PSAvoidAssignmentToAutomaticVariable" Describe "AvoidAssignmentToAutomaticVariables" { - Context "ReadOnly Variables produce warning of severity error" { - It "Variable '' produce warning of severity error and throw SessionStateUnauthorizedAccessException" -TestCases @( + Context "ReadOnly Variables" { + + $readOnlyVariableSeverity = "Error" + $testCases_ReadOnlyVariables = @( @{ VariableName = '?' } @{ VariableName = 'Error' } @{ VariableName = 'ExecutionContext' } @@ -18,12 +20,40 @@ Describe "AvoidAssignmentToAutomaticVariables" { @{ VariableName = 'PSVersionTable' } @{ VariableName = 'ShellId' } @{ VariableName = 'true' } - ) { + ) + + It "Variable '' produces warning of severity error" -TestCases $testCases_ReadOnlyVariables { param ($VariableName) + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$$($VariableName) = 'foo'" | Where-Object { $_.RuleName -eq $ruleName } $warnings.Count | Should Be 1 - $warnings.Severity | Should Be "Error" - + $warnings.Severity | Should Be $readOnlyVariableSeverity + } + + It "Using Variable '' as parameter name produces warning of severity error" -TestCases $testCases_ReadOnlyVariables { + param ($VariableName) + + [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 + } + + It "Using Variable '' as parameter name in param block produces warning of severity error" -TestCases $testCases_ReadOnlyVariables { + param ($VariableName) + + [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 + } + + 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 '' throws SessionStateUnauthorizedAccessException to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { + param ($VariableName) + Set-Variable -Name $VariableName -Value 'foo' -ErrorVariable errorVariable -ErrorAction SilentlyContinue $ErrorVariable | Should Not Be Null $ErrorVariable.Exception | Should Not Be Null From cf7c4617107506c1041771aec29533d85b4c9911 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 21:06:27 +0000 Subject: [PATCH 09/12] improve and simplify testing for exception --- ...oidAssignmentToAutomaticVariable.tests.ps1 | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index 56ba92e5b..b020229b1 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -51,17 +51,23 @@ Describe "AvoidAssignmentToAutomaticVariables" { $warnings.Count | Should Be 0 } - It "Setting Variable '' throws SessionStateUnauthorizedAccessException to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { + It "Setting Variable '' throws exception to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { param ($VariableName) - - Set-Variable -Name $VariableName -Value 'foo' -ErrorVariable errorVariable -ErrorAction SilentlyContinue - $ErrorVariable | Should Not Be Null - $ErrorVariable.Exception | Should Not Be Null - if ($VariableName -ne 'Error') # setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string + + # Setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string -> exclude + if ($VariableName -ne 'Error') { - $ErrorVariable.Exception.GetType() | Should Not Be Null - $ErrorVariable.Exception.GetType().FullName | Should Be 'System.Management.Automation.SessionStateUnauthorizedAccessException' + try + { + Set-Variable -Name $VariableName -Value 'foo' -ErrorVariable errorVariable -ErrorAction Stop + throw "Expected exception did not occur when assigning value to read-only variable '$VariableName'" + } + catch + { + $_.FullyQualifiedErrorId | Should Be 'VariableNotWritable,Microsoft.PowerShell.Commands.SetVariableCommand' + } } } + } } \ No newline at end of file From 52df8d09dda82069c71dac5886537845ced08e4b Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 3 Feb 2018 21:17:07 +0000 Subject: [PATCH 10/12] exclude PSEdition from test in wmf 4 --- Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index b020229b1..cff12b06c 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -54,8 +54,9 @@ Describe "AvoidAssignmentToAutomaticVariables" { It "Setting Variable '' throws exception to verify the variables is read-only" -TestCases $testCases_ReadOnlyVariables { param ($VariableName) - # Setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string -> exclude - if ($VariableName -ne 'Error') + # 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)) { try { From 9e559d495f37c197e03d1cba27ca3c875b823aee Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 6 Feb 2018 20:47:22 +0000 Subject: [PATCH 11/12] Address PR comments --- RuleDocumentation/AvoidAssignmentToAutomaticVariable.md | 2 +- Rules/Strings.Designer.cs | 2 +- Rules/Strings.resx | 2 +- Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md b/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md index a4995b28a..d9616e418 100644 --- a/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md +++ b/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md @@ -4,7 +4,7 @@ ## Description -`PowerShell` exposes some of its build 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. The remaining automatic variables should only be assigned to in certain special cases to achieve a certain effect as a special technique. +`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. To understand more about automatic variables, see ```Get-Help about_Automatic_Variables```. diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index aaedddce4..ebfc2fcfb 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -134,7 +134,7 @@ internal static string AvoidAssignmentToReadOnlyAutomaticVariableDescription { } /// - /// Looks up a localized string similar to Variable {0} should use a different name since it is a readonly automatic variable.. + /// Looks up a localized string similar to The Variable '{0}' cannot be assigned since it is a readonly automatic variable that is built into PowerShell, please use a different name.. /// internal static string AvoidAssignmentToReadOnlyAutomaticVariableError { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 4aa651684..142b66e09 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -991,7 +991,7 @@ This automatic variables is built into PowerShell and readonly. - Variable {0} should use a different name since it is a readonly automatic variable. + The Variable '{0}' cannot be assigned since it is a readonly automatic variable that is built into PowerShell, please use a different name. AvoidAssignmentToAutomaticVariable diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index cff12b06c..c56ba6f08 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -25,7 +25,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { It "Variable '' produces warning of severity error" -TestCases $testCases_ReadOnlyVariables { param ($VariableName) - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$$($VariableName) = 'foo'" | Where-Object { $_.RuleName -eq $ruleName } + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "`$${VariableName} = 'foo'" | Where-Object { $_.RuleName -eq $ruleName } $warnings.Count | Should Be 1 $warnings.Severity | Should Be $readOnlyVariableSeverity } @@ -41,7 +41,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { It "Using Variable '' as parameter name in param block produces warning of severity error" -TestCases $testCases_ReadOnlyVariables { param ($VariableName) - [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" | Where-Object {$_.RuleName -eq $ruleName } + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" | Where-Object {$_.RuleName -eq $ruleName } $warnings.Count | Should Be 1 $warnings.Severity | Should Be $readOnlyVariableSeverity } From 14ecd3a0c2d8d75209ba3dedd884e9baf549d182 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 6 Feb 2018 21:01:13 +0000 Subject: [PATCH 12/12] trivial test fix due to merge with upstream branch --- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 0ca96f243..647301e3c 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -61,7 +61,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 53 + $expectedNumRules = 54 if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because