From 9a6bc97ae499c80621e1145f60281d5dbaa45b6b Mon Sep 17 00:00:00 2001 From: James Truher Date: Fri, 30 Mar 2018 13:15:33 -0700 Subject: [PATCH 1/4] Remove obsolete attribute from GetCommandInfo Add overload for GetCommandInfo without CommandType to match general usage Remove GetCommandInfoLegacy method --- Engine/Helper.cs | 82 ++++++++---------------------- Rules/AvoidPositionalParameters.cs | 2 +- Rules/UseCmdletCorrectly.cs | 2 +- Rules/UseShouldProcessCorrectly.cs | 2 +- 4 files changed, 23 insertions(+), 65 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 4adf407ad..7e892fc17 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -399,7 +399,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.GetCommandInfoLegacy("export-modulemember", CommandTypes.Cmdlet); + CommandInfo exportMM = Helper.Instance.GetCommandInfo("export-modulemember", CommandTypes.Cmdlet); // switch parameters IEnumerable switchParams = (exportMM != null) ? exportMM.Parameters.Values.Where(pm => pm.SwitchParameter) : Enumerable.Empty(); @@ -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,18 +694,19 @@ 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) + private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes commandType) { + if (string.IsNullOrWhiteSpace(cmdName)) + { + return null; + } + 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() .FirstOrDefault(); @@ -715,67 +716,24 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command } /// - - /// 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. + /// Get a CommandInfo object of the given command name and type + /// + /// /// - /// Command Name. - /// Not being used. - /// - [Obsolete] - public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = null) + /// The first CommandInfo found based on the name and type or null if nothing was found + 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; - } + return GetCommandInfoInternal(name, commandType); } /// - /// Given a command's name, checks whether it exists. + /// Given a command's name, retrieve its CommandInfo if it exists /// /// - /// - /// - public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) + /// The first CommandInfo found based on the name of any command type or null if nothing was found + public CommandInfo GetCommandInfo(string name) { - if (string.IsNullOrWhiteSpace(name)) - { - return null; - } - - lock (getCommandLock) - { - if (commandInfoCache.ContainsKey(name)) - { - return commandInfoCache[name]; - } - - var commandInfo = GetCommandInfoInternal(name, commandType); - - return commandInfo; - } + return GetCommandInfo(name, CommandTypes.All); } /// diff --git a/Rules/AvoidPositionalParameters.cs b/Rules/AvoidPositionalParameters.cs index de530ab78..60f5cea8f 100644 --- a/Rules/AvoidPositionalParameters.cs +++ b/Rules/AvoidPositionalParameters.cs @@ -39,7 +39,7 @@ public IEnumerable 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; diff --git a/Rules/UseCmdletCorrectly.cs b/Rules/UseCmdletCorrectly.cs index fae21cfad..37cfa3cdb 100644 --- a/Rules/UseCmdletCorrectly.cs +++ b/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; diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 6be9db87e..8f619067d 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/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; From d59d9312b4cf893ada04a220f4b8baf72500d3a7 Mon Sep 17 00:00:00 2001 From: James Truher Date: Mon, 2 Apr 2018 14:35:24 -0700 Subject: [PATCH 2/4] Be sure to use the command cache to improve performance Previous implementation of command cache usage would take the lock during the pipeline execution of Get-Command which could be very long. This breaks up the lock to not include the pipeline invocation. --- Engine/Helper.cs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 7e892fc17..a0e1ef06f 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -701,6 +701,15 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes commandT 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") @@ -711,6 +720,14 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes commandT var commandInfo = psCommand.Invoke() .FirstOrDefault(); + lock (getCommandLock) + { + CommandInfo cmdletInfo; + if (! commandInfoCache.TryGetValue(cmdName, out cmdletInfo) ) + { + commandInfoCache.Add(cmdName, commandInfo); + } + } return commandInfo; } } @@ -723,6 +740,8 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes commandT /// The first CommandInfo found based on the name and type or null if nothing was found public CommandInfo GetCommandInfo(string name, CommandTypes commandType) { + // We don't check for alias in this case as the user has been explicit about + // the CommandTypes desired return GetCommandInfoInternal(name, commandType); } @@ -733,7 +752,13 @@ public CommandInfo GetCommandInfo(string name, CommandTypes commandType) /// The first CommandInfo found based on the name of any command type or null if nothing was found public CommandInfo GetCommandInfo(string name) { - return GetCommandInfo(name, CommandTypes.All); + // 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)) + { + cmdletName = name; + } + return GetCommandInfo(cmdletName, CommandTypes.All); } /// From 0f562dfa15b65c78604f47a6c0874fa100af3fcd Mon Sep 17 00:00:00 2001 From: James Truher Date: Wed, 4 Apr 2018 11:26:33 -0700 Subject: [PATCH 3/4] Be sure to remove the target location of script analyzer to be sure that no pollution has occurred --- appveyor.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/appveyor.yml b/appveyor.yml index ef8d68836..c99695dce 100644 --- a/appveyor.yml +++ b/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" From 9ab158f5e7af72a8261d533aa27e6ff8fbc49ef0 Mon Sep 17 00:00:00 2001 From: James Truher Date: Wed, 4 Apr 2018 13:15:13 -0700 Subject: [PATCH 4/4] Simplify failing tests by inlining it and don't check the message as all we should really check is the rule violation --- Tests/Rules/AvoidPositionalParameters.ps1 | 2 -- .../Rules/AvoidPositionalParameters.tests.ps1 | 23 ++++++++----------- Tests/Rules/UseCmdletCorrectly.ps1 | 5 ---- Tests/Rules/UseCmdletCorrectly.tests.ps1 | 21 +++++++---------- 4 files changed, 18 insertions(+), 33 deletions(-) delete mode 100644 Tests/Rules/AvoidPositionalParameters.ps1 delete mode 100644 Tests/Rules/UseCmdletCorrectly.ps1 diff --git a/Tests/Rules/AvoidPositionalParameters.ps1 b/Tests/Rules/AvoidPositionalParameters.ps1 deleted file mode 100644 index 93e3acc38..000000000 --- a/Tests/Rules/AvoidPositionalParameters.ps1 +++ /dev/null @@ -1,2 +0,0 @@ -# give it 3 positional parameters -Get-Command "abc" 4 4.3 \ No newline at end of file diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index 9ff360e3e..8b371bbbf 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/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 } } -} \ No newline at end of file +} diff --git a/Tests/Rules/UseCmdletCorrectly.ps1 b/Tests/Rules/UseCmdletCorrectly.ps1 deleted file mode 100644 index 373078af8..000000000 --- a/Tests/Rules/UseCmdletCorrectly.ps1 +++ /dev/null @@ -1,5 +0,0 @@ -Write-Warning -Wrong-Cmd -Write-Verbose -Message "Write Verbose" -Write-Verbose "Warning" -OutVariable $test -Write-Verbose "Warning" | PipeLineCmdlet \ No newline at end of file diff --git a/Tests/Rules/UseCmdletCorrectly.tests.ps1 b/Tests/Rules/UseCmdletCorrectly.tests.ps1 index 7447eceb2..6b3147cd9 100644 --- a/Tests/Rules/UseCmdletCorrectly.tests.ps1 +++ b/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 } } -} \ No newline at end of file +}