diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index c09955434..ffab47fe3 100644 Binary files a/Rules/Strings.Designer.cs and b/Rules/Strings.Designer.cs differ diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 7644356bf..c4ed5c240 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1,17 +1,17 @@  - @@ -160,7 +160,7 @@ Cmdlet Verbs - Checks that variables are used in more than just their assignment. Generally this is a red flag that a variable is not needed. This rule does not check if the assignment and usage are in the same function. + Ensure declared variables are used elsewhere in the script and not just during assignment. The variable '{0}' is assigned but never used. diff --git a/Rules/UseDeclaredVarsMoreThanAssignments.cs b/Rules/UseDeclaredVarsMoreThanAssignments.cs index 4243a05eb..7b4d11df0 100644 --- a/Rules/UseDeclaredVarsMoreThanAssignments.cs +++ b/Rules/UseDeclaredVarsMoreThanAssignments.cs @@ -1,4 +1,3 @@ -// // Copyright (c) Microsoft Corporation. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR @@ -8,16 +7,15 @@ // 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. -// using System; using System.Collections.Generic; using System.Management.Automation.Language; -using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; #if !CORECLR using System.ComponentModel.Composition; #endif using System.Globalization; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -37,77 +35,24 @@ public class UseDeclaredVarsMoreThanAssignments : IScriptRule /// A List of results from this rule public IEnumerable AnalyzeScript(Ast ast, string fileName) { - if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - - IEnumerable assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true); - IEnumerable varAsts = ast.FindAll(testAst => testAst is VariableExpressionAst, true); - IEnumerable varsInAssignment; - - Dictionary assignments = new Dictionary(StringComparer.OrdinalIgnoreCase); - - string varKey; - bool inAssignment; - - if (assignmentAsts != null) + if (ast == null) { - foreach (AssignmentStatementAst assignmentAst in assignmentAsts) - { - // Only checks for the case where lhs is a variable. Ignore things like $foo.property - VariableExpressionAst assignmentVarAst = assignmentAst.Left as VariableExpressionAst; - - if (assignmentVarAst != null) - { - //Ignore if variable is global or environment variable or scope is function - if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst, ast) - && !assignmentVarAst.VariablePath.IsScript - && !string.Equals(assignmentVarAst.VariablePath.DriveName, "function", StringComparison.OrdinalIgnoreCase)) - { - - string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath); - - if (!assignments.ContainsKey(variableName)) - { - assignments.Add(variableName, assignmentAst); - } - } - } - } + throw new ArgumentNullException(Strings.NullAstErrorMessage); } - if (varAsts != null) + var scriptBlockAsts = ast.FindAll(x => x is ScriptBlockAst, true); + if (scriptBlockAsts == null) { - foreach (VariableExpressionAst varAst in varAsts) - { - varKey = Helper.Instance.VariableNameWithoutScope(varAst.VariablePath); - inAssignment = false; - - if (assignments.ContainsKey(varKey)) - { - 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)) - { - assignments.Remove(varKey); - } - } - } + yield break; } - foreach (string key in assignments.Keys) + foreach (var scriptBlockAst in scriptBlockAsts) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key), - assignments[key].Left.Extent, GetName(), DiagnosticSeverity.Warning, fileName, key); + var sbAst = scriptBlockAst as ScriptBlockAst; + foreach (var diagnosticRecord in AnalyzeScriptBlockAst(sbAst, fileName)) + { + yield return diagnosticRecord; + } } } @@ -162,10 +107,92 @@ public string GetSourceName() { return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); } - } -} + /// + /// Checks if a variable is initialized and referenced in either its assignment or children scopes + /// + /// Ast of type ScriptBlock + /// Name of file containing the ast + /// An enumerable containing diagnostic records + private IEnumerable 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); + string varKey; + bool inAssignment; + + if (assignmentAsts == null) + { + yield break; + } + + foreach (AssignmentStatementAst assignmentAst in assignmentAsts) + { + // Only checks for the case where lhs is a variable. Ignore things like $foo.property + VariableExpressionAst assignmentVarAst = assignmentAst.Left as VariableExpressionAst; + + if (assignmentVarAst != null) + { + // Ignore if variable is global or environment variable or scope is function + if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst, scriptBlockAst) + && !assignmentVarAst.VariablePath.IsScript + && !string.Equals(assignmentVarAst.VariablePath.DriveName, "function", StringComparison.OrdinalIgnoreCase)) + { + string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath); + + if (!assignments.ContainsKey(variableName)) + { + assignments.Add(variableName, assignmentAst); + } + } + } + } + + if (varAsts != null) + { + foreach (VariableExpressionAst varAst in varAsts) + { + varKey = Helper.Instance.VariableNameWithoutScope(varAst.VariablePath); + inAssignment = false; + if (assignments.ContainsKey(varKey)) + { + 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)) + { + assignments.Remove(varKey); + } + } + } + } + + foreach (string key in assignments.Keys) + { + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key), + assignments[key].Left.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + key); + } + } + } +} diff --git a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 index 78212e392..82ad40d44 100644 --- a/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 +++ b/Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 @@ -15,6 +15,23 @@ Describe "UseDeclaredVarsMoreThanAssignments" { $violations[1].Message | Should Match $violationMessage } + It "flags the variable in the correct scope" { + $target = @' +function MyFunc1() { + $a = 1 + $b = 1 + $a + $b +} + +function MyFunc2() { + $a = 1 + $b = 1 + $a + $a +} +'@ + $local:violations = Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName + $local:violations.Count | Should Be 1 + } } Context "When there are no violations" {