Do not trigger UseDeclaredVarsMoreThanAssignment for variables being used via Get-Variable #925
Conversation
| @@ -73,5 +73,10 @@ function MyFunc2() { | |||
| It "returns no violations" { | |||
| $noViolations.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' | Where-Object { $_.RuleName -eq $violationName } | |||
There was a problem hiding this comment.
is the where-object needed?
could this just be
Invoke-ScriptAnalyzer -ScriptDefinition '$a=4;Get-Variable a' | Should -BeNullOrEmpty
There was a problem hiding this comment.
I removed the Where-Object now since this seems to be your preferred approach.
| if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst) | ||
| { | ||
| var variableName = constantExpressionAst.Value; | ||
| if (assignments.ContainsKey(variableName)) |
There was a problem hiding this comment.
a comment indicating this is case insensitive? (I missed that until I saw the entire file - maybe it's ok)
There was a problem hiding this comment.
Comments can get out of date quickly, therefore I renamed the dictionary variable assignments to assignmentsDictionary_OrdinalIgnoreCase.
…nalyzer into GetVariable_DoNotTriggerUseDeclaredVarsMoreThanAssignment
|
@bergmeister could you fix the conflict and resubmit? |
…nalyzer into GetVariable_DoNotTriggerUseDeclaredVarsMoreThanAssignment Conflict Resolves by taking both # Conflicts: # Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
|
@JamesWTruher Ok. Done. |
| @@ -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); | |||
There was a problem hiding this comment.
Can you please explain me what the variable name: "assignmentsDictionary_OrdinalIgnoreCase" stands for ?
There was a problem hiding this comment.
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.
…nalyzer into GetVariable_DoNotTriggerUseDeclaredVarsMoreThanAssignment conflict resolved by taking both # Conflicts: # Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
PR Summary
Fixes #831
Detect the simplest possible use case
Get-Variableand mark the variables as being used to not trigger false positives. Because there are so many ways howGet-Variablecan be called, extracting the value of-Nameseemed to be quite difficult to me (unless I am not aware of a helper), therefore this improvement only works for simple use cases of Get-Variable (or its alias) asGet-Variable variableName. I think this is OK because it is a minimum, viable improvement of triggering less false positives of theUseDeclaredVarsMoreThanAssignmentwithout any side effects.PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready