Skip to content
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

Enhance UseDeclaredVarsMoreThanAssignments to detect also take into account the usage of Get-Variable with an array of variables and usage of named parameter -Name #1310

Merged
merged 4 commits into from
Aug 19, 2019
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
66 changes: 50 additions & 16 deletions Rules/UseDeclaredVarsMoreThanAssignments.cs
Original file line number Diff line number Diff line change
Expand Up @@ -208,33 +208,67 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip
}
}

// Detect usages of Get-Variable
AnalyzeGetVariableCommands(scriptBlockAst, assignmentsDictionary_OrdinalIgnoreCase);

foreach (string key in assignmentsDictionary_OrdinalIgnoreCase.Keys)
{
yield return new DiagnosticRecord(
string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key),
assignmentsDictionary_OrdinalIgnoreCase[key].Left.Extent,
GetName(),
DiagnosticSeverity.Warning,
fileName,
key);
}
}

/// <summary>
/// Detects variables retrieved by usage of Get-Variable and remove those
/// variables from the entries in <paramref name="assignmentsDictionary_OrdinalIgnoreCase"/>.
/// </summary>
/// <param name="scriptBlockAst"></param>
/// <param name="assignmentsDictionary_OrdinalIgnoreCase"></param>
private void AnalyzeGetVariableCommands(
ScriptBlockAst scriptBlockAst,
Dictionary<string, AssignmentStatementAst> assignmentsDictionary_OrdinalIgnoreCase)
{
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)
// The following extracts the variable name(s) only in the simplest possible usage of Get-Variable.
// Usage of a named parameter and an array of variables is accounted for though.
if (commandElements.Count < 2 || commandElements.Count > 3) { continue; }

var commandElementAstOfVariableName = commandElements[commandElements.Count - 1];
if (commandElements.Count == 3)
{
var variableName = constantExpressionAst.Value;
if (assignmentsDictionary_OrdinalIgnoreCase.ContainsKey(variableName))
if (!(commandElements[1] is CommandParameterAst commandParameterAst)) { continue; }
// Check if the named parameter -Name is used (PowerShell does not need the full
// parameter name and there is no other parameter of Get-Variable starting with n).
if (!commandParameterAst.ParameterName.StartsWith("n", StringComparison.OrdinalIgnoreCase))
{
assignmentsDictionary_OrdinalIgnoreCase.Remove(variableName);
continue;
}
}
}

foreach (string key in assignmentsDictionary_OrdinalIgnoreCase.Keys)
{
yield return new DiagnosticRecord(
string.Format(CultureInfo.CurrentCulture, Strings.UseDeclaredVarsMoreThanAssignmentsError, key),
assignmentsDictionary_OrdinalIgnoreCase[key].Left.Extent,
GetName(),
DiagnosticSeverity.Warning,
fileName,
key);
if (commandElementAstOfVariableName is StringConstantExpressionAst constantExpressionAst)
{
assignmentsDictionary_OrdinalIgnoreCase.Remove(constantExpressionAst.Value);
continue;
}

if (!(commandElementAstOfVariableName is ArrayLiteralAst arrayLiteralAst)) { continue; }
foreach (var expressionAst in arrayLiteralAst.Elements)
{
if (expressionAst is StringConstantExpressionAst constantExpressionAstOfArray)
{
assignmentsDictionary_OrdinalIgnoreCase.Remove(constantExpressionAstOfArray.Value);
}
}
}
}
}
Expand Down
40 changes: 36 additions & 4 deletions Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function MyFunc2() {
It "returns no violations" {
$noViolations.Count | Should -Be 0
}

It "Does not flag += operator" {
$results = Invoke-ScriptAnalyzer -ScriptDefinition '$array=@(); $list | ForEach-Object { $array += $c }' | Where-Object { $_.RuleName -eq $violationName }
$results.Count | Should -Be 0
Expand All @@ -88,9 +88,41 @@ function MyFunc2() {
$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
It "No warning when using 'Get-Variable' with variables declaration '<DeclareVariables>' and command parameter <GetVariableCommandParameter>" -TestCases @(
@{
DeclareVariables = '$a = 1'; GetVariableCommandParameter = 'a';
}
@{
DeclareVariables = '$a = 1'; GetVariableCommandParameter = '-Name a';
}
@{
DeclareVariables = '$a = 1'; GetVariableCommandParameter = '-n a';
}
@{
DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = 'a,b'
}
@{
DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = '-Name a,b'
}
@{
DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = '-n a,b'
}
@{
DeclareVariables = '$a = 1; $b = 2'; GetVariableCommandParameter = 'A,B'
}
) {
Param(
$DeclareVariables,
$GetVariableCommandParameter
)
$scriptDefinition = "$DeclareVariables; Get-Variable $GetVariableCommandParameter"
$noViolations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition
$noViolations.Count | Should -Be 0 -Because $scriptDefinition
}

It "Does not misinterpret switch parameter of Get-Variable as variable" {
$scriptDefinition = '$ArbitrarySwitchParameter = 1; Get-Variable -ArbitrarySwitchParameter'
(Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition).Count | Should -Be 1 -Because $scriptDefinition
}
}
}