AssignmentInConditional reports false positives for getters

[rkrisztian]

Using Gradle 5.6.4 (Groovy 2.5.4) with the codenarc plugin, the following example code produces a false positive for the AssignmentInConditional rule.

a = ​[1, 2, 3, 4, null]
i = 0
while (b = a.get(i++))​ { print b }

The error is:

[SRC]while (b = a.get(i++)) { print b }
[MSG]Assignment used as conditional value, which always results in true. Use the == operator instead

The problem is, when the right side of the assignment is not a constant or literal, but a function/closure call, then it is more likely to match the actual intention of the code author.

Yes, I am aware that the code above should be rewritten to functional style to make it more readable:

[1, 2, 3, 4, null].takeWhile { it }.each { print it } 

However, Groovy still does not have a shorthand equivalent for ZipInputStream entries handling (and probably never will), where the rule would report a violation too:

while (entry = someZipInputStream.nextEntry) { /* ... */ }

Therefore, I would suggest to relax this rule somehow.

[chrismair]

Hmmm, interesting scenario. So far I can't think of a way to avoid that without also silencing a bunch of "legitimate" violations (actual mistakes of = instead of ==), e.g. if (appointment.date = currentDate()).

[rkrisztian]

Yeah, I agree in an if statement, such expressions are more likely mistakes. I'd be really confused to read correct code like that, because that line alone there looks too complex to begin with: the assignment is logically unrelated to what you want to do in the condition, and just looks sloppy code. (Of course I know it was just an example for a valid violation.)

A while loop however, is similar to a for loop in the sense that you use that expression not only for checking for the end condition, but if there is an assignment in it, then also for advancing the loop variable. I don't mean a.get(i++) though, because we have more sophisticated iteration constructs now, like Java iterators, for-each constructs, functional style, etc. To be honest, I'd prefer having false positives when the code can be rewritten to functional style. At the same level, we as code readers aren't interested in both the delicate details of the "how" (the code is doing what it does), and the "what" (the code is doing). Sadly, not all code can be rewritten like that, and we can't know all the exceptions.

I believe that many of such problems can be avoided well if you write tests. A good set of unit test should easily reveal an unintended assignment, considering you would test not just happy paths. E.g. an infinite while loop due to an unintended assignment should be easily pointed out by any kind of unit test covering that line actually. Testing complements static code analysis.

So this problem has quite a few faces. First, the rule is already useful as giving warnings, analogously to "security hotspots" in SonarQube, where only potential mistakes are told, but manual review is needed. Probably more correctness can be achieved for this rule with deeper analysis, but if I recall, that's an NP-hard problem. So we might want to stick to a strategy like "which guesses can result in less probable false positives."

In other words, I propose that:

• Either we should have manually reviewable warnings (I don't know how this could fit into SonarQube, which I also use, together with the Groovy plugin using CodeNarc), or:
• We make the rule do much less false positives by something like:
  • We should be very cautious with what we call as error in a while condition. We might want to let a few true positives go.
  • We can be much stricter in if conditions. Usually assignments are a problem of code readability too.

I know no better, but I'm open for better ideas.

[chrismair]

Re: warnings, what I have done in such cases is to set the rule priority to 4, and include those in the report(s), but those violations will not fail the build. But of course that applies to the rule in all cases.