Rule to warn against use of equal sign when comparator (-eq) should be used

[jongross4]

When moving from other languages where a simple '=' equal sign can be used both as assignment and operator, a warning should be logged. Other Boolean operations should be flagged too. Examples:

if ($x = $y) {} -- Always true

for ($i = 1; $i > 99; $i++) - alias to out-file

[nightroman]

I agree about >, in certain contexts it may be treated as a very probable mistake.

But I have some doubts about =.
It is not true that the above $x = $y is always true.

$y = 0 # or $null, empty array, string
if ($x = $y) {'yes'} else {'no'} # it is 'no'

I agree that if ($x = $y) may look like a potential mistake.
But it may be a concise (and a bit more effective) code, too.

Example:

if ($x = <something>) {
    ... # work with $x
}
else {
    throw "something is empty!"
}

instead of the same

$x = <something>
if ($x) {
    ... # work with $x
}
else {
    throw "something is empty!"
}

[jongross4]

While I agree with the last example, it would be a very advanced concept for most scripters and more likely been done in error. Multiple times I have been tripped up by use of '=' in an 'if' statement, that reads syntactically correct for many other languages, but is an assignment statement in PowerShell.

[nightroman]

I agree with you, too. It's just going to be a noise rule for me...

[imfrancisd]

Can you allow the new rule "PSPossibleIncorrectUsageOfAssignmentOperator" to be suppressed by parenthesizing the condition?

For example:

    if ($a = 7) {}

can be fixed in the script with

    if (($a = 7)) {}

You can see this fix in other places like warn-assignment-condition.cpp

    if (x = 7) {} // expected-warning {{using the result of an assignment as a condition without parentheses}} \
                  // expected-note{{use '==' to turn this assignment into an equality comparison}} \
    // expected-note{{place parentheses around the assignment to silence this warning}}
    if ((x = 7)) {}

[imfrancisd]

To be clear

#Should not warn incorrect usage of assignment operator, but does
#
Invoke-ScriptAnalyzer -ScriptDefinition 'if (($a = 7)){}'

RuleName                            Severity     ScriptName Line  Message                                                     
--------                            --------     ---------- ----  -------                                                     
PSUseDeclaredVarsMoreThanAssignment Warning                 1     The variable 'a' is assigned but never used.                
s                                                                                                                             
PSPossibleIncorrectUsageOfAssignmen Information             1     Did you really mean to make an assignment inside an if      
tOperator                                                         statement? If you rather meant to check for equality, use   
                                                                  the '-eq'  operator.                                        

[bergmeister]

@imfrancisd Interesting thought but I think this might spark a discussion from the PowerShell purists about unnecessary parenthesis.
PR 881 that is referenced above is proposing to change it to only warn about assignments inside if statements if the assigned variable is not being referenced in some way in the statement block. This way the false-positive problem gets solved in a nice way in my opinion. Feel free to comment and participate in the PR discussion.
Examples of the new proposed behaviour in the PR

if ($a=$b){}           # will warn because one should either use the variable or the assignment operator was used unintentionally
if ($a=$b){$c=$a} # will not warn because $a is referenced
if ($a=$b){$a=$c} # will not warn because $a is referenced
# similar behaviour applies to the 'else if' block

[imfrancisd]

Aversion to parentheses is a good heuristic!

@imfrancisd Interesting thought but I think this might spark a discussion from the PowerShell purists about unnecessary parenthesis.

If you see a purist use "unnecessary" parentheses like this

if (($x = 7)) {}

then you know they mean it, and it is not an accident! :-)

Simple Rule, Simple Remedy, Simple Enforcement

With this approach (which is also used by C analyzers), the rule is simple:

If the result of an assignment is used as a condition, clarify by wrapping it in parentheses, otherwise change the assignment to a comparison.

Pitfalls to this approach

The complexity with this approach comes with conditions like this:

    #Is the assignment ($a = $b) okay or not?
    #
    if (($a = $b) -and ($c -eq $d)) {}

But in cases like this, I don't think anyone will complain about an analyzer telling them to change the code.

[imfrancisd]

Problem with checking if variable is used

How do you know if this is okay?

    $a = $b

    if ($a = $c) {
       $a
    }

The variable "$a" is used, but how do you know that "$a = $c" is what I intended?

[bergmeister]

@imfrancisd I referenced your comments in PR 881 because that's where I'd like to see the discussion happen. You have interesting ideas so let's see what other people think about this new approach/mindset. About your last comment: It is hard and in some cases impossible to get 100% test case coverage but it is more important to limit the number of false positives as much as possible. This rule is brand new and therefore the approach is to first find cases where we know for sure that we should warn and once this works reasonably well, we can improve it further.