From 98d0daa2ea4de0b8d91fb1c1da07b1f334af87fb Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 15 Nov 2016 20:09:29 -0800 Subject: [PATCH 1/4] UseDeclareVarsMoreThanAssign rule checks nested variables Consider the following case: ```powershell $a = 1 1..5 | ForEach-Object {$a = 2} $a ``` In this case, the rule would flag the assignement in the `ForEach-Object` block. This commit fixes this issue. --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 114 ++++++++++++++++---- 1 file changed, 95 insertions(+), 19 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index f2731474a..c3c523c15 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -27,6 +27,16 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class UseDeclaredVarsMoreThanAssignments : IScriptRule { +// TODO arrange private and public functions +// TODO Code formatting +// TODO inline documentation + private Dictionary> scriptBlockAssignmentMap; + private Dictionary> scriptblockVariableUsageMap; + private Dictionary scriptBlockAstParentMap; + + private Ast ast; + private string fileName; + /// /// AnalyzeScript: Analyzes the ast to check that variables are used in more than just there assignment. /// @@ -46,16 +56,69 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) yield break; } + scriptBlockAssignmentMap = new Dictionary>(); + scriptblockVariableUsageMap = new Dictionary>(); + scriptBlockAstParentMap = new Dictionary(); + this.ast = ast; + this.fileName = fileName; foreach (var scriptBlockAst in scriptBlockAsts) { var sbAst = scriptBlockAst as ScriptBlockAst; - foreach (var diagnosticRecord in AnalyzeScriptBlockAst(sbAst, fileName)) + AnalyzeScriptBlockAst(sbAst, fileName); + } + + foreach (var diagnosticRecord in GetViolations()) + { + yield return diagnosticRecord; + } + } + + private IEnumerable GetViolations() + { + // throw violation if + // if a variable in a scope is unused + // AND + // if it is unused in the parent scopes + + foreach (var sbAst in scriptblockVariableUsageMap.Keys) + { + foreach (var variable in scriptblockVariableUsageMap[sbAst].Keys) { - yield return diagnosticRecord; + if (!DoesScriptBlockUseVariable(sbAst, variable)) + { + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, variable), + scriptBlockAssignmentMap[sbAst][variable].Left.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + variable); + + } } } } + private bool DoesScriptBlockUseVariable(ScriptBlockAst scriptBlockAst, string variable) + { + if (scriptblockVariableUsageMap[scriptBlockAst].ContainsKey(variable)) + { + if (!scriptblockVariableUsageMap[scriptBlockAst][variable]) + { + if (scriptBlockAstParentMap[scriptBlockAst] == null) + { + return false; + } + + return DoesScriptBlockUseVariable(scriptBlockAstParentMap[scriptBlockAst], variable); + } + + return true; + } + + return false; + } + /// /// GetName: Retrieves the name of this rule. /// @@ -114,22 +177,24 @@ public string GetSourceName() /// Ast of type ScriptBlock /// Name of file containing the ast /// An enumerable containing diagnostic records - private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileName) + private void AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileName) { IEnumerable assignmentAsts = scriptBlockAst.FindAll(testAst => testAst is AssignmentStatementAst, false); IEnumerable varAsts = scriptBlockAst.FindAll(testAst => testAst is VariableExpressionAst, true); IEnumerable varsInAssignment; Dictionary assignments = new Dictionary(StringComparer.OrdinalIgnoreCase); + Dictionary isVariableUsed = new Dictionary(StringComparer.OrdinalIgnoreCase); string varKey; bool inAssignment; if (assignmentAsts == null) { - yield break; + return; } + // TODO Remove key removal foreach (AssignmentStatementAst assignmentAst in assignmentAsts) { // Only checks for the case where lhs is a variable. Ignore things like $foo.property @@ -147,6 +212,7 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip if (!assignments.ContainsKey(variableName)) { assignments.Add(variableName, assignmentAst); + isVariableUsed.Add(variableName, false); } } } @@ -163,35 +229,45 @@ private IEnumerable AnalyzeScriptBlockAst(ScriptBlockAst scrip { varsInAssignment = assignments[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true); - // Checks if this variableAst is part of the logged assignment + foreach (VariableExpressionAst varInAssignment in varsInAssignment) { inAssignment |= varInAssignment.Equals(varAst); } - if (!inAssignment) - { - assignments.Remove(varKey); - } - // Check if variable belongs to PowerShell built-in variables - if (Helper.Instance.HasSpecialVars(varKey)) + // Checks if this variableAst is part of the logged assignment + if (!inAssignment || Helper.Instance.HasSpecialVars(varKey)) { assignments.Remove(varKey); + isVariableUsed[varKey] = true; } } } } - foreach (string key in assignments.Keys) + scriptBlockAssignmentMap[scriptBlockAst] = assignments; + scriptblockVariableUsageMap[scriptBlockAst] = isVariableUsed; + scriptBlockAstParentMap[scriptBlockAst] = GetScriptBlockAstParent(scriptBlockAst); + } + + private ScriptBlockAst GetScriptBlockAstParent(Ast scriptBlockAst) + { + if (scriptBlockAst == this.ast) + { + return null; + } + else { - yield return new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key), - assignments[key].Left.Extent, - GetName(), - DiagnosticSeverity.Warning, - fileName, - key); + var parent = scriptBlockAst.Parent as ScriptBlockAst; + if (parent == null) + { + return GetScriptBlockAstParent(scriptBlockAst.Parent); + } + else + { + return parent; + } } } } From 3f3dbb77ccb06f78ab5e389f4cd4daa6268975be Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 16 Nov 2016 10:08:58 -0800 Subject: [PATCH 2/4] Remove key removal in UseDeclaredVarsMoreThanAssign --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 128 +++++++++----------- 1 file changed, 60 insertions(+), 68 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index c3c523c15..6f8a1c595 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -27,13 +27,10 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class UseDeclaredVarsMoreThanAssignments : IScriptRule { -// TODO arrange private and public functions // TODO Code formatting -// TODO inline documentation private Dictionary> scriptBlockAssignmentMap; private Dictionary> scriptblockVariableUsageMap; private Dictionary scriptBlockAstParentMap; - private Ast ast; private string fileName; @@ -73,52 +70,6 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } } - private IEnumerable GetViolations() - { - // throw violation if - // if a variable in a scope is unused - // AND - // if it is unused in the parent scopes - - foreach (var sbAst in scriptblockVariableUsageMap.Keys) - { - foreach (var variable in scriptblockVariableUsageMap[sbAst].Keys) - { - if (!DoesScriptBlockUseVariable(sbAst, variable)) - { - yield return new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, variable), - scriptBlockAssignmentMap[sbAst][variable].Left.Extent, - GetName(), - DiagnosticSeverity.Warning, - fileName, - variable); - - } - } - } - } - - private bool DoesScriptBlockUseVariable(ScriptBlockAst scriptBlockAst, string variable) - { - if (scriptblockVariableUsageMap[scriptBlockAst].ContainsKey(variable)) - { - if (!scriptblockVariableUsageMap[scriptBlockAst][variable]) - { - if (scriptBlockAstParentMap[scriptBlockAst] == null) - { - return false; - } - - return DoesScriptBlockUseVariable(scriptBlockAstParentMap[scriptBlockAst], variable); - } - - return true; - } - - return false; - } - /// /// GetName: Retrieves the name of this rule. /// @@ -181,11 +132,8 @@ private void AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileNam { IEnumerable assignmentAsts = scriptBlockAst.FindAll(testAst => testAst is AssignmentStatementAst, false); IEnumerable varAsts = scriptBlockAst.FindAll(testAst => testAst is VariableExpressionAst, true); - IEnumerable varsInAssignment; - Dictionary assignments = new Dictionary(StringComparer.OrdinalIgnoreCase); Dictionary isVariableUsed = new Dictionary(StringComparer.OrdinalIgnoreCase); - string varKey; bool inAssignment; @@ -194,7 +142,6 @@ private void AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileNam return; } - // TODO Remove key removal foreach (AssignmentStatementAst assignmentAst in assignmentAsts) { // Only checks for the case where lhs is a variable. Ignore things like $foo.property @@ -227,19 +174,13 @@ private void AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileNam if (assignments.ContainsKey(varKey)) { - varsInAssignment = assignments[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true); - - - foreach (VariableExpressionAst varInAssignment in varsInAssignment) - { - inAssignment |= varInAssignment.Equals(varAst); - } + // Check if varAst is part of an AssignmentStatementAst + inAssignment = varAst.Parent is AssignmentStatementAst; // Check if variable belongs to PowerShell built-in variables // Checks if this variableAst is part of the logged assignment if (!inAssignment || Helper.Instance.HasSpecialVars(varKey)) { - assignments.Remove(varKey); isVariableUsed[varKey] = true; } } @@ -251,24 +192,75 @@ private void AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileNam scriptBlockAstParentMap[scriptBlockAst] = GetScriptBlockAstParent(scriptBlockAst); } + /// + /// Gets the first upstream node away from the input argument that is of type ScriptBlockAst + /// + /// Ast + /// Null if the input argument's Parent is null + /// or if Parent is ast, the input to AnalyzeAst private ScriptBlockAst GetScriptBlockAstParent(Ast scriptBlockAst) { - if (scriptBlockAst == this.ast) + if (scriptBlockAst == this.ast + || scriptBlockAst.Parent == null) { return null; } - else + + var parent = scriptBlockAst.Parent as ScriptBlockAst; + if (parent == null) + { + return GetScriptBlockAstParent(scriptBlockAst.Parent); + } + + return parent; + } + + /// + /// Returns the violations in the given ast + /// + private IEnumerable GetViolations() + { + foreach (var sbAst in scriptblockVariableUsageMap.Keys) { - var parent = scriptBlockAst.Parent as ScriptBlockAst; - if (parent == null) + foreach (var variable in scriptblockVariableUsageMap[sbAst].Keys) { - return GetScriptBlockAstParent(scriptBlockAst.Parent); + if (!DoesScriptBlockUseVariable(sbAst, variable)) + { + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, variable), + scriptBlockAssignmentMap[sbAst][variable].Left.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + variable); + } } - else + } + } + + /// + /// Returns true if the input variable argument is used more than just declaration in + /// the input scriptblock or in its parent scriptblock, otherwise returns false + /// + private bool DoesScriptBlockUseVariable(ScriptBlockAst scriptBlockAst, string variable) + { + if (scriptblockVariableUsageMap[scriptBlockAst].ContainsKey(variable)) + { + if (!scriptblockVariableUsageMap[scriptBlockAst][variable]) { - return parent; + if (scriptBlockAstParentMap[scriptBlockAst] == null) + { + return false; + } + + return DoesScriptBlockUseVariable(scriptBlockAstParentMap[scriptBlockAst], variable); } + + return true; } + + return false; } + } } From 9f7807d47eb1c877d91e1dc0837724541b03554d Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 16 Nov 2016 15:20:58 -0800 Subject: [PATCH 3/4] Update UseDeclaredVarsMoreThanAssignments tests --- ...eDeclaredVarsMoreThanAssignments.tests.ps1 | 54 +++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 4f94c35d3..2ce275973 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -9,6 +9,17 @@ $violationName = "PSUseDeclaredVarsMoreThanAssignments" $violations = Invoke-ScriptAnalyzer $directory\UseDeclaredVarsMoreThanAssignments.ps1 | Where-Object {$_.RuleName -eq $violationName} $noViolations = Invoke-ScriptAnalyzer $directory\UseDeclaredVarsMoreThanAssignmentsNoViolations.ps1 | Where-Object {$_.RuleName -eq $violationName} +Function Test-UseDeclaredVarsMoreThanAssignments +{ + param( + [string] $targetScript, + [int] $expectedNumViolations + ) + Invoke-ScriptAnalyzer -ScriptDefinition $targetScript -IncludeRule $violationName | ` + Get-Count | ` + Should Be $expectedNumViolations +} + Describe "UseDeclaredVarsMoreThanAssignments" { Context "When there are violations" { It "has 2 use declared vars more than assignments violations" { @@ -33,21 +44,46 @@ function MyFunc2() { $a + $a } '@ - Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName | ` - Get-Count | ` - Should Be 1 + Test-UseDeclaredVarsMoreThanAssignments $target 1 + } + + It "flags variables assigned in downstream scopes" { +$target = @' +function Get-Directory() { + $a = 1 + 1..10 | ForEach-Object { $a = $_ } +} +'@ + Test-UseDeclaredVarsMoreThanAssignments $target 2 } It "does not flag `$InformationPreference variable" { - Invoke-ScriptAnalyzer -ScriptDefinition '$InformationPreference=Stop' -IncludeRule $violationName | ` - Get-Count | ` - Should Be 0 + Test-UseDeclaredVarsMoreThanAssignments '$InformationPreference=Stop' 0 } It "does not flag `$PSModuleAutoLoadingPreference variable" { - Invoke-ScriptAnalyzer -ScriptDefinition '$PSModuleAutoLoadingPreference=None' -IncludeRule $violationName | ` - Get-Count | ` - Should Be 0 + Test-UseDeclaredVarsMoreThanAssignments '$PSModuleAutoLoadingPreference=None' 0 + } + + It "does not flags variables used in downstream scopes" { +$target = @' +function Get-Directory() { + $a = 1 + 1..10 | ForEach-Object { Write-Output $a } +} +'@ + Test-UseDeclaredVarsMoreThanAssignments $target 0 + } + + It "does not flag variables assigned in downstream scope but used in parent scope" { +$target = @' +function Get-Directory() { + $a = 1 + 1..10 | ForEach-Object { $a = $_ } + $a +} +'@ + Test-UseDeclaredVarsMoreThanAssignments $target 0 } } From eb0b11a447cb6f0d2f6533b75ae8c249026b93bb Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 17 Nov 2016 14:10:16 -0800 Subject: [PATCH 4/4] Move tests in UseDeclaredVarsMoreThanAssignment rule --- Rules/UseDeclaredVarsMoreThanAssignments.cs | 1 - .../UseDeclaredVarsMoreThanAssignments.tests.ps1 | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 6f8a1c595..ca48dc087 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -27,7 +27,6 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class UseDeclaredVarsMoreThanAssignments : IScriptRule { -// TODO Code formatting private Dictionary> scriptBlockAssignmentMap; private Dictionary> scriptblockVariableUsageMap; private Dictionary scriptBlockAstParentMap; diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 2ce275973..061e4d470 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -56,6 +56,12 @@ function Get-Directory() { '@ Test-UseDeclaredVarsMoreThanAssignments $target 2 } + } + + Context "When there are no violations" { + It "returns no violations" { + $noViolations.Count | Should Be 0 + } It "does not flag `$InformationPreference variable" { Test-UseDeclaredVarsMoreThanAssignments '$InformationPreference=Stop' 0 @@ -86,10 +92,4 @@ function Get-Directory() { Test-UseDeclaredVarsMoreThanAssignments $target 0 } } - - Context "When there are no violations" { - It "returns no violations" { - $noViolations.Count | Should Be 0 - } - } } \ No newline at end of file