Atom of confusion detection using sonar

[tim.kraeuter (Tim)]

Hi Sonar community,

I recently read this paper about Atoms of confusion in Java, and most of the atoms of confusion (ACs) look like code smells to me. However, I wondered if someone knows a rule to check the AC Logic as Control Flow (LaCF).

The problem in LaCF is that lazy-or (||) or lazy-and (&&) contain variable changes in the second argument.

Example confusing, aka non-compliant code (sample 40 from the paper):

    int V1 = 1;
    int V2 = 5;
    if (++V1 > 0 || ++V2 > 0) {
      V1 = V1 * 2;
      V2 = V2 * 2;
    }
    System.out.println(V1 + " " + V2);

Example non-confusing, aka compliant code (sample 40 from the paper):

    int V1 = 2;
    int V2 = 4;
    if (++V1 > 0) {
      V1 = V1 * 2;
      V2 = V2 * 2;
    } else if (++V2 > 0) {
      V1 = V1 * 2;
      V2 = V2 * 2;
    }
    System.out.println(V1 + " " + V2);

Does anyone know a sonar rule which would detect the LaCF AC?
I currently could not find or think of one specific rule. RSPEC-881 somewhat helps but could be circumvented using a setter.

Is this a code smell in your opinion?

Thanks in advance for any hints and opinions!

[jbeleites (Johann Beleites)]

Hello Tim,

This looks like an interesting topic. I cannot find the exact example you provided in the paper (or sample 40) - I assume this is an example you created on the base of the paper?

I am not convinced that the second example is non-confusing. There is still logic embedded in the conditions, now it is just split over two conditions. V1 will still be incremented while evaluating the first condition and V2 is still sometimes incremented, depending on whether or not the first then branch is executed. Worse, we now have two branches that do the same thing, an antipattern (and covered in S1871).

That being said, I agree with you that this kind of problem would be great to flag to developers. Some more specific cases may be covered by some of our rules already, such as S881, which you already found. I would think that coming up with a single rule that covers all cases of LaCF is not a trivial task.

What if a custom method is called? In many cases, we won’t statically know its behavior.

To develop a rule, the first important step is to come up with a well-defined scope for a rule, which I don’t quite see here yet. Is this something you would like to explore further?

[tim.kraeuter (Tim)]

The sample was taken from the appendix of the paper 4.
I was mostly interested in potential rules detecting such situations, which do not seem to exist yet besides S-881.

As you pointed out correctly, the snippet might not be the best since it introduces duplicate code.

I am unfamiliar with what is possible with sonar rules, but can one “easily” detect if a method has side effects (change fields in objects or public fields)? If yes, a rule could forbid side effects in the second argument of lazy-or (||) and lazy-and (&&).

I suspect one cannot easily detect this during static analysis. Then it would not be worth exploring this further.

Thanks for your insight!

[jbeleites (Johann Beleites)]

The sample was taken from the appendix of the paper 4.

Thanks, got it now.

As you already guessed, it is quite a challenge to statically detect whether a method has side effects, at least in languages such as Java. In some cases, it boils down to the halting problem.

Take abstract/overridden methods, where the implementing/overriding method is not in the code base we are currently scanning. Even if we decide to ignore impossible scenarios like this, finding out if an arbitrary method X (and all code called from that method) has any side effects would be very computationally expensive to do accurately and not possible to do perfectly. It would require techniques such as symbolic execution.

So while I think this is a very interesting topic, depending on the scope, it can become quite a beast to achieve a good rate of detection with an acceptable false-positive rate.

[tim.kraeuter (Tim)]

I agree. The tradeoff does not seem to be worth it for now.

Thanks!