Add new AvoidDynamicallyCreatingVariableNames rule

Rules/AvoidDynamicallyCreatingVariableNames.cs

@@ -0,0 +1,141 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Linq;
+using System.Management.Automation.Language;
+
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// Rule that informs the user when they create variables with dynamic names in the general variable scope.
+    /// This might lead to conflicts with other variables.
+    /// </summary>
+#if !CORECLR
+    [Export(typeof(IScriptRule))]
+#endif
+    public class AvoidDynamicallyCreatingVariableNames : ConfigurableRule
+    {
+
+        /// <summary>
+        /// Construct an object of AvoidDynamicallyCreatingVariableNames type.
+        /// </summary>
+        public AvoidDynamicallyCreatingVariableNames() {
+            Enable = false;
+        }
+
+        readonly HashSet<string> cmdList = new HashSet<string>(Helper.Instance.CmdletNameAndAliases("New-Variable"), StringComparer.OrdinalIgnoreCase);
+
+        /// <summary>
+        /// Analyzes the given ast to find the [violation]
+        /// </summary>
+        /// <param name="ast">AST to be analyzed. This should be non-null</param>
+        /// <param name="fileName">Name of file that corresponds to the input AST.</param>
+        /// <returns>A an enumerable type containing the violations</returns>
+        public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
+            // Find all "New-Variable" commands in the Ast
+            IEnumerable<CommandAst> newVariableAsts = ast.FindAll(testAst =>
+                testAst is CommandAst cmdAst &&
+                cmdList.Contains(cmdAst.GetCommandName()),
+                true
+            ).Cast<CommandAst>();
+
+            foreach (CommandAst newVariableAst in newVariableAsts)
+            {
+                // Use StaticParameterBinder to reliably get parameter values
+                var bindingResult = StaticParameterBinder.BindCommand(newVariableAst, true);
+                if (!bindingResult.BoundParameters.ContainsKey("Name")) { continue; }
+                var nameBindingResult = bindingResult.BoundParameters["Name"];
+                // Dynamic parameters return null for the ConstantValue property
+                if (nameBindingResult.ConstantValue != null) { continue; }
+                string variableName = nameBindingResult.Value.ToString();
+                if (variableName.StartsWith("\"") && variableName.EndsWith("\""))
+                {
+                    variableName = variableName.Substring(1, variableName.Length - 2);
+                }
+                yield return new DiagnosticRecord(
+                    string.Format(
+                        CultureInfo.CurrentCulture,
+                        Strings.AvoidDynamicallyCreatingVariableNamesError,
+                        variableName),
+                    newVariableAst.Extent,
+                    GetName(),
+                    DiagnosticSeverity.Information,
+                    fileName,
+                    variableName
+                );
+            }
+        }
+
+        /// <summary>
+        /// Retrieves the common name of this rule.
+        /// </summary>
+        public override string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDynamicallyCreatingVariableNamesCommonName);
+        }
+
+        /// <summary>
+        /// Retrieves the description of this rule.
+        /// </summary>
+        public override string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDynamicallyCreatingVariableNamesDescription);
+        }
+
+        /// <summary>
+        /// Retrieves the name of this rule.
+        /// </summary>
+        public override string GetName()
+        {
+            return string.Format(
+                CultureInfo.CurrentCulture,
+                Strings.NameSpaceFormat,
+                GetSourceName(),
+                Strings.AvoidDynamicallyCreatingVariableNamesName);
+        }
+
+        /// <summary>
+        /// Retrieves the severity of the rule: error, warning or information.
+        /// </summary>
+        public override RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Information;
+        }
+
+        /// <summary>
+        /// Gets the severity of the returned diagnostic record: error, warning, or information.
+        /// </summary>
+        /// <returns></returns>
+        public DiagnosticSeverity GetDiagnosticSeverity()
+        {
+            return DiagnosticSeverity.Information;
+        }
+
+        /// <summary>
+        /// Retrieves the name of the module/assembly the rule is from.
+        /// </summary>
+        public override string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+
+        /// <summary>
+        /// Retrieves the type of the rule, Builtin, Managed or Module.
+        /// </summary>
+        public override SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+    }
+}

Rules/Strings.resx

@@ -873,6 +873,18 @@
   <data name="UseCompatibleTypesTypeAcceleratorError" xml:space="preserve">
     <value>The type accelerator '{0}' is not available by default in PowerShell version '{1}' on platform '{2}'</value>
   </data>
+  <data name="AvoidDynamicallyCreatingVariableNamesName" xml:space="preserve">
+    <value>AvoidDynamicallyCreatingVariableNames</value>
+  </data>
+  <data name="AvoidDynamicallyCreatingVariableNamesCommonName" xml:space="preserve">
+    <value>Avoid dynamically creating variable names</value>
+  </data>
+  <data name="AvoidDynamicallyCreatingVariableNamesDescription" xml:space="preserve">
+    <value>Do not create variables with a dynamic name, as this might introduce conflicts with other variables and is difficult to maintain.</value>
+  </data>
+  <data name="AvoidDynamicallyCreatingVariableNamesError" xml:space="preserve">
+    <value>'{0}' is a dynamic variable name. Please avoid creating variables with a dynamic name</value>
+  </data>
   <data name="AvoidGlobalFunctionsCommonName" xml:space="preserve">
     <value>Avoid global functions and aliases</value>
   </data>

Tests/Engine/GetScriptAnalyzerRule.tests.ps1

