Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a .Net property and only look at the LHS #1008

bergmeister · 2018-06-02T20:53:22Z

PR Summary

Fixes NullReferenceException in PSAvoidAssignmentToAutomaticVariable rule when assigning .Net property to .Net property (v 1.17.0) #1007 by adding a null check for the variable variableExpressionAst
Fixes PSAvoidAssignmentToAutomaticVariable finds auto variable in RHS when assigning to a static property #1013 by only looking at the LHS for variable names.
Fixes PSAvoidAssignmentToAutomaticVariable triggers on property assignment of automatic variable #1012 by making sure the VariableExpressionAst's parent is the AssignmentAst (i.e. not a property of the variable or something like that)

The issues are slightly related to each other, therefore they are both addressed in this PR to keep things simple.

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

… when assigning a net property to a .Net property

bergmeister · 2018-06-02T21:08:13Z

Rules/AvoidAssignmentToAutomaticVariable.cs

@@ -47,18 +47,21 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
            foreach (var assignmentStatementAst in assignmentStatementAsts)
            {
                var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
-                var variableName = variableExpressionAst.VariablePath.UserPath;
-                if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase))
+                if (variableExpressionAst != null)


only this null check was added, all the code diff below is only the added indentation

from a flow perspective, this could also be

if ( variableExpressionAst == null ) continue

and the indenting is preserved :)

JamesWTruher

neither of my comments are blockers

JamesWTruher · 2018-06-04T19:58:29Z

Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1

@@ -63,6 +63,21 @@ Describe "AvoidAssignmentToAutomaticVariables" {
            $warnings.Count | Should -Be 0
        }

+        It "Does not throw a NullReferenceException when using assigning a .Net property to a .Net property (Bug in 1.17.0 - issue 1007)" {
+            $exceptionThrown = $false
+            try


shouldn't this just be:
{ Invoke-ScriptAnalyzer -ScriptDefinition '[foo]::bar = [baz]::quz' -ErrorAction Stop } | Should Not Throw
It will make the log easier to understand if it fails.

Pester only works with terminating errors. PSSA throws non-terminating errors, see pester/Pester#366

i would have thought that -ErrorAction Stop would do the trick (after all, it's in a try/catch, so you're doing the same thing) so somebody is halting the pipeline

JamesWTruher · 2018-06-04T19:59:53Z

Rules/AvoidAssignmentToAutomaticVariable.cs

@@ -47,18 +47,21 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
            foreach (var assignmentStatementAst in assignmentStatementAsts)
            {
                var variableExpressionAst = assignmentStatementAst.Find(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: false) as VariableExpressionAst;
-                var variableName = variableExpressionAst.VariablePath.UserPath;
-                if (_readOnlyAutomaticVariables.Contains(variableName, StringComparer.OrdinalIgnoreCase))
+                if (variableExpressionAst != null)


from a flow perspective, this could also be

if ( variableExpressionAst == null ) continue

and the indenting is preserved :)

…the LHS

JamesWTruher

lgtm

JamesWTruher · 2018-06-04T22:42:11Z

Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1

@@ -63,6 +63,21 @@ Describe "AvoidAssignmentToAutomaticVariables" {
            $warnings.Count | Should -Be 0
        }

+        It "Does not throw a NullReferenceException when using assigning a .Net property to a .Net property (Bug in 1.17.0 - issue 1007)" {
+            $exceptionThrown = $false
+            try


i would have thought that -ErrorAction Stop would do the trick (after all, it's in a try/catch, so you're doing the same thing) so somebody is halting the pipeline

Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule…

3701e65

… when assigning a net property to a .Net property

bergmeister added the Issue - Bug label Jun 2, 2018

bergmeister added this to the 1.17.1 milestone Jun 2, 2018

bergmeister self-assigned this Jun 2, 2018

bergmeister requested a review from JamesWTruher June 2, 2018 20:53

bergmeister commented Jun 2, 2018

View reviewed changes

JamesWTruher approved these changes Jun 4, 2018

View reviewed changes

bergmeister added 3 commits June 4, 2018 21:11

when variableExpressionAst is null, then continue loop to reduce nesting

9910fda

fix indentation for cleaner diff

e4d1c72

Fix issue PowerShell#1013 as well by making sure the rule looks only …

8136638

…the LHS

bergmeister changed the title ~~Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a net property to a .Net property~~ Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a .Net property and only look at the LHS Jun 4, 2018

Fix issue PowerShell#1012 as well

f818df7

JamesWTruher approved these changes Jun 4, 2018

View reviewed changes

Simplify test that checks that PSSA does not throw to address PR review

9fbe4e1

bergmeister merged commit 23095f6 into PowerShell:development Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a .Net property and only look at the LHS #1008

Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a .Net property and only look at the LHS #1008

bergmeister commented Jun 2, 2018 •

edited

Loading

bergmeister Jun 2, 2018

JamesWTruher Jun 4, 2018

JamesWTruher left a comment

JamesWTruher Jun 4, 2018

bergmeister Jun 4, 2018

JamesWTruher Jun 4, 2018

JamesWTruher Jun 4, 2018

JamesWTruher left a comment

JamesWTruher Jun 4, 2018

Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a .Net property and only look at the LHS #1008

Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a .Net property and only look at the LHS #1008

Conversation

bergmeister commented Jun 2, 2018 • edited Loading

PR Summary

PR Checklist

bergmeister Jun 2, 2018

Choose a reason for hiding this comment

JamesWTruher Jun 4, 2018

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

JamesWTruher Jun 4, 2018

Choose a reason for hiding this comment

bergmeister Jun 4, 2018

Choose a reason for hiding this comment

JamesWTruher Jun 4, 2018

Choose a reason for hiding this comment

JamesWTruher Jun 4, 2018

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

JamesWTruher Jun 4, 2018

Choose a reason for hiding this comment

bergmeister commented Jun 2, 2018 •

edited

Loading