Skip to content

Do not trigger UseDeclaredVarsMoreThanAssignment for variables being used via Get-Variable #925

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 28 additions & 9 deletions Rules/UseDeclaredVarsMoreThanAssignments.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#endif
using System.Globalization;
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
using System.Linq;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
Expand Down Expand Up @@ -113,7 +114,7 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip
IEnumerable<Ast> varAsts = scriptBlockAst.FindAll(testAst => testAst is VariableExpressionAst, true);
IEnumerable<Ast> varsInAssignment;

Dictionary<string, AssignmentStatementAst> assignments = new Dictionary<string, AssignmentStatementAst>(StringComparer.OrdinalIgnoreCase);
Dictionary<string, AssignmentStatementAst> assignmentsDictionary_OrdinalIgnoreCase = new Dictionary<string, AssignmentStatementAst>(StringComparer.OrdinalIgnoreCase);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain me what the variable name: "assignmentsDictionary_OrdinalIgnoreCase" stands for ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. As per Jim's comment here, I renamed it to make it more obvious that the dictionary is case insensitive (one can see this option only when looking at its constructor). PowerShell is overall case sensitive (except for this special case), therefore this a standard appoach and OrdinalIgnoreCase is used because InvariantCultureIgnoreCase is not fully available in netstandard. About the variable itself: the rule first tries to find all variable assignments and adds them to the dictionary, then it looks for all usages of variables and removes the variable from the dictionary if it gets used. The variables that are left must therefore be unused and PSSA returns then the warnings.


string varKey;
bool inAssignment;
Expand Down Expand Up @@ -148,9 +149,9 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip
{
string variableName = Helper.Instance.VariableNameWithoutScope(assignmentVarAst.VariablePath);

if (!assignments.ContainsKey(variableName))
if (!assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName))
{
assignments.Add(variableName, assignmentAst);
assignmentsDictionary_OrdinalIgnoreCase.Add(variableName, assignmentAst);
}
}
}
Expand All @@ -163,9 +164,9 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip
varKey = Helper.Instance.VariableNameWithoutScope(varAst.VariablePath);
inAssignment = false;

if (assignments.ContainsKey(varKey))
if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(varKey))
{
varsInAssignment = assignments[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true);
varsInAssignment = assignmentsDictionary_OrdinalIgnoreCase[varKey].Left.FindAll(testAst => testAst is VariableExpressionAst, true);

// Checks if this variableAst is part of the logged assignment
foreach (VariableExpressionAst varInAssignment in varsInAssignment)
Expand Down Expand Up @@ -195,23 +196,41 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip

if (!inAssignment)
{
assignments.Remove(varKey);
assignmentsDictionary_OrdinalIgnoreCase.Remove(varKey);
}

// Check if variable belongs to PowerShell built-in variables
if (Helper.Instance.HasSpecialVars(varKey))
{
assignments.Remove(varKey);
assignmentsDictionary_OrdinalIgnoreCase.Remove(varKey);
}
}
}
}

foreach (string key in assignments.Keys)
// Detect usages of Get-Variable
var getVariableCmdletNamesAndAliases = Helper.Instance.CmdletNameAndAliases("Get-Variable");
IEnumerable<Ast> getVariableCommandAsts = scriptBlockAst.FindAll(testAst => testAst is CommandAst commandAst &&
getVariableCmdletNamesAndAliases.Contains(commandAst.GetCommandName(), StringComparer.OrdinalIgnoreCase), true);
foreach (CommandAst getVariableCommandAst in getVariableCommandAsts)
{
var commandElements = getVariableCommandAst.CommandElements.ToList();
// The following extracts the variable name only in the simplest possibe case 'Get-Variable variableName'
if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst)
{
var variableName = constantExpressionAst.Value;
if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName))
{
assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName);
}
}
}

foreach (string key in assignmentsDictionary_OrdinalIgnoreCase.Keys)
{
yield return new DiagnosticRecord(
string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key),
assignments[key].Left.Extent,
assignmentsDictionary_OrdinalIgnoreCase[key].Left.Extent,
GetName(),
DiagnosticSeverity.Warning,
fileName,
Expand Down
5 changes: 5 additions & 0 deletions Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -87,5 +87,10 @@ function MyFunc2() {
$results = Invoke-ScriptAnalyzer -ScriptDefinition '$env:foo = 1; function foo(){ $env:bar = 42 }'
$results.Count | Should -Be 0
}

It "Using a variable via 'Get-Variable' does not trigger a warning" {
$noViolations = Invoke-ScriptAnalyzer -ScriptDefinition '$a=4; get-variable a'
$noViolations.Count | Should -Be 0
}
}
}