@@ -152,7 +152,7 @@ Describe "TestSeverity" {
 
     It "filters rules based on multiple severity inputs"{
         $rules = Get-ScriptAnalyzerRule -Severity Error,Information
-        $rules.Count | Should -Be 19
+        $rules.Count | Should -Be 20
     }
 
         It "takes lower case inputs" {

Tests/Rules/AvoidDynamicallyCreatingVariableNames.tests.ps1

@@ -0,0 +1,164 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+[Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'False positive')]
+[Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidUsingCmdletAliases', 'nv', Justification = 'For test purposes')]
+param()
+
+BeforeAll {
+    $ruleName = "PSAvoidDynamicallyCreatingVariableNames"
+    $ruleMessage = "'{0}' is a dynamic variable name. Please avoid creating variables with a dynamic name"
+}
+
+Describe "AvoidDynamicallyCreatingVariableNames" {
+
+    BeforeAll {
+        $Settings = @{
+            IncludeRules = @($ruleName)
+            Rules        = @{ $ruleName = @{ Enable = $true } }
+        }
+    }
+
+    Context "Violates" {
+        It "Basic dynamic variable name" {
+            $scriptDefinition = { New-Variable -Name $Test }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations.Count       | Should -Be 1
+            $violations.Severity    | Should -Be Information
+            $violations.Extent.Text | Should -Be {New-Variable -Name $Test}.ToString()
+            $violations.Message     | Should -Be ($ruleMessage -f '$Test')
+        }
+
+        It "Using alias" {
+            $scriptDefinition = { nv -Name $Test }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations.Count       | Should -Be 1
+            $violations.Severity    | Should -Be Information
+            $violations.Extent.Text | Should -Be {nv -Name $Test}.ToString()
+            $violations.Message     | Should -Be ($ruleMessage -f '$Test')
+        }
+
+        It "Using uppercase" {
+            $scriptDefinition = { NEW-VARIABLE -Name $Test }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations.Count       | Should -Be 1
+            $violations.Severity    | Should -Be Information
+            $violations.Extent.Text | Should -Be {NEW-VARIABLE -Name $Test}.ToString()
+            $violations.Message     | Should -Be ($ruleMessage -f '$Test')
+        }
+
+        It "Common dynamic variable iteration" {
+            $scriptDefinition = {
+                'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
+                    New-Variable -Name "My$_" -Value ($i++)
+                }
+                $MyTwo # returns 2
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations.Count       | Should -Be 1
+            $violations.Severity    | Should -Be Information
+            $violations.Extent.Text | Should -Be {New-Variable -Name "My$_" -Value ($i++)}.ToString()
+            $violations.Message     | Should -Be ($ruleMessage -f 'My$_')
+        }
+
+        It "Unquoted positional binding" {
+            $scriptDefinition = {
+                $myVarName = 'foo'
+                New-Variable $myVarName
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations.Count       | Should -Be 1
+            $violations.Severity    | Should -Be Information
+            $violations.Extent.Text | Should -Be {New-Variable $myVarName}.ToString()
+            $violations.Message     | Should -Be ($ruleMessage -f '$myVarName')
+        }
+
+        It "Quoted positional binding" {
+            $scriptDefinition = {
+                'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
+                    New-Variable "My$_" ($i++)
+                }
+                $MyTwo # returns 2
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations.Count       | Should -Be 1
+            $violations.Severity    | Should -Be Information
+            $violations.Extent.Text | Should -Be {New-Variable "My$_" ($i++)}.ToString()
+            $violations.Message     | Should -Be ($ruleMessage -f 'My$_')
+        }
+    }
+
+    Context "Compliant" {
+        It "Common hash table population" {
+            $scriptDefinition = {
+                $My = @{}
+                'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
+                    $My[$_] = $i++
+                }
+                $My.Two # returns 2
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations | Should -BeNullOrEmpty
+        }
+
+        It "Scoped hash table population" {
+            $scriptDefinition = {
+                New-Variable -Name My -Value @{} -Option ReadOnly -Scope Script
+                'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
+                    $Script:My[$_] = $i++
+                }
+                $Script:My.Two # returns 2
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations | Should -BeNullOrEmpty
+        }
+
+        It "Verbatim (single quoted) name with dollar sign" {
+            $scriptDefinition = {
+                New-Variable -Name '$Sign1'
+                New-Variable -Name '$Sign2' -Value 'Dollar'
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations | Should -BeNullOrEmpty
+        }
+    }
+
+    Context "Disabled" {
+        BeforeAll {
+            $Settings = @{
+                IncludeRules = @($ruleName)
+                Rules        = @{ $ruleName = @{ Enable = $false } }
+            }
+        }
+
+        It "ConvertFrom-SecureString -AsPlainText" {
+            $scriptDefinition = { New-Variable -Name $Test }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations | Should -BeNullOrEmpty
+        }
+    }
+
+    Context "Suppressed" {
+        It "Basic dynamic variable name" {
+            $scriptDefinition = {
+                [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicallyCreatingVariableNames', '$Test', Justification = 'Test')]
+                Param()
+                New-Variable -Name $Test
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations | Should -BeNullOrEmpty
+        }
+        It "Common dynamic variable iteration" {
+            $scriptDefinition = {
+                [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidDynamicallyCreatingVariableNames', 'My$_', Justification = 'Test')]
+                Param()
+                'One', 'Two', 'Three' | ForEach-Object -Begin { $i = 1 } -Process {
+                    New-Variable -Name "My$_" -Value ($i++)
+                }
+                $MyTwo # returns 2
+            }.ToString()
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
+            $violations | Should -BeNullOrEmpty
+        }
+    }
+}