From 26003b319e9952ec5ea9b9a2e794abd1dd630101 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 10 Mar 2018 20:44:06 +0000 Subject: [PATCH 1/7] Warn when 'Get-' prefix was omitted. Rename some variables to make the code clearer --- Rules/AvoidAlias.cs | 47 ++++++++++++++------------- Rules/Strings.Designer.cs | 13 ++++++-- Rules/Strings.resx | 7 ++-- Tests/Rules/AvoidUsingAlias.tests.ps1 | 5 +++ 4 files changed, 46 insertions(+), 26 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 30c0858cf..3a399d493 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -1,14 +1,5 @@ -// -// 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. -// +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. using System; using System.Collections.Generic; @@ -104,38 +95,50 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) IEnumerable 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(aliasName, StringComparer.OrdinalIgnoreCase)) + if (commandName == null + || whiteList.Contains(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, commandName, cmdletNameIfCommandNameWasAlias), + GetCommandExtent(cmdAst), + GetName(), + DiagnosticSeverity.Warning, + fileName, + commandName, + suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias)); + } + + var commdNameWithGetPrefix = $"Get-{commandName}"; + var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}"); + if (cmdletNameIfCommandWasMissingGetPrefix!= null) { yield return new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, aliasName, cmdletName), + string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, commandName, commdNameWithGetPrefix), GetCommandExtent(cmdAst), GetName(), DiagnosticSeverity.Warning, fileName, - aliasName, - suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletName)); + commandName, + suggestedCorrections: GetCorrectionExtent(cmdAst, commdNameWithGetPrefix)); } } } diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 565e67629..7c0e53ec3 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -710,7 +710,7 @@ internal static string AvoidUsingClearHostName { } /// - /// Looks up a localized string similar to Avoid Using Cmdlet Aliases. + /// Looks up a localized string similar to Avoid Using Cmdlet Aliases or omitting the 'Get-' prefix.. /// internal static string AvoidUsingCmdletAliasesCommonName { get { @@ -728,7 +728,7 @@ internal static string AvoidUsingCmdletAliasesCorrectionDescription { } /// - /// Looks up a localized string similar to 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.. + /// Looks up a localized string similar to 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 availa [rest of string was truncated]";. /// internal static string AvoidUsingCmdletAliasesDescription { get { @@ -745,6 +745,15 @@ internal static string AvoidUsingCmdletAliasesError { } } + /// + /// Looks up a localized string similar to '{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.. + /// + internal static string AvoidUsingCmdletAliasesMissingGetPrefixError { + get { + return ResourceManager.GetString("AvoidUsingCmdletAliasesMissingGetPrefixError", resourceCulture); + } + } + /// /// Looks up a localized string similar to AvoidUsingCmdletAliases. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 5e0e1314e..352d5ee3f 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -118,10 +118,10 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - 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. + 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. - Avoid Using Cmdlet Aliases + Avoid Using Cmdlet Aliases or omitting the 'Get-' prefix. 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. @@ -1005,4 +1005,7 @@ AvoidAssignmentToAutomaticVariable + + '{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. + \ No newline at end of file diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index 415d20fd9..e48ac2fc3 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -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" { From dd873907f28a8af529509841626b6b3f874ef6b9 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 10 Mar 2018 20:50:34 +0000 Subject: [PATCH 2/7] update docs --- RuleDocumentation/AvoidUsingCmdletAliases.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RuleDocumentation/AvoidUsingCmdletAliases.md b/RuleDocumentation/AvoidUsingCmdletAliases.md index 0165b6d90..39d7437d0 100644 --- a/RuleDocumentation/AvoidUsingCmdletAliases.md +++ b/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. From 9b1da90343e18f38fca9065a7669b6bf82790276 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Mar 2018 22:45:19 +0000 Subject: [PATCH 3/7] do not warn for get- completion when the command exists natively (e.g. 'date' or service' on Unix systems) --- Rules/AvoidAlias.cs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 3a399d493..d6621dd58 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -127,18 +127,22 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias)); } - var commdNameWithGetPrefix = $"Get-{commandName}"; - var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}"); - if (cmdletNameIfCommandWasMissingGetPrefix!= null) + var isNativeCommand = Helper.Instance.GetCommandInfo(commandName) != null; + if (!isNativeCommand) { - yield return new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesError, commandName, commdNameWithGetPrefix), - GetCommandExtent(cmdAst), - GetName(), - DiagnosticSeverity.Warning, - fileName, - commandName, - suggestedCorrections: GetCorrectionExtent(cmdAst, commdNameWithGetPrefix)); + 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)); + } } } } From db0c195ffa1128eb20b6aa8e4053dc658408f44b Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Mar 2018 23:12:06 +0000 Subject: [PATCH 4/7] add test that there are no warnings about get- completion when native command takes precedence. Tested on Ubuntu 16.04 LTS --- Tests/Rules/AvoidUsingAlias.tests.ps1 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index e48ac2fc3..e4ecaa329 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -100,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:(!$IsLinux -and !$IsmacOS) { + $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'service' | Where-Object { $_.RuleName -eq $violationName } + $violations.Count | Should -Be 0 + } } } From 59fae3b5a72d02d52b6a73fa68caf8183edb9746 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 17 Mar 2018 23:15:07 +0000 Subject: [PATCH 5/7] use `date` for test because on some MacOs distros, service is not available --- Tests/Rules/AvoidUsingAlias.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index e4ecaa329..e95ef6e62 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -102,7 +102,7 @@ Configuration MyDscConfiguration { } It "do not warn when about Get-* completed cmdlets when the command exists natively on Unix platforms" -skip:(!$IsLinux -and !$IsmacOS) { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'service' | Where-Object { $_.RuleName -eq $violationName } + $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'date' | Where-Object { $_.RuleName -eq $violationName } $violations.Count | Should -Be 0 } } From 6e79eece80fed539e1521621985aff43a7d42ff1 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 26 Mar 2018 22:13:21 +0100 Subject: [PATCH 6/7] 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 --- Engine/Helper.cs | 64 ++++++++++++++++++++++++------ Rules/AvoidAlias.cs | 3 +- Rules/AvoidPositionalParameters.cs | 2 +- Rules/UseCmdletCorrectly.cs | 2 +- Rules/UseShouldProcessCorrectly.cs | 2 +- 5 files changed, 56 insertions(+), 17 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 31120ed99..a396cb7d4 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -410,7 +410,7 @@ public HashSet GetExportedFunction(Ast ast) IEnumerable 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 switchParams = (exportMM != null) ? exportMM.Parameters.Values.Where(pm => pm.SwitchParameter) : Enumerable.Empty(); @@ -625,7 +625,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; @@ -705,26 +705,38 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositio /// Get a CommandInfo object of the given command name /// /// Returns null if command does not exists - 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() - .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() + .FirstOrDefault(); + + return commandInfo; } } /// - /// Given a command's name, checks whether it exists + + /// Legacy method, new callers should use 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. /// - /// - /// + /// Command Name. + /// Not being used. /// - public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) + [Obsolete] + public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = null) { if (string.IsNullOrWhiteSpace(name)) { @@ -751,6 +763,32 @@ public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) } } + /// + /// Given a command's name, checks whether it exists. + /// + /// + /// + /// + 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; + } + } + /// /// Returns the get, set and test targetresource dsc function /// diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index d6621dd58..ad35ec4fe 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -10,6 +10,7 @@ #endif using System.Globalization; using System.Linq; +using System.Management.Automation; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -127,7 +128,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias)); } - var isNativeCommand = Helper.Instance.GetCommandInfo(commandName) != null; + var isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null; if (!isNativeCommand) { var commdNameWithGetPrefix = $"Get-{commandName}"; diff --git a/Rules/AvoidPositionalParameters.cs b/Rules/AvoidPositionalParameters.cs index 15902b6c2..fb2134d2d 100644 --- a/Rules/AvoidPositionalParameters.cs +++ b/Rules/AvoidPositionalParameters.cs @@ -48,7 +48,7 @@ public IEnumerable 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; diff --git a/Rules/UseCmdletCorrectly.cs b/Rules/UseCmdletCorrectly.cs index 6146711dc..43d230bf4 100644 --- a/Rules/UseCmdletCorrectly.cs +++ b/Rules/UseCmdletCorrectly.cs @@ -88,7 +88,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; diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index c7cb64296..b5e5b4a64 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -308,7 +308,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; From 31055fd0f659867b05fa067e5c0c1f0b306368d4 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 26 Mar 2018 22:18:33 +0100 Subject: [PATCH 7/7] change test syntax as requested in PR --- Tests/Rules/AvoidUsingAlias.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Rules/AvoidUsingAlias.tests.ps1 b/Tests/Rules/AvoidUsingAlias.tests.ps1 index e95ef6e62..e99d8aa47 100644 --- a/Tests/Rules/AvoidUsingAlias.tests.ps1 +++ b/Tests/Rules/AvoidUsingAlias.tests.ps1 @@ -101,7 +101,7 @@ Configuration MyDscConfiguration { $violations.Count | Should -Be 0 } - It "do not warn when about Get-* completed cmdlets when the command exists natively on Unix platforms" -skip:(!$IsLinux -and !$IsmacOS) { + 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 }