Warn when 'Get-' prefix was omitted as part of the PSAvoidUsingCmdletAliases rule. (#927)

* Warn when 'Get-' prefix was omitted. Rename some variables to make the code clearer

* update docs

* do not warn for get- completion when the command exists natively (e.g. 'date' or service' on Unix systems)

* add test that there are no warnings about get- completion when native command takes precedence. Tested on Ubuntu 16.04 LTS

* use `date` for test because on some MacOs distros, service is not available

* Fix GetCommandInfoInternal method to actually use the CommandTypes argument
However, its calling method GetCommandInfo was relying on this bug (or its callers), therefore the method was declared legacy in order to not break existing behaviour

* change test syntax as requested in PR

Author: bergmeister

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.GetCommandInfo("export-modulemember", CommandTypes.Cmdlet);
+            CommandInfo exportMM = Helper.Instance.GetCommandInfoLegacy("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 = GetCommandInfo(cmdAst.GetCommandName());
+            var commandInfo = GetCommandInfoLegacy(cmdAst.GetCommandName());
             if (commandInfo == null || (commandInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet))
             {
                 return false;
@@ -694,26 +694,38 @@ 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 = null)
+        private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType)
         {
             using (var ps = System.Management.Automation.PowerShell.Create())
             {
-                var cmdInfo = ps.AddCommand("Get-Command")
-                                .AddArgument(cmdName)
-                                .AddParameter("ErrorAction", "SilentlyContinue")
-                                .Invoke<CommandInfo>()
-                                .FirstOrDefault();
-                return cmdInfo;
+                var psCommand = ps.AddCommand("Get-Command")
+                    .AddParameter("Name", cmdName)
+                    .AddParameter("ErrorAction", "SilentlyContinue");
+
+                if(commandType!=null)
+                {
+                    psCommand.AddParameter("CommandType", commandType);
+                }
+
+                var commandInfo = psCommand.Invoke<CommandInfo>()
+                         .FirstOrDefault();
+
+                return commandInfo;
             }
         }
 
         /// <summary>
-        /// Given a command's name, checks whether it exists
+
+        ///  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.
         /// </summary>
-        /// <param name="name"></param>
-        /// <param name="commandType"></param>
+        /// <param name="name">Command Name.</param>
+        /// <param name="commandType">Not being used.</param>
         /// <returns></returns>
-        public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null)
+        [Obsolete]
+        public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = null)
         {
             if (string.IsNullOrWhiteSpace(name))
             {
@@ -740,6 +752,32 @@ public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null)
             }
         }
 
+        /// <summary>
+        /// Given a command's name, checks whether it exists.
+        /// </summary>
+        /// <param name="name"></param>
+        /// <param name="commandType"></param>
+        /// <returns></returns>
+        public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null)
+        {
+            if (string.IsNullOrWhiteSpace(name))
+            {
+                return null;
+            }
+
+            lock (getCommandLock)
+            {
+                if (commandInfoCache.ContainsKey(name))
+                {
+                    return commandInfoCache[name];
+                }
+
+                var commandInfo = GetCommandInfoInternal(name, commandType);
+
+                return commandInfo;
+            }
+        }
+
         /// <summary>
         /// Returns the get, set and test targetresource dsc function
         /// </summary>

RuleDocumentation/AvoidUsingCmdletAliases.md

@@ -5,7 +5,7 @@
 ## Description
 
 An alias is an alternate name or nickname for a CMDLet or for a command element, such as a function, script, file, or executable file.
-You can use the alias instead of the command name in any Windows PowerShell commands.
+You can use the alias instead of the command name in any Windows PowerShell commands. There are also implicit aliases: When PowerShell cannot find the cmdlet name, it will try to append `Get-` to the command as a last resort before, therefore e.g. `verb` will excute `Get-Verb`.
 
 Every PowerShell author learns the actual command names, but different authors learn and use different aliases. Aliases can make code difficult to read, understand and
 impact availability.

Rules/AvoidAlias.cs

@@ -10,6 +10,7 @@
 #endif
 using System.Globalization;
 using System.Linq;
