From 9e36ac36f31a7468eab60a70bc028673f7f09cd6 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 15 Jan 2020 17:45:35 -0800 Subject: [PATCH 1/4] Add tests --- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 43775b36d..af79c6e7a 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -66,6 +66,22 @@ function MyFunc2() { $results | Get-Count | Should -Be 1 $results[0].Extent | Should -Be '$mySecondvar' } + + It "Understands that variables set in a child scope are not the same" { + $script = @' +$flag = $true +& { + $flag = $false +} +Write-Output $flag +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script + $diagnostics | Should -HaveCount 1 + $diagnostics[0].Extent.StartLineNumber | Should -Be 3 + $diagnostics[0].Extent.Text | Should -BeExactly '$flag' + } + } Context "When there are no violations" { @@ -124,5 +140,32 @@ function MyFunc2() { $scriptDefinition = '$ArbitrarySwitchParameter = 1; Get-Variable -ArbitrarySwitchParameter' (Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition).Count | Should -Be 1 -Because $scriptDefinition } + + It "Understands that variables immediately dot-sourced in a script block are the same" { + $script = @' +$flag = $true +. { + $flag = $false +} +Write-Output $flag +'@ + + Invoke-ScriptAnalyzer -ScriptDefinition $script | Should -HaveCount 0 + } + + It "Understands that variables used in a ForEach-Object script block are the same" { + $script = @' +$flag = $false +1,2,3 | foreach-object { + if ($_ -eq 2) + { + $flag = $true + } +} +Write-Host $flag +'@ + + Invoke-ScriptAnalyzer -ScriptDefinition $script | Should -HaveCount 0 + } } } From ce78a4fd68f175f2a8807c9007b2e6c60e201a9a Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 15 Jan 2020 18:10:16 -0800 Subject: [PATCH 2/4] Add another test --- .../UseDeclaredVarsMoreThanAssignments.tests.ps1 | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index af79c6e7a..adfb28a96 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -82,6 +82,21 @@ Write-Output $flag $diagnostics[0].Extent.Text | Should -BeExactly '$flag' } + It "Understands when variables are not used in child scopes" { +$script = @' +$duck = $true +& { + $duck = $false + Write-Output $duck +} +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script + $diagnostics | Should -HaveCount 1 + $diagnostics[0].Extent.StartLineNumber | Should -Be 1 + $diagnostics[0].Extent.Text | Should -BeExactly '$duck' + } + } Context "When there are no violations" { From 9b9bf810a693b54c1d381d5b4ee0aa7f2800b36a Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 15 Jan 2020 18:38:36 -0800 Subject: [PATCH 3/4] Add another test --- .../UseDeclaredVarsMoreThanAssignments.tests.ps1 | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index adfb28a96..b918c22d7 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -67,6 +67,18 @@ function MyFunc2() { $results[0].Extent | Should -Be '$mySecondvar' } + It "Flags when variables are assigned after use" { + $script = @' +Write-Output $moo +$moo = "Hi" +'@ + + $diagnostics = Invoke-ScriptAnalyzer -ScriptDefinition $script + $diagnostics | Should -HaveCount 1 + $diagnostics[0].Extent.StartLineNumber | Should -Be 2 + $diagnostics[0].Extent.Text | Should -BeExactly '$moo' + } + It "Understands that variables set in a child scope are not the same" { $script = @' $flag = $true From bf2068e36ee45b43f57f111ecb763af245e4dfd8 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 15 Jan 2020 20:51:26 -0800 Subject: [PATCH 4/4] Use AST to determine SupportsShouldProcess when an error is thrown --- Rules/UseShouldProcessCorrectly.cs | 96 +++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 23 deletions(-) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index e08ca098a..2138a4113 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -299,48 +299,98 @@ private bool SupportsShouldProcess(string cmdName) return false; } - var cmdInfo = Helper.Instance.GetCommandInfo(cmdName); + CommandInfo cmdInfo = Helper.Instance.GetCommandInfo(cmdName); if (cmdInfo == null) { return false; } - var cmdletInfo = cmdInfo as CmdletInfo; - if (cmdletInfo == null) + switch (cmdInfo) { - // check if it is of functioninfo type - var funcInfo = cmdInfo as FunctionInfo; - if (funcInfo != null - && funcInfo.CmdletBinding - && funcInfo.ScriptBlock != null - && funcInfo.ScriptBlock.Attributes != null) - { - foreach (var attr in funcInfo.ScriptBlock.Attributes) + case CmdletInfo cmdletInfo: + return cmdletInfo.ImplementingType.GetCustomAttribute(inherit: true).SupportsShouldProcess; + + case FunctionInfo functionInfo: + try { - var cmdletBindingAttr = attr as CmdletBindingAttribute; - if (cmdletBindingAttr != null) + if (!functionInfo.CmdletBinding + || functionInfo.ScriptBlock?.Attributes == null) + { + break; + } + + foreach (CmdletBindingAttribute cmdletBindingAttribute in functionInfo.ScriptBlock.Attributes.OfType()) { - return cmdletBindingAttr.SupportsShouldProcess; + return cmdletBindingAttribute.SupportsShouldProcess; + } + } + catch + { + // functionInfo.ScriptBlock.Attributes may throw if it cannot resolve an attribute type + // Instead we fall back to AST analysis + // See: https://github.com/PowerShell/PSScriptAnalyzer/issues/1217 + if (TryGetShouldProcessValueFromAst(functionInfo, out bool hasShouldProcessSet)) + { + return hasShouldProcessSet; } } - } - return false; + break; } - var attributes = cmdletInfo.ImplementingType.GetTypeInfo().GetCustomAttributes( - typeof(System.Management.Automation.CmdletCommonMetadataAttribute), - true); + return false; + } + + /// + /// Attempt to find whether a function has SupportsShouldProcess set based on its AST + /// + /// The function info object referring to the function. + /// True if SupportsShouldProcess is set, false if not. Value is not valid if this method returns false. + /// True if a value for SupportsShouldProcess was found, false otherwise. + private bool TryGetShouldProcessValueFromAst(FunctionInfo functionInfo, out bool hasShouldProcessSet) + { + // Get the body of the function + ScriptBlockAst functionBodyAst = (ScriptBlockAst)functionInfo.ScriptBlock.Ast.Find(ast => ast is ScriptBlockAst, searchNestedScriptBlocks: false); - foreach (var attr in attributes) + // Go through attributes on the parameter block, since this is where [CmdletBinding()] will be + foreach (AttributeAst attributeAst in functionBodyAst.ParamBlock.Attributes) { - var cmdletAttribute = attr as System.Management.Automation.CmdletAttribute; - if (cmdletAttribute != null) + // We're looking for [CmdletBinding()] + if (!String.Equals(attributeAst.TypeName.FullName, "CmdletBinding", StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + foreach (NamedAttributeArgumentAst namedArgumentAst in attributeAst.NamedArguments) { - return cmdletAttribute.SupportsShouldProcess; + // We want [CmdletBinding(SupportsShouldProcess)] + if (!String.Equals(namedArgumentAst.ArgumentName, "SupportsShouldProcess", StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + // [CmdletBinding(SupportsShouldProcess)] is the same as [CmdletBinding(SupportsShouldProcess = $true)] + if (namedArgumentAst.ExpressionOmitted) + { + hasShouldProcessSet = true; + return true; + } + + // Otherwise try to get the value assigned to the parameter, and assume false if value cannot be determined + try + { + hasShouldProcessSet = LanguagePrimitives.IsTrue(namedArgumentAst.Argument.SafeGetValue()); + } + catch + { + hasShouldProcessSet = false; + } + + return true; } } + hasShouldProcessSet = false; return false; }