Skip to content

Warn when 'Get-' prefix was omitted. #927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
64 changes: 51 additions & 13 deletions Engine/Helper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

{
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))
{
Expand All @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion RuleDocumentation/AvoidUsingCmdletAliases.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
39 changes: 28 additions & 11 deletions Rules/AvoidAlias.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#endif
using System.Globalization;
using System.Linq;
using System.Management.Automation;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
Expand Down Expand Up @@ -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));
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Rules/AvoidPositionalParameters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 11 additions & 2 deletions Rules/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 5 additions & 2 deletions Rules/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
2 changes: 1 addition & 1 deletion Rules/UseCmdletCorrectly.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion Rules/UseShouldProcessCorrectly.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 10 additions & 0 deletions Tests/Rules/AvoidUsingAlias.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down Expand Up @@ -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
}
}
}