Warn when an if statement contains an assignment by bergmeister · Pull Request #859 · PowerShell/PSScriptAnalyzer

bergmeister · 2018-01-28T23:02:05Z

Closes #809
This is a simple prototype that checks if there is an assignment ast inside an if statement.
It currently works and catches cases like if( ($a = $b_){} and if( ($a = $b_){} but also warns if the execution block of the if statement contains an assignment like e.g. in if ($a -eq $b){$a=$b}. This needs to be fixed and is being tracked by the currently failing test.
The idea is to keep it simple to avoid false positives. For example the rule does not catch cases where there is an evalution inside the if statemenet as e.g. in if ( ($a == $b) ){}

…se where the execution block of the if statement contains an assignment

… counter tests

JamesWTruher

I don't think this is quite ready yet. There are a couple of problems which should be addressed first. There is a pattern which is very useful which will cause warnings to appear:

if ( $files = get-childitem ) {
... do something ...
}
else {
... do something else ...
}

I think if you can be more specific about the test of 2 variables in the ifstatementast (which should help you address the issue you found in the assignment outside of the test.

bergmeister · 2018-01-29T20:54:36Z

Hmm, such a case was discussed in the issue as well but my thinking was that the helpfulness of the warning makes up for those cases where the warning can be ignored. But you have a fair point, I will have a think if it is possible to get more certainty about the use case of the assignment operator.
But thanks for the feedback, that's exactly why I made a WIP PR because it is better to fail early. :-)
When looking at the warning level, I was not really satisfied with the 3 currently available levels (error, warning, info). Maybe if we had a level that addresses smells or possibe problems, then a user could do a thorough scan of the script e.g. for new code but has to review each warning individually but only require e.g. info level to pass in CI. A good example of warnings where the user has to use its own intelligence on deciding whether the warning can be suppressed or not is the Possible multiple enumeration of IEnumerable warning of ReSharper where ReSharper cannot figure out how an IEnumerable is being used. What are your thoughts on this?

JamesWTruher · 2018-01-31T21:27:37Z

The IsStatementAst has the Clauses member which should help. Here's a simple example in script:

using namespace System.Management.Automation.Language
param ( [scriptblock]$scriptblock = { if ( $a = $b ) { "the same" } } )
$If = $scriptblock.Ast.Find({$args[0] -is [IfStatementAst]}, $false)
if ( ! $If ) { write-verbose -verbose "no if" ; return }
foreach ( $clause in $If.Clauses ) {
    $assignment = $clause.Item1.Find({$args[0] -is [assignmentstatementast]}, $true)
    if ( $assignment ) {
        $msg =  ("Checking " + $assignment)
        if ( $assignment.Right.Expression -is [variableexpressionast] ) {
            Write-Error "${msg} : Don't assign a variable to a variable"
        }
        else {
            Write-Verbose -verbose "${msg} : Is OK!"
        }
    }
    else {
        Write-Verbose -verbose ("${msg} : no assignment in '" + $clause.item1 + "'")
    
}

when run, you can see what happens!

PS> /tmp/ast1 -scriptblock { if ( $a = get-date ) { 1 } }
VERBOSE: Checking $a = get-date : Is OK!
PS> /tmp/ast1 -scriptblock { if ( $a = $b ) { 1 } }
/tmp/ast1.ps1 : Checking $a = $b : Don't assign a variable to a variable
At line:1 char:1
+ /tmp/ast1 -scriptblock { if ( $a = $b ) { 1 } }
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [Write-Error], WriteErrorException
+ FullyQualifiedErrorId : Microsoft.PowerShell.Commands.WriteErrorException,ast1.ps1

PS> /tmp/ast1 -scriptblock { if ( $a = get-date ) { 1 } elseif ( $b = $c ) { 2 } else { 3 } }
VERBOSE: Checking $a = get-date : Is OK!
/tmp/ast1.ps1 : Checking $b = $c : Don't assign a variable to a variable
At line:1 char:1
+ /tmp/ast1 -scriptblock { if ( $a = get-date ) { 1 } elseif ( $b = $c  ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [Write-Error], WriteErrorException
+ FullyQualifiedErrorId : Microsoft.PowerShell.Commands.WriteErrorException,ast1.ps1

…e sure that the following case does not get flagged up (added a test case for it) 'if ( $files = get-childitem ) { }'

bergmeister · 2018-02-01T07:16:46Z

Thanks for the pointers. I improved it but still need to make sure that the case which you pointed out does not get flagged up (I added a test for it in a TDD manner). I will let you know once the PR is ready for re-review.

…nly false positive that is left is 'if ($a = (Get-ChildItem)){}' where the RHS is wrapped in an exprssion

bergmeister · 2018-02-03T11:26:55Z

@JamesWTruher : The PR is ready for review now. I have improved it to not flag false positives like if ($a = (Get-ChildItem)){} and as far as I can see I can detect all positives cases of using = or == inside if statements. Also please feel free to comment on the rule name and the wording of the warning strings.

JamesWTruher · 2018-02-05T23:48:58Z

Rules/PossibleIncorrectUsageOfAssignmentOperator.cs

+                    {
+                        // Check if someone used '==', which can easily happen when the person is used to coding a lot in C#.
+                        // In most cases, this will be a runtime error because PowerShell will look for a cmdlet name starting with '=', which is technically possible to define
+                        if (assignmentStatementAst.Right.Extent.Text.StartsWith("="))


this will miss the case of
$a = = $b
but that's probably ok

bergmeister added 2 commits January 28, 2018 22:56

first try at implementation rule. basically works but also catches ca…

413e6c0

…se where the execution block of the if statement contains an assignment

remove unused variables and fix test to import pssa and fix some rule…

e452a99

… counter tests

JamesWTruher suggested changes Jan 29, 2018

View reviewed changes

Improve rule to only assert against clause in if statement. TODO: Mak…

647a6a6

…e sure that the following case does not get flagged up (added a test case for it) 'if ( $files = get-childitem ) { }'

bergmeister added 3 commits February 1, 2018 23:24

Improve rule to reduce false warnings and adds lots more tests. The o…

93660bd

…nly false positive that is left is 'if ($a = (Get-ChildItem)){}' where the RHS is wrapped in an exprssion

fix false positive when the command is wrapped in an expression

3a35ba3

simplify and cleanup

79fa369

bergmeister changed the title ~~WIP: Warn when an if statement contains an assignment~~ Warn when an if statement contains an assignment Feb 3, 2018

Uncomment import of PSSA in test to be consistent with other tests

4029359

JamesWTruher reviewed Feb 5, 2018

View reviewed changes

JamesWTruher approved these changes Feb 5, 2018

View reviewed changes

JamesWTruher merged commit 048c7f3 into PowerShell:development Feb 5, 2018

LaurentDardenne mentioned this pull request Feb 14, 2018

Warn when using FileRedirection operator inside if/while statements and improve new PossibleIncorrectUsageOfAssignmentOperator rule #881

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when an if statement contains an assignment#859

Warn when an if statement contains an assignment#859
JamesWTruher merged 7 commits intoPowerShell:developmentfrom
bergmeister:warnWhenUsingAssignmentInsideIfStatement

bergmeister commented Jan 28, 2018 •

edited

Loading

Uh oh!

JamesWTruher left a comment

Uh oh!

bergmeister commented Jan 29, 2018 •

edited

Loading

Uh oh!

JamesWTruher commented Jan 31, 2018

Uh oh!

bergmeister commented Feb 1, 2018 •

edited

Loading

Uh oh!

bergmeister commented Feb 3, 2018 •

edited

Loading

Uh oh!

JamesWTruher Feb 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bergmeister commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesWTruher commented Jan 31, 2018

Uh oh!

bergmeister commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergmeister commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesWTruher Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bergmeister commented Jan 28, 2018 •

edited

Loading

bergmeister commented Jan 29, 2018 •

edited

Loading

bergmeister commented Feb 1, 2018 •

edited

Loading

bergmeister commented Feb 3, 2018 •

edited

Loading