Skip to content

Commit 1d6718c

Browse files
author
Kapil Borle
authored
Merge pull request #613 from PowerShell/FixUseDeclaredVarsMoreThanAssgn
Fix false negatives for identically named variable in different scopes
2 parents 233193e + 19828fa commit 1d6718c

4 files changed

+141
-97
lines changed

‎Rules/Strings.Designer.cs

-97.5 KB
Binary file not shown.

‎Rules/Strings.resx

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
<?xml version="1.0" encoding="utf-8"?>
22
<root>
3-
<!--
4-
Microsoft ResX Schema
5-
3+
<!--
4+
Microsoft ResX Schema
5+
66
Version 2.0
7-
8-
The primary goals of this format is to allow a simple XML format
9-
that is mostly human readable. The generation and parsing of the
10-
various data types are done through the TypeConverter classes
7+
8+
The primary goals of this format is to allow a simple XML format
9+
that is mostly human readable. The generation and parsing of the
10+
various data types are done through the TypeConverter classes
1111
associated with the data types.
12-
12+
1313
Example:
14-
14+
1515
... ado.net/XML headers & schema ...
1616
<resheader name="resmimetype">text/microsoft-resx</resheader>
1717
<resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
2626
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
2727
<comment>This is a comment</comment>
2828
</data>
29-
30-
There are any number of "resheader" rows that contain simple
29+
30+
There are any number of "resheader" rows that contain simple
3131
name/value pairs.
32-
33-
Each data row contains a name, and value. The row also contains a
34-
type or mimetype. Type corresponds to a .NET class that support
35-
text/value conversion through the TypeConverter architecture.
36-
Classes that don't support this are serialized and stored with the
32+
33+
Each data row contains a name, and value. The row also contains a
34+
type or mimetype. Type corresponds to a .NET class that support
35+
text/value conversion through the TypeConverter architecture.
36+
Classes that don't support this are serialized and stored with the
3737
mimetype set.
38-
39-
The mimetype is used for serialized objects, and tells the
40-
ResXResourceReader how to depersist the object. This is currently not
38+
39+
The mimetype is used for serialized objects, and tells the
40+
ResXResourceReader how to depersist the object. This is currently not
4141
extensible. For a given mimetype the value must be set accordingly:
42-
43-
Note - application/x-microsoft.net.object.binary.base64 is the format
44-
that the ResXResourceWriter will generate, however the reader can
42+
43+
Note - application/x-microsoft.net.object.binary.base64 is the format
44+
that the ResXResourceWriter will generate, however the reader can
4545
read any of the formats listed below.
46-
46+
4747
mimetype: application/x-microsoft.net.object.binary.base64
48-
value : The object must be serialized with
48+
value : The object must be serialized with
4949
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
5050
: and then encoded with base64 encoding.
51-
51+
5252
mimetype: application/x-microsoft.net.object.soap.base64
53-
value : The object must be serialized with
53+
value : The object must be serialized with
5454
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
5555
: and then encoded with base64 encoding.
5656
5757
mimetype: application/x-microsoft.net.object.bytearray.base64
58-
value : The object must be serialized into a byte array
58+
value : The object must be serialized into a byte array
5959
: using a System.ComponentModel.TypeConverter
6060
: and then encoded with base64 encoding.
6161
-->
@@ -160,7 +160,7 @@
160160
<value>Cmdlet Verbs</value>
161161
</data>
162162
<data name="UseDeclaredVarsMoreThanAssignmentsDescription" xml:space="preserve">
163-
<value>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.</value>
163+
<value>Ensure declared variables are used elsewhere in the script and not just during assignment.</value>
164164
</data>
165165
<data name="UseDeclaredVarsMoreThanAssignmentsError" xml:space="preserve">
166166
<value>The variable '{0}' is assigned but never used.</value>

‎Rules/UseDeclaredVarsMoreThanAssignments.cs

Lines changed: 96 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
//
21
// Copyright (c) Microsoft Corporation.
32
//
43
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
@@ -8,16 +7,15 @@
87
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
98
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
109
// THE SOFTWARE.
11-
//
1210

1311
using System;
1412
using System.Collections.Generic;
1513
using System.Management.Automation.Language;
16-
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
1714
#if !CORECLR
1815
using System.ComponentModel.Composition;
1916
#endif
2017
using System.Globalization;
18+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
2119

2220
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
2321
{
@@ -37,77 +35,24 @@ public class UseDeclaredVarsMoreThanAssignments : IScriptRule
3735
/// <returns>A List of results from this rule</returns>
3836
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
3937
{
40-
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
41-
42-
IEnumerable<Ast> assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true);
43-
IEnumerable<Ast> varAsts = ast.FindAll(testAst => testAst is VariableExpressionAst, true);
44-
IEnumerable<Ast> varsInAssignment;
45-
46-
Dictionary<string, AssignmentStatementAst> assignments = new Dictionary<string, AssignmentStatementAst>(StringComparer.OrdinalIgnoreCase);
47-
48-
string varKey;
49-
bool inAssignment;
50-
51-
if (assignmentAsts != null)
38+
if (ast == null)
5239
{
53-
foreach (AssignmentStatementAst assignmentAst in assignmentAsts)
54-
{
55-
// Only checks for the case where lhs is a variable. Ignore things like $foo.property
56-
VariableExpressionAst assignmentVarAst = assignmentAst.Left as VariableExpressionAst;
57-
58-
if (assignmentVarAst != null)
59-
{
60-
//Ignore if variable is global or environment variable or scope is function
61-
if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst, ast)
62-
&& !assignmentVarAst.VariablePath.IsScript
63-
&& !string.Equals(assignmentVarAst.VariablePath.DriveName, "function", StringComparison.OrdinalIgnoreCase))
64-
{
65-
66-
string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath);
67-
68-
if (!assignments.ContainsKey(variableName))
69-
{
70-
assignments.Add(variableName, assignmentAst);
71-
}
72-
}
73-
}
74-
}
40+
throw new ArgumentNullException(Strings.NullAstErrorMessage);
7541
}
7642

77-
if (varAsts != null)
43+
var scriptBlockAsts = ast.FindAll(x => x is ScriptBlockAst, true);
44+
if (scriptBlockAsts == null)
7845
{
79-
foreach (VariableExpressionAst varAst in varAsts)
80-
{
81-
varKey = Helper.Instance.VariableNameWithoutScope(varAst.VariablePath);
82-
inAssignment = false;
83-
84-
if (assignments.ContainsKey(varKey))
85-
{
86-
varsInAssignment = assignments[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true); ;
87-
88-
//Checks if this variableAst is part of the logged assignment
89-
foreach (VariableExpressionAst varInAssignment in varsInAssignment)
90-
{
91-
inAssignment |= varInAssignment.Equals(varAst);
92-
}
93-
94-
if (!inAssignment)
95-
{
96-
assignments.Remove(varKey);
97-
}
98-
//Check if variable belongs to PowerShell built-in variables
99-
if (Helper.Instance.HasSpecialVars(varKey))
100-
{
101-
assignments.Remove(varKey);
102-
}
103-
}
104-
}
46+
yield break;
10547
}
10648

107-
foreach (string key in assignments.Keys)
49+
foreach (var scriptBlockAst in scriptBlockAsts)
10850
{
109-
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key),
110-
assignments[key].Left.Extent, GetName(), DiagnosticSeverity.Warning, fileName, key);
51+
var sbAst = scriptBlockAst as ScriptBlockAst;
52+
foreach (var diagnosticRecord in AnalyzeScriptBlockAst(sbAst, fileName))
53+
{
54+
yield return diagnosticRecord;
55+
}
11156
}
11257
}
11358

@@ -162,10 +107,92 @@ public string GetSourceName()
162107
{
163108
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
164109
}
165-
}
166110

