WIP: restore GetCommandInfo to non-obsolete status

Engine/Helper.cs

@@ -399,7 +399,7 @@ public HashSet<string> GetExportedFunction(Ast ast)
             IEnumerable<Ast> cmdAsts = ast.FindAll(item => item is CommandAst
                 && exportFunctionsCmdlet.Contains((item as CommandAst).GetCommandName(), StringComparer.OrdinalIgnoreCase), true);
 
-            CommandInfo exportMM = Helper.Instance.GetCommandInfoLegacy("export-modulemember", CommandTypes.Cmdlet);
+            CommandInfo exportMM = Helper.Instance.GetCommandInfo("export-modulemember", CommandTypes.Cmdlet);
 
             // switch parameters
             IEnumerable<ParameterMetadata> switchParams = (exportMM != null) ? exportMM.Parameters.Values.Where<ParameterMetadata>(pm => pm.SwitchParameter) : Enumerable.Empty<ParameterMetadata>();
@@ -614,7 +614,7 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositio
                 return false;
             }
 
-            var commandInfo = GetCommandInfoLegacy(cmdAst.GetCommandName());
+            var commandInfo = GetCommandInfo(cmdAst.GetCommandName());
             if (commandInfo == null || (commandInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet))
             {
                 return false;
@@ -694,88 +694,71 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositio
         /// Get a CommandInfo object of the given command name
         /// </summary>
         /// <returns>Returns null if command does not exists</returns>
-        private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType)
+        private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes commandType)
         {
+            if (string.IsNullOrWhiteSpace(cmdName))
+            {
+                return null;
+            }
+
+            // Don't hold the lock during the pipeline call
+            lock (getCommandLock)
+            {
+                CommandInfo cmdletInfo;
+                if (commandInfoCache.TryGetValue(cmdName, out cmdletInfo)) {
+                    return cmdletInfo;
+                }
+            }
+
             using (var ps = System.Management.Automation.PowerShell.Create())
             {
                 var psCommand = ps.AddCommand("Get-Command")
                     .AddParameter("Name", cmdName)
-                    .AddParameter("ErrorAction", "SilentlyContinue");
-
-                if(commandType!=null)
-                {
-                    psCommand.AddParameter("CommandType", commandType);
-                }
+                    .AddParameter("ErrorAction", "SilentlyContinue")
+                    .AddParameter("CommandType", commandType);
 
                 var commandInfo = psCommand.Invoke<CommandInfo>()
                          .FirstOrDefault();
 
+                lock (getCommandLock)
+                {
+                    CommandInfo cmdletInfo;
+                    if (! commandInfoCache.TryGetValue(cmdName, out cmdletInfo) )
+                    {
+                        commandInfoCache.Add(cmdName, commandInfo);
+                    }
+                }
                 return commandInfo;
             }
         }
 
         /// <summary>
-
-        ///  Legacy method, new callers should use <see cref="GetCommandInfo"/> instead.
-        ///  Given a command's name, checks whether it exists. It does not use the passed in CommandTypes parameter, which is a bug.
-        ///  But existing method callers are already depending on this behaviour and therefore this could not be simply fixed.
-        ///  It also populates the commandInfoCache which can have side effects in some cases.
+        /// Get a CommandInfo object of the given command name and type
+        /// <param name="name"></param>
+        /// <param name="commandType"></param>
         /// </summary>
-        /// <param name="name">Command Name.</param>
-        /// <param name="commandType">Not being used.</param>
-        /// <returns></returns>
-        [Obsolete]
-        public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = null)
+        /// <returns>The first CommandInfo found based on the name and type or null if nothing was found</returns>
+        public CommandInfo GetCommandInfo(string name, CommandTypes commandType)
         {
-            if (string.IsNullOrWhiteSpace(name))
-            {
-                return null;
-            }
-
-            // check if it is an alias
-            string cmdletName = Helper.Instance.GetCmdletNameFromAlias(name);
-            if (string.IsNullOrWhiteSpace(cmdletName))
-            {
-                cmdletName = name;
-            }
-
-            lock (getCommandLock)
-            {
-                if (commandInfoCache.ContainsKey(cmdletName))
-                {
-                    return commandInfoCache[cmdletName];
-                }
-
-                var commandInfo = GetCommandInfoInternal(cmdletName, commandType);
-                commandInfoCache.Add(cmdletName, commandInfo);
-                return commandInfo;
-            }
+            // We don't check for alias in this case as the user has been explicit about
+            // the CommandTypes desired
+            return GetCommandInfoInternal(name, commandType);
         }
 
         /// <summary>
-        /// Given a command's name, checks whether it exists.
+        /// Given a command's name, retrieve its CommandInfo if it exists
         /// </summary>
         /// <param name="name"></param>
-        /// <param name="commandType"></param>
-        /// <returns></returns>
-        public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null)
+        /// <returns>The first CommandInfo found based on the name of any command type or null if nothing was found</returns>
+        public CommandInfo GetCommandInfo(string name)
         {
-            if (string.IsNullOrWhiteSpace(name))
-            {
-                return null;
-            }
-
-            lock (getCommandLock)
+            // check to see if it's an alias and use the resolved cmdlet name if it is
+            string cmdletName = Helper.Instance.GetCmdletNameFromAlias(name);
+            if ( string.IsNullOrWhiteSpace(cmdletName))
             {
-                if (commandInfoCache.ContainsKey(name))
-                {
-                    return commandInfoCache[name];
-                }
-
-                var commandInfo = GetCommandInfoInternal(name, commandType);
-
-                return commandInfo;
+                cmdletName = name;
             }
+            return GetCommandInfo(cmdletName, CommandTypes.All);
         }
 
         /// <summary>

Rules/AvoidPositionalParameters.cs

