Skip to content

/ PSScriptAnalyzer

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

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

Merged
merged 6 commits into from May 11, 2018
Merged

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

merged 6 commits into from May 11, 2018

Conversation

@bergmeister
Copy link
Collaborator

bergmeister commented Mar 9, 2018

PR Summary

Fixes #831

Detect the simplest possible use case Get-Variable and mark the variables as being used to not trigger false positives. Because there are so many ways how Get-Variable can be called, extracting the value of -Name seemed 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) as Get-Variable variableName. I think this is OK because it is a minimum, viable improvement of triggering less false positives of the UseDeclaredVarsMoreThanAssignment without any side effects.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • User facing documentation needed
  • Change is not breaking
  • Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready
@bergmeister
Do not trigger UseDeclaredVarsMoreThanAssignment when using Get-Variable
Loading status checks…
0de98e9
@bergmeister bergmeister self-assigned this Mar 9, 2018
@bergmeister bergmeister requested a review from JamesWTruher Mar 9, 2018
@bergmeister bergmeister added in progress and removed in progress labels Mar 9, 2018
@bergmeister
update licence text
Loading status checks…
c67fc8d
@JamesWTruher
JamesWTruher reviewed Mar 16, 2018
View changes
Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1 Outdated
@@ -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 }

This comment has been minimized.

Sign in to view
@JamesWTruher

JamesWTruher Mar 16, 2018 Member

is the where-object needed?
could this just be

Invoke-ScriptAnalyzer -ScriptDefinition '$a=4;Get-Variable a' | Should -BeNullOrEmpty

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Mar 16, 2018 •
edited
Author Collaborator

I removed the Where-Object now since this seems to be your preferred approach.
Rules/UseDeclaredVarsMoreThanAssignments.cs Outdated
if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst)
{
var variableName = constantExpressionAst.Value;
if (assignments.ContainsKey(variableName))

This comment has been minimized.

Sign in to view
@JamesWTruher

JamesWTruher Mar 16, 2018 Member

a comment indicating this is case insensitive? (I missed that until I saw the entire file - maybe it's ok)

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Mar 23, 2018 Author Collaborator

Comments can get out of date quickly, therefore I renamed the dictionary variable assignments to assignmentsDictionary_OrdinalIgnoreCase.
bergmeister added 2 commits Mar 23, 2018
@bergmeister
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
0046b84 
…nalyzer into GetVariable_DoNotTriggerUseDeclaredVarsMoreThanAssignment
@bergmeister
address PR comments
Loading status checks…
361c0d8
@JamesWTruher
Copy link
Member

JamesWTruher commented Mar 30, 2018

@bergmeister could you fix the conflict and resubmit?
@bergmeister
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
Loading status checks…
7b6f8dd 
…nalyzer into GetVariable_DoNotTriggerUseDeclaredVarsMoreThanAssignment

Conflict Resolves by taking both
# Conflicts:
#	Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
@bergmeister
Copy link
Collaborator Author

bergmeister commented Mar 30, 2018

@JamesWTruher Ok. Done.
@kalgiz
kalgiz approved these changes Apr 5, 2018
View changes
Copy link
Contributor

kalgiz left a comment

I have just one comment about the variable naming.
Rules/UseDeclaredVarsMoreThanAssignments.cs
@@ -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);

This comment has been minimized.

Sign in to view
@kalgiz

kalgiz Apr 5, 2018 Contributor

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

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Apr 5, 2018 Author Collaborator

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.
@bergmeister
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
Loading status checks…
ec7d594 
…nalyzer into GetVariable_DoNotTriggerUseDeclaredVarsMoreThanAssignment

conflict resolved by taking both
# Conflicts:
#	Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
@JamesWTruher
JamesWTruher approved these changes May 11, 2018
View changes
@JamesWTruher JamesWTruher merged commit 703ebe4 into PowerShell:development May 11, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalgiz kalgiz

@JamesWTruher JamesWTruher

Assignees

@bergmeister bergmeister

Labels
Area - Rules
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

PSUseDeclaredVarsMoreThanAssignments false positive when used via Get-Variable
3 participants
@bergmeister @JamesWTruher @kalgiz
You can’t perform that action at this time.