167-
}
111+
/// <summary>
112+
/// Checks if a variable is initialized and referenced in either its assignment or children scopes
113+
/// </summary>
114+
/// <param name="scriptBlockAst">Ast of type ScriptBlock</param>
115+
/// <param name="fileName">Name of file containing the ast</param>
116+
/// <returns>An enumerable containing diagnostic records</returns>
117+
private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileName)
118+
{
119+
IEnumerable<Ast> assignmentAsts = scriptBlockAst.FindAll(testAst => testAst is AssignmentStatementAst, false);
120+
IEnumerable<Ast> varAsts = scriptBlockAst.FindAll(testAst => testAst is VariableExpressionAst, true);
121+
IEnumerable<Ast> varsInAssignment;
122+
123+
Dictionary<string, AssignmentStatementAst> assignments = new Dictionary<string, AssignmentStatementAst>(StringComparer.OrdinalIgnoreCase);
168124

125+
string varKey;
126+
bool inAssignment;
127+
128+
if (assignmentAsts == null)
129+
{
130+
yield break;
131+
}
132+
133+
foreach (AssignmentStatementAst assignmentAst in assignmentAsts)
134+
{
135+
// Only checks for the case where lhs is a variable. Ignore things like $foo.property
136+
VariableExpressionAst assignmentVarAst = assignmentAst.Left as VariableExpressionAst;
137+
138+
if (assignmentVarAst != null)
139+
{
140+
// Ignore if variable is global or environment variable or scope is function
141+
if (!Helper.Instance.IsVariableGlobalOrEnvironment(assignmentVarAst, scriptBlockAst)
142+
&& !assignmentVarAst.VariablePath.IsScript
143+
&& !string.Equals(assignmentVarAst.VariablePath.DriveName, "function", StringComparison.OrdinalIgnoreCase))
144+
{
145+
string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath);
146+
147+
if (!assignments.ContainsKey(variableName))
148+
{
149+
assignments.Add(variableName, assignmentAst);
150+
}
151+
}
152+
}
153+
}
154+
155+
if (varAsts != null)
156+
{
157+
foreach (VariableExpressionAst varAst in varAsts)
158+
{
159+
varKey = Helper.Instance.VariableNameWithoutScope(varAst.VariablePath);
160+
inAssignment = false;
169161

162+
if (assignments.ContainsKey(varKey))
163+
{
164+
varsInAssignment = assignments[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true);
170165

166+
// Checks if this variableAst is part of the logged assignment
167+
foreach (VariableExpressionAst varInAssignment in varsInAssignment)
168+
{
169+
inAssignment |= varInAssignment.Equals(varAst);
170+
}
171171

172+
if (!inAssignment)
173+
{
174+
assignments.Remove(varKey);
175+
}
176+
177+
// Check if variable belongs to PowerShell built-in variables
178+
if (Helper.Instance.HasSpecialVars(varKey))
179+
{
180+
assignments.Remove(varKey);
181+
}
182+
}
183+
}
184+
}
185+
186+
foreach (string key in assignments.Keys)
187+
{
188+
yield return new DiagnosticRecord(
189+
string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key),
190+
assignments[key].Left.Extent,
191+
GetName(),
192+
DiagnosticSeverity.Warning,
193+
fileName,
194+
key);
195+
}
196+
}
197+
}
198+
}

‎Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,23 @@ Describe "UseDeclaredVarsMoreThanAssignments" {
1515
$violations[1].Message | Should Match $violationMessage
1616
}
1717

18+
It "flags the variable in the correct scope" {
19+
$target = @'
20+
function MyFunc1() {
21+
$a = 1
22+
$b = 1
23+
$a + $b
24+
}
25+
26+
function MyFunc2() {
27+
$a = 1
28+
$b = 1
29+
$a + $a
30+
}
31+
'@
32+
$local:violations = Invoke-ScriptAnalyzer -ScriptDefinition $target -IncludeRule $violationName
33+
$local:violations.Count | Should Be 1
34+
}
1835
}
1936

2037
Context "When there are no violations" {

0 commit comments

Comments
 (0)