@@ -39,7 +39,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                 // MSDN: CommandAst.GetCommandName Method
                 if (cmdAst.GetCommandName() == null) continue;
 
-                if (Helper.Instance.GetCommandInfoLegacy(cmdAst.GetCommandName()) != null
+                if (Helper.Instance.GetCommandInfo(cmdAst.GetCommandName()) != null
                     && Helper.Instance.PositionalParameterUsed(cmdAst, true))
                 {
                     PipelineAst parent = cmdAst.Parent as PipelineAst;

Rules/UseCmdletCorrectly.cs

@@ -79,7 +79,7 @@ private bool MandatoryParameterExists(CommandAst cmdAst)
 
             #region Compares parameter list and mandatory parameter list.
 
-            cmdInfo = Helper.Instance.GetCommandInfoLegacy(cmdAst.GetCommandName());
+            cmdInfo = Helper.Instance.GetCommandInfo(cmdAst.GetCommandName());
             if (cmdInfo == null || (cmdInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet))
             {
                 return true;

Rules/UseShouldProcessCorrectly.cs

@@ -299,7 +299,7 @@ private bool SupportsShouldProcess(string cmdName)
                 return false;
             }
 
-            var cmdInfo = Helper.Instance.GetCommandInfoLegacy(cmdName);
+            var cmdInfo = Helper.Instance.GetCommandInfo(cmdName);
             if (cmdInfo == null)
             {
                 return false;

Tests/Rules/AvoidPositionalParameters.tests.ps1

@@ -1,21 +1,18 @@
 Import-Module PSScriptAnalyzer
-$violationMessage = "Cmdlet 'Get-Command' has positional parameter. Please use named parameters instead of positional parameters when calling a command."
-$violationName = "PSAvoidUsingPositionalParameters"
-$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
-$violations = Invoke-ScriptAnalyzer $directory\AvoidPositionalParameters.ps1 | Where-Object {$_.RuleName -eq $violationName}
-$noViolations = Invoke-ScriptAnalyzer $directory\AvoidPositionalParametersNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName}
-$noViolationsDSC = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\serviceconfigdisabled.ps1 | Where-Object {$_.RuleName -eq $violationName}
 
 Describe "AvoidPositionalParameters" {
+    BeforeAll {
+        $directory = $PSScriptRoot
+        $violationName = "PSAvoidUsingPositionalParameters"
+        $violation = Invoke-ScriptAnalyzer -ScriptDefinition 'Get-Command "abc" 4 4.3'
+        $noViolations = Invoke-ScriptAnalyzer $directory\AvoidPositionalParametersNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName}
+        $noViolationsDSC = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\serviceconfigdisabled.ps1 | Where-Object {$_.RuleName -eq $violationName}
+    }
     Context "When there are violations" {
         It "has 1 avoid positional parameters violation" {
-            $violations.Count | Should -Be 1
-        }
-
-        It "has the correct description message" {
-            $violations[0].Message | Should -Match $violationMessage
+            @($violation).Count | Should -Be 1
+            $violation.RuleName | Should -Be $violationName
         }
-
     }
 
     Context "When there are no violations" {
@@ -27,4 +24,4 @@ Describe "AvoidPositionalParameters" {
             $noViolationsDSC.Count | Should -Be 0
         }
     }
-}
+}

Tests/Rules/UseCmdletCorrectly.tests.ps1

@@ -1,24 +1,19 @@
-Import-Module -Verbose PSScriptAnalyzer
-$violationMessage = "Cmdlet 'Write-Warning' may be used incorrectly. Please check that all mandatory parameters are supplied."
-$violationName = "PSUseCmdletCorrectly"
-$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
-$violations = Invoke-ScriptAnalyzer $directory\UseCmdletCorrectly.ps1 | Where-Object {$_.RuleName -eq $violationName}
-$noViolations = Invoke-ScriptAnalyzer $directory\GoodCmdlet.ps1 | Where-Object {$_.RuleName -eq $violationName}
-
+
 Describe "UseCmdletCorrectly" {
     Context "When there are violations" {
         It "has 1 Use Cmdlet Correctly violation" {
-            $violations.Count | Should -Be 1
-        }
-
-        It "has the correct description message" {
-            $violations[0].Message | Should -Match $violationMessage
+            $violationName = "PSUseCmdletCorrectly"
+            $violation = Invoke-ScriptAnalyzer -ScriptDefinition 'Write-Warning;Write-Warning -Message "a warning"'
+            $violation.Count | Should -Be 1
+            $violation.RuleName | Should -Be $violationName
         }
     }
 
     Context "When there are no violations" {
         It "returns no violations" {
+            $directory = $PSScriptRoot
+            $noViolations = Invoke-ScriptAnalyzer $directory\GoodCmdlet.ps1 
             $noViolations.Count | Should -Be 0
         }
     }
-}
+}

appveyor.yml

@@ -38,6 +38,7 @@ test_script:
   - ps:   |
             if ($env:PowerShellEdition -eq 'WindowsPowerShell') {
                 $modulePath = $env:PSModulePath.Split([System.IO.Path]::PathSeparator) | Where-Object { Test-Path $_} | Select-Object -First 1
+                if ( Test-Path "$modulePath\PSScriptAnalyzer" ) { Remove-Item -recurse -force "$modulePath\PSScriptAnalyzer" }
                 Copy-Item "${env:APPVEYOR_BUILD_FOLDER}\out\PSScriptAnalyzer" "$modulePath\" -Recurse -Force
                 $testResultsFile = ".\TestResults.xml"
                 $testScripts = "${env:APPVEYOR_BUILD_FOLDER}\Tests\Engine","${env:APPVEYOR_BUILD_FOLDER}\Tests\Rules"