+using System.Management.Automation;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 {
@@ -95,38 +96,54 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
             IEnumerable<Ast> foundAsts = ast.FindAll(testAst => testAst is CommandAst, true);
 
             // Iterates all CommandAsts and check the command name.
-            foreach (Ast foundAst in foundAsts)
+            foreach (CommandAst cmdAst in foundAsts)
             {
-                CommandAst cmdAst = (CommandAst)foundAst;
-
                 // Check if the command ast should be ignored
                 if (IgnoreCommandast(cmdAst))
                 {
                     continue;
                 }
 
-                string aliasName = cmdAst.GetCommandName();
+                string commandName = cmdAst.GetCommandName();
 
                 // Handles the exception caused by commands like, {& $PLINK $args 2> $TempErrorFile}.
                 // You can also review the remark section in following document,
                 // MSDN: CommandAst.GetCommandName Method
-                if (aliasName == null
-                    || whiteList.Contains<string>(aliasName, StringComparer.OrdinalIgnoreCase))
+                if (commandName == null
+                    || whiteList.Contains<string>(commandName, StringComparer.OrdinalIgnoreCase))
                 {
                     continue;
                 }
 
-                string cmdletName = Helper.Instance.GetCmdletNameFromAlias(aliasName);
-                if (!String.IsNullOrEmpty(cmdletName))
+                string cmdletNameIfCommandNameWasAlias = Helper.Instance.GetCmdletNameFromAlias(commandName);
+                if (!String.IsNullOrEmpty(cmdletNameIfCommandNameWasAlias))
                 {
                     yield return new DiagnosticRecord(
-                        string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, aliasName, cmdletName),
+                        string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, commandName, cmdletNameIfCommandNameWasAlias),
                         GetCommandExtent(cmdAst),
                         GetName(),
                         DiagnosticSeverity.Warning,
                         fileName,
-                        aliasName,
-                        suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletName));
+                        commandName,
+                        suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias));
+                }
+
+                var isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null;
+                if (!isNativeCommand)
+                {
+                    var commdNameWithGetPrefix = $"Get-{commandName}";
+                    var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}");
+                    if (cmdletNameIfCommandWasMissingGetPrefix != null)
+                    {
+                        yield return new DiagnosticRecord(
+                            string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesMissingGetPrefixError, commandName, commdNameWithGetPrefix),
+                            GetCommandExtent(cmdAst),
+                            GetName(),
+                            DiagnosticSeverity.Warning,
+                            fileName,
+                            commandName,
+                            suggestedCorrections: GetCorrectionExtent(cmdAst, commdNameWithGetPrefix));
+                    }
                 }
             }
         }

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.GetCommandInfo(cmdAst.GetCommandName()) != null
+                if (Helper.Instance.GetCommandInfoLegacy(cmdAst.GetCommandName()) != null
                     && Helper.Instance.PositionalParameterUsed(cmdAst, true))
                 {
                     PipelineAst parent = cmdAst.Parent as PipelineAst;

Rules/Strings.Designer.cs

@@ -118,10 +118,10 @@
     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
   </resheader>
   <data name="AvoidUsingCmdletAliasesDescription" xml:space="preserve">
-    <value>An alias is an alternate name or nickname for a cmdlet or for a command element, such as a function, script, file, or executable file. But when writing scripts that will potentially need to be maintained over time, either by the original author or another Windows PowerShell scripter, please consider using full cmdlet name instead of alias. Aliases can introduce these problems, readability, understandability and availability.</value>
+    <value>An alias is an alternate name or nickname for a cmdlet or for a command element, such as a function, script, file, or executable file. An implicit alias is also the omission of the 'Get-' prefix for commands with this prefix. But when writing scripts that will potentially need to be maintained over time, either by the original author or another Windows PowerShell scripter, please consider using full cmdlet name instead of alias. Aliases can introduce these problems, readability, understandability and availability.</value>
   </data>
   <data name="AvoidUsingCmdletAliasesCommonName" xml:space="preserve">
-    <value>Avoid Using Cmdlet Aliases</value>
+    <value>Avoid Using Cmdlet Aliases or omitting the 'Get-' prefix.</value>
   </data>
   <data name="AvoidUsingEmptyCatchBlockDescription" xml:space="preserve">
     <value>Empty catch blocks are considered poor design decisions because if an error occurs in the try block, this error is simply swallowed and not acted upon. While this does not inherently lead to bad things. It can and this should be avoided if possible. To fix a violation of this rule, using Write-Error or throw statements in catch blocks.</value>
@@ -1005,4 +1005,7 @@
   <data name="AvoidAssignmentToAutomaticVariableName" xml:space="preserve">
     <value>AvoidAssignmentToAutomaticVariable</value>
   </data>
+  <data name="AvoidUsingCmdletAliasesMissingGetPrefixError" xml:space="preserve">
+    <value>'{0}' is implicitly aliasing '{1}' because it is missing the 'Get-' prefix. This can introduce possible problems and make scripts hard to maintain. Please consider changing command to its full name.</value>
+  </data>
 </root>

Rules/Strings.resx

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

Rules/UseCmdletCorrectly.cs

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

Rules/UseShouldProcessCorrectly.cs

@@ -26,6 +26,11 @@ Describe "AvoidUsingAlias" {
             Test-CorrectionExtent $violationFilepath $violations[1] 1 'cls' 'Clear-Host'
             $violations[1].SuggestedCorrections[0].Description | Should -Be 'Replace cls with Clear-Host'
         }
+
+        It "warns when 'Get-' prefix was omitted" {
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'verb' | Where-Object { $_.RuleName -eq $violationName }
+            $violations.Count | Should -Be 1
+        }
     }
 
     Context "Violation Extent" {
@@ -95,5 +100,10 @@ Configuration MyDscConfiguration {
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition "CD" -Settings $settings -IncludeRule $violationName
             $violations.Count | Should -Be 0
         }
+
+        It "do not warn when about Get-* completed cmdlets when the command exists natively on Unix platforms" -skip:(-not ($IsLinux -or $IsMacOS)) {
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'date' | Where-Object { $_.RuleName -eq $violationName }
+            $violations.Count | Should -Be 0
+        }
     }
 }