Sonar coding_rules Rule ID java:S881 - Mentioning C/C++ resources, do they contribute or not

Rules and Languages
,
1

Hi.

A colleague of mine showed

Increment (++) and decrement (--) operators should not be used in a method call or mixed with other operators in an expression
Rule ID: java:S881, Analysis scope: main sources,
Rule repo: Sonar (Java), Effort: 5min, cert

to me and some other colleagues.

I may have issues with the rule reasonings - but that’s for later (maybe).

First a “more structural”(?) question:

In section “More info” three “Resources” are listed:

- CERT, EXP30-C. - Do not depend on the order of evaluation for side effects
- CERT, EXP50-CPP. - Do not depend on the order of evaluation for side effects
- CERT, EXP05-J. - Do not follow a write by a subsequent write or read of the same object within an expression

(including hyperlinks).

Is it by intention that C and C++ resources are given? (The first and the second resource.)

I guess not so, and that they were mentioned by accident/mistake. In these two resources nothing with impact to java is mentioned whatsoever - so what can they possibly add for the reasonings. Worse, they distract.

Is there a way/workflow for me to work toward a deletion of the mentioning of these two resources?

Of course someone may have put thought into this. Possibly so, I do not know (I am new here). That’s ok.

Thank you.

Volker Glave

2

Hi Volker,

In fact, they are listed on purpose. I know, because I used to be in charge of rules and I did that. :smiley:

I had what seemed like good reasons at the time. But it’s probably time to re-examine the thinking. So I’ll flag this for the language experts.

 
Ann

5

Thank you, Ann.

The remaining resource CERT, EXP05-J

"EXP05-J. Do not follow a write by a subsequent write or read of the same object within an expression
Deprecated

This rule may be deprecated and replaced by a similar guideline.

06/28/2014 – Version 1.0"

is not very convincing either.

I would question Rule ID java:S881 alltogether in it’s current form

"Increment (++) and decrement (–) operators should not be used in a method call or mixed with other operators in an expression

The use of increment and decrement operators in method calls or in combination with other arithmetic operators is not recommended, because:

  • It can significantly impair the readability of the code.
  • It introduces additional side effects into a statement, with the potential for undefined behavior.
  • It is safer to use these operators in isolation from any other arithmetic operators.

…"

It argues againt for example

    foo(++counter);

Following the rule we instead should write

    ++counter;
    foo(counter);

or

    counter++;
    foo(counter);

(Already a bad sign we now have two “good” alternatives. Instead of only one before.)

So what have we achived?

“It can significantly impair the readability of the code.”

The alternative is more/better readable than the first form? To my eyes not so.

“It introduces additional side effects into a statement, with the potential for undefined behavior.”

No “potential for undefined behavior” was present in the first place (original line of code). To the contrary, the behavior is defined in Java. So no improvement.

“It is safer to use these operators in isolation from any other arithmetic operators.”

No “other arithmetic operators” were present in the first place. So no improvement.

Even if they were, for example

    bar(++counter1, counter2--);

the rational does not hold (I would say),

    ++counter1;
    bar(counter1, counter2);
    --counter2;

… because … is it only me? … I do not understand … what may it be, that this is considered now “safer” than the original? bummer

In it’s current form, the rule ditches to many valid uses, I would say. The rule want’s to limit “overuse”, I understand. Another spelling for the rule should be found. (Not too easy; requires some work, I guess.)

Greetings, Volker

(sorry for having expanded the topic; should have created a new one I guess)

6

Hello Volker,

Thank you for reaching out and sharing your feedback. I’m a developer on the Java Analyzer team, and I’m sorry to hear that this rule is negatively impacting your workflow.

To give you a bit of history, rule java:S881 was created back in 2013 and has actually never been part of our default “Sonar way” Quality Profile. When it was initially introduced, it generated a large number of findings. Deciding whether a given finding is a true positive or a false positive is highly controversial, as it often boils down to a specific team’s coding style and their willingness to separate expression responsibilities. Because it was never added to the default profile, we haven’t invested time into improving its implementation over the years.

You are completely right that the rule description for Java is confusing. The rationale makes perfect sense for C/C++, where the evaluation order of method arguments is undefined (meaning the behavior of something like f( a(++x), b(++x) ) is unpredictable). In Java, however, the evaluation order is strictly defined, so the compiler is never confused. Realistically, for Java, this rule should only target highly complex cases—like the ones described in SEI CERT EXP05-J—where it becomes “undefined” or confusing in the developer’s mind rather than the compiler’s. That said, we are not sure if the effort required to refine the rule to this level would bring enough value right now.

Currently, the analyzer uses a very naive logic for this rule: it essentially just dictates “put the increment and decrement outside of this expression.” If you are curious, you can see exactly what it flags by looking at the rule’s unit tests in our repository (lines marked with // Noncompliant indicate an issue being raised).

If we ever wanted to activate this rule in the default Quality Profile, we would need to completely rework it to identify cases where keeping the increment/decrement inline isn’t too complex and is actually more readable.
For example, a loop like
while (++i < len) { ... }
is arguably much cleaner than writing
i++; while (i < len) { ... i++; }.

Given its current naive implementation and the fact that it is prone to generating noise, my recommendation is to simply remove this rule from your Quality Profile.

I hope this helps clarify the background of the rule.

Alban

8

I created SONARJAVA-6301 ticket to improve the rule description.

11

Thank you, Alban.