Skip to content

Fix false negatives for identically named variable in different scopes #613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 7, 2016
Merged

Fix false negatives for identically named variable in different scopes #613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee4cd62
Check for variable usage in same or nested scope
Sep 7, 2016
bf472e4
Add test case for UseDeclaredVarsMoreThanAssignment
Sep 7, 2016
19828fa
Rephrase UseDeclaredVarsMoreThanAssignment description
Sep 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified Rules/Strings.Designer.cs
View file
Open in desktop
Binary file not shown.
56 changes: 28 additions & 28 deletions Rules/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
<!--
Microsoft ResX Schema

Version 2.0
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>
There are any number of "resheader" rows that contain simple

There are any number of "resheader" rows that contain simple
name/value pairs.
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -160,7 +160,7 @@
<value>Cmdlet Verbs</value>
</data>
<data name="UseDeclaredVarsMoreThanAssignmentsDescription" xml:space="preserve">
<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>
<value>Ensure declared variables are used elsewhere in the script and not just during assignment.</value>
</data>
<data name="UseDeclaredVarsMoreThanAssignmentsError" xml:space="preserve">
<value>The variable '{0}' is assigned but never used.</value>
Expand Down
165 changes: 96 additions & 69 deletions Rules/UseDeclaredVarsMoreThanAssignments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//
// Copyright (c) Microsoft Corporation.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
Expand All @@ -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
{
Expand All @@ -37,77 +35,24 @@ public class UseDeclaredVarsMoreThanAssignments : IScriptRule
/// <returns>A List of results from this rule</returns>
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);

IEnumerable<Ast> assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true);
IEnumerable<Ast> varAsts = ast.FindAll(testAst => testAst is VariableExpressionAst, true);
IEnumerable<Ast> varsInAssignment;

Dictionary<string, AssignmentStatementAst> assignments = new Dictionary<string, AssignmentStatementAst>(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;
}
}
}

Expand Down Expand Up @@ -162,10 +107,92 @@ public string GetSourceName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
}
}

}
/// <summary>
/// Checks if a variable is initialized and referenced in either its assignment or children scopes
/// </summary>
/// <param name="scriptBlockAst">Ast of type ScriptBlock</param>
/// <param name="fileName">Name of file containing the ast</param>
/// <returns>An enumerable containing diagnostic records</returns>
private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scriptBlockAst, string fileName)
{
IEnumerable<Ast> assignmentAsts = scriptBlockAst.FindAll(testAst => testAst is AssignmentStatementAst, false);
IEnumerable<Ast> varAsts = scriptBlockAst.FindAll(testAst => testAst is VariableExpressionAst, true);
IEnumerable<Ast> varsInAssignment;

Dictionary<string, AssignmentStatementAst> assignments = new Dictionary<string, AssignmentStatementAst>(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);
}
}
}
}
17 changes: 17 additions & 0 deletions Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down