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/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md b/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md new file mode 100644 index 000000000..d9616e418 --- /dev/null +++ b/RuleDocumentation/AvoidAssignmentToAutomaticVariable.md @@ -0,0 +1,33 @@ +# AvoidAssignmentToAutomaticVariable + +**Severity Level: Warning** + +## Description + +`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```. + +## 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" } +``` diff --git a/Rules/AvoidAssignmentToAutomaticVariable.cs b/Rules/AvoidAssignmentToAutomaticVariable.cs new file mode 100644 index 000000000..a474b4492 --- /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; +#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 + { + 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" + }; + + /// + /// Checks for assignment to automatic variables. + /// 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); + foreach (var assignmentStatementAst in assignmentStatementAsts) + { + var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst; + var variableName = variableExpressionAst.VariablePath.UserPath; + if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase)) + { + yield return new DiagnosticRecord(DiagnosticRecordHelper.FormatError(Strings.AvoidAssignmentToReadOnlyAutomaticVariableError, variableName), + 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); + } + } + } + + /// + /// 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.AvoidAssignmentToReadOnlyAutomaticVariableCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidAssignmentToReadOnlyAutomaticVariableDescription); + } + + /// + /// 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 9905be482..565e67629 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -97,6 +97,51 @@ internal static string AlignAssignmentStatementName { } } + /// + /// 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 Use a different variable name. + /// + internal static string AvoidAssignmentToReadOnlyAutomaticVariable { + get { + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariable", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Changing automtic variables might have undesired side effects. + /// + internal static string AvoidAssignmentToReadOnlyAutomaticVariableCommonName { + get { + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariableCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to This automatic variables is built into PowerShell and readonly.. + /// + internal static string AvoidAssignmentToReadOnlyAutomaticVariableDescription { + get { + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariableDescription", resourceCulture); + } + } + + /// + /// 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 { + return ResourceManager.GetString("AvoidAssignmentToReadOnlyAutomaticVariableError", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using ComputerName Hardcoded. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 69de3a14f..5e0e1314e 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -990,4 +990,19 @@ PossibleIncorrectUsageOfAssignmentOperator + + Use a different variable name + + + Changing automtic variables might have undesired side effects + + + This automatic variables is built into PowerShell and readonly. + + + The Variable '{0}' cannot be assigned since it is a readonly automatic variable that is built into PowerShell, please use a different name. + + + AvoidAssignmentToAutomaticVariable + \ No newline at end of file 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 diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 new file mode 100644 index 000000000..c56ba6f08 --- /dev/null +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -0,0 +1,74 @@ +Import-Module PSScriptAnalyzer +$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' } + ) + + 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 $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(`$$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 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, 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 + { + 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