[Java] Avoid bitwise operators

Rules and Languages New rules / language support
,
1

Hi,

I would like to propose a new rule for SonarJava. While it is certainly not a candidate for inclusion in the default Sonar Way quality profile, I believe it will still be useful to many teams and organizations, including my own company.

If the rule gets approved, I will finish up my proof of concept implementation and create a PR.

Here are the details:

Rule description & motivation

Java’s bitwise operators (and/or/xor, left/right shift, unsigned right shift and corresponding compound assignment operators) have several valid uses, e.g. binary file formats, cryptography algorithms or graphics programming.

That said, many developers working outside these areas are not accustomed to these operators because their application code usually has no need for them. In such applications, performing bitwise manipulation of numbers may be an indicator of premature optimization attempts and/or developers who are more familiar with low-level coding than with object-oriented concepts or the Java API (e.g. enum and EnumSet<MyEnum> as a replacement for the classic bit fields approach).

For teams who are working in such contexts, SonarJava should therefore offer a rule that alerts them whenever bitwise operators are used. This way, the code can be verified to be a justified and correctly implemented use of bit manipulation, or replaced with more obvious and/or type-safe high-level code.

Noncompliant Code

public int double(int base) {
    return original << 1;
}

class Style {
    public static final int BOLD = 1;
    public static final int ITALICS = 2;
    public static final int UNDERLINE = 4;
}

public void demo() {
    int heading = Style.BOLD | Style.UNDERLINE;
    apply(heading);
}

public void apply(int styles) {
    if ((styles & Style.BOLD) == Style.BOLD) applyBold();
    if ((styles & Style.ITALICS) == Style.ITALICS) applyItalics();
    if ((styles & Style.UNDERLINE) == Style.UNDERLINE) applyUnderline();
}

Compliant Code

public int double(int base) {
    return original * 2; 
}

enum Style {
    BOLD, ITALICS, UNDERLINE;
}

public void demo() {
    Set<Style> heading = EnumSet.of(Style.BOLD, Style.UNDERLINE);
    apply(heading);
}

public void apply(Set<Style> styles) {
    if (styles.contains(Style.BOLD)) applyBold();
    if (styles.contains(Style.ITALICS)) applyItalics();
    if (styles.contains(Style.UNDERLINE)) applyUnderline();
}

Impact to keep this code as it is
Code which needlessly uses bit manipulation tends to make the author’s intention less obvious than the object-oriented equivalent. Also, it increases the risk that future changes introduce bugs, e.g. adding another flag to an int bit field but accidentally using the same value as an existing one.

Notes
No issue should be raised when the and/or/xor operators or their compound assignment counterparts are used with boolean values.

References

Type
Code Smell

Tags
clumsy, pitfall

2

Hi @bannmann,

Thank you for suggesting this rule. As a java developer I agree with the ideas that bitwise operators should be used carefully and premature optimization is the root of all evil.

However, as you said this rule cannot be part of Sonar Way because it would raise too many false positives. There are many cases where bitwise operations are perfectly valid, even in projects where the use of bitwise operators is rare. Developers would have to either disable the rule or mark issues as False Positives/Won’t Fix. We don’t have, and don’t intend to add, a “Reviewed” status for Code Smells and Bugs. This would make SonarQube an audit tool.

I’m afraid that we can’t accept the rule as it is.

Cheers,
Nicolas

3

Hi @Nicolas_Harraudeau,

I must say I’m surprised of your answer, and respectfully ask you to reconsider your decision.

Before I give my rationale, I’d like to emphasize that none of the following, no matter how critical it is, was intended to insult or disrespect you or anyone else personally, or to talk down any of the decisions, processes or products at SonarSource. Instead, my goal is to shed some light on a few aspects that you maybe just haven’t thought of yet. Also, I’d like to understand the criteria you have for what makes rules acceptable so I can direct my future efforts towards the right things.

Let’s dive in.

  1. Your answer can be read as if you’re stating that a new rule would only be considered if it is suitable for Sonar Way. However, that probably wasn’t your intention as it’s obvious that SonarQube does offer rules that are opt-in because of their controversial nature: of all Java code smell rules currently available on SonarCloud, 211 are active in Sonar Way and 130 are not. Limited to the ones added in 2018 and 2019, the numbers are 44 to 14.

  2. The distinction between “rule raises many false positives” or “is useful” highly depends on the purpose of the codebase and the coding standards of the team maintaining it.

    To demonstrate this, I’d like to look at the results of applying a selection of rules not part of Sonar Way, and the “avoid bitwise operators” rule I proposed, to two codebases: first, the sonarqube-6.5 codebase used for SonarJava integration tests, second, my company’s primary codebase for which I’ll use the alias corporate here.

    For each rule, I’ll also give my personal opinion on whether the rule could/should be activated and/or how much work checking the issues would be.

    To make the issue numbers comparable despite the different codebase sizes (200K vs 62K SLOC), I’m going to indicate the issue “density” which I calculate as issue_count / SLOC * 1000.

    • S1774 The ternary operator should not be used raises an issue each time the ternary operator is used (condition ? if_true : if_false)
      • sonarqube-6.5: 286 issues (density ~1.43)
      • corporate: 4 issues (density ~0.06; rule is active)
      • Conclusion: Obviously, one team uses the ternary operator freely while the other does not like it at all. Put another way, while the rule can help corporate stay true to its chosen coding convention, it seems totally useless to sonarqube-6.5 (plus probably a majority of all Java developers) as they don’t share the concerns stated in the rule description.
    • S1314 Octal values should not be used raises an issue each time an integer literal is not (hexa)decimal, but octal (021 is octal for decimal 17)
      • sonarqube-6.5: 0 issues
      • corporate: 0 issues (rule is active).
      • Conclusion: This rule could clearly be activated for sonarqube-6.5 as well. However, other codebases could have valid uses of octal number literals (e.g. unix file permissions), so it may be a bad idea to add this rule to Sonar Way.
    • S2039 Member variable visibility should be specified raises an issue when visibility of a member is “package-protected” (officially called “default”).
      • sonarqube-6.5: 783 issues (density ~3.9). That’s quite a large number; I would guess that most of these issues are cases where it’s perfectly valid to make a member visible for the package (and subclasses) and should therefore be marked as “Won’t fix”. But activating this rule would still cause a lot of manual checking work and therefore probably does not make much sense for SonarSource.
      • corporate: 16 issues (density ~0.3). We don’t plan on activating this rule. Still, it raises so few issues in our codebase that even if they all end up as “Won’t fix” (i.e. because we deem them to be valid uses of package-protected visibility), checking and marking them would not be much work.
      • Conclusion: S2039 is not only controversial, it can raise a large number of issues. Personally, I don’t really get the point of the rule, but I guess that for some organization (or even to a significant portion of Java developers) it is useful - otherwise the rule presumably would not have been added in the first place.
    • S4551 Enum values should be compared with “==” raises an issue when enums are compared with .equals() which is error-prone.
      • sonarqube-6.5: 47 issues (density ~0.2). A low number of issues, so checking each issue (and manually fixing the code or closing the issue) is not too much work. If they want, SonarSource could easily activate this rule.
      • corporate: 0 issues (rule is active).
      • Conclusion: This rule is a matter of general opinion, not something you would evaluate for each piece of code. So if a team decides to activate this rule, there will be no false positive in the sense of “well this time, .equals() is better than ==”. However, other teams may favor the consistency of the approach “compare any object including enums with .equals()” over the slight advantages of “compare enums with ==”.
    • A pair of rules about serialVersionUID:
      • S2057 “Serializable” classes should have a “serialVersionUID” raises an issue each time a serializable class does not have a hard-coded serialVersionUID.
        • sonarqube-6.5: 8 issues (density ~0.04).
        • corporate: 22 issues (density ~0.4)
      • S4926 “serialVersionUID” should not be declared blindly raises an issue each time a serializable class has a hard-coded serialVersionUID.
        • sonarqube-6.5: 4 issues (density ~0.02).
        • corporate: 0 issues (rule is active).
      • Conclusion: these rules are direct opposites: S2057 says “specify”, S4926 says “leave out”. If a codebase makes heavy use of serialization (unlike the two above), either rule will obviously produce lots of issues if the code was written using the opposite approach. However, in any case, both rules can help a team stay on track with their chosen approach.
    • Avoid bitwise operators: I included my proof of concept implementation of this rule in SonarJava’s ruling test and checked its results for plausibility with a few regex searches.
      • sonarqube-6.5: 2 issues (density ~0.01), both appearing in (...).projectanalysis.analysis.Analysis#hashCode() (link to source line).
      • corporate: 0 issues
      • Conclusion: Bitwise operators are extremely rare to non-existant in these two codebases. If this rule existed, the teams could keep it that way and would not have to deal with many initial issues. My own company would definitely like to activate that rule. And while I’m not saying SonarSource itself should also activate that rule, the sonarqube-6.5 codebase certainly proves that bitwise operators are rare in certain types of applications. (Of course I’m not pretending that this rule would make sense for e.g. Guava or the JDK itself.)

  3. There are many cases where bitwise operations are perfectly valid, even in projects where the use of bitwise operators is rare. Developers would have to either disable the rule or mark issues as False Positives/Won’t Fix.

    • Then again, there are also many cases where e.g. the ternary operator or octal number literals are perfectly valid, but still SonarQube offers optional code smell rules that flag each and every use of these language features (S1774 and S1314 respectively). I think that the ternary operator is even more commonly used in the Java world than bitwise operators, so this feels like a contradiction. How does SonarSource decide whether an optional “do not use this” code smell rule makes sense for a given language feature?
    • Also, if we’re in a project “where the use of bitwise operators is rare”, the rule would obviously rarely raise issues. I don’t understand why it’s a problem to mark these rare issues as “Won’t Fix”. Isn’t that precisely the point of “Won’t Fix”? For example, with the existing SonarQube rules below, developers are likely to do the exact same thing:
      • A team activates S1774 (“The ternary operator should not be used”) because they feel the ternary operator makes their code harder to read. However, they may accept it in some cases where avoiding it would make the code unnecessarily verbose (e.g. the occasional algorithm, or a hashCode() implementation), so they mark it as “Won’t Fix” in these cases.
      • When S4926 (“serialVersionUID” should not be declared blindly) is active, any reported issue would usually be fixed. However, it sometimes may instead be marked as “Won’t fix”, e.g. if a particular class needs to be backwards compatible (deserializing an instance from the previous release) and therefore keep its hard-coded serialVersionUID.

  4. Developers would have to (…) disable the rule.

    • My interpretation of the “plug the leak” metaphor is that just because a certain rule raises a large number of issues, you would never disable that rule. The reason is that when a team uses the approach recommended by SonarSource in the linked article (TLDR: fix issues in new code, ignore issues in old unchanging code), that rule still helps them to not introduce more problems with new code, and helps them fix the issues along the way when they touch old code (the “boy scout rule”).

  5. We don’t have, and don’t intend to add, a “Reviewed” status for Code Smells and Bugs. This would make SonarQube an audit tool.

    • I don’t think such a Reviewed status would be necessary for this rule or any other rule. The existing statuses Open (“set by SonarQube on new issues”) and Confirmed (“set manually to indicate that the issue is valid”) are perfectly sufficient (see documentation on Issues lifecycle).
      But maybe I’m missing your point here. Could you please explain why adding this rule would only make sense if SonarQube had a Reviewed status? I’d especially like to understand why on the other hand S1774 (“The ternary operator should not be used”) does not need such a status.
    • In theory, a Reviewed status would be required if SonarQube can’t really judge a given piece of code, and not even say “this looks suspicious” - this is the case with Security Hotspots. However, all the examples above are rules which alert to deviations from the usual approach and coding conventions of a team, which is always suspicious. So if the coding conventions says “don’t use bitwise operators” and the codebase has 0 bitwise operator usages, a rule enforcing this would not equal to auditing - it would just help keeping the desired status quo.

I apologize that this post got so lengthy, but hope it was worth the read.

Please let me know what you think!

Kind regards,
Jens

1 Like
4

Hi @bannmann,

Don’t worry, challenging our decisions is perfectly ok and it enables us to improve and clarify our approach. In this case my first answer should have explained better why I don’t think that this rule can be included as it is.

Regarding point 1., forget what I said about Sonar Way. Let’s talk more about the value of the rule from different angles.

False Positives: Let’s suppose that we are in a codebase with no bitwise operators. After a review developers accepted that some algorithm should be optimized using bitwise operators. A common use case is to use bitwise operators with some flags (example in android code). The first time an issue is raised it correctly asks developers to review the code. However when new flags are added and the algorithm has to be modified, every additional issue is a False Positive because the algorithm was already accepted. The only solution is to disable the rule for the whole file and hope that it won’t spread in other files.

User experience: Rules which ask for a review from developers should be extremely rare. In our experience developers stop to look at issues when too many of them are not actual issues to fix. It is sometimes acceptable to have a rule raising a warning when the impact of the issue is extremely high (ex: S4926 “serialVersionUID” should not be declared blindly where an ID can make serialization fail). However it is less justifiable to add this burden on developers when the impact is not clear, such as a sign of potential early optimization.
Even for Security Hotspot rules, which are designed to ask developers to review their code when it is security sensitive, we strive to be as precise as possible in order to reduce the number of issues. When a Security Hotspot rule raises too many issues on safe code we simply remove the rule.

But we could add the rule. People are free to enable it when they want. Every rule in SonarQube is maintained and migrated when we refactor our code. For example it took months to migrate every rule when we made SonarJava rely on the Eclipse frontend (available in SonarJava 6.0) and the migration of rules is still in progress for SonarJS after we moved to ESLint frontend last year. There is also the cost of constantly evaluating and testing rules at SonarSource as we improve them. Thus we have sometime to refuse rules when their value seems lower than the cost of having an additional rule. This is obviously subjective and we try to be as transparent as possible.

Even rules which have already been implemented can be removed when their value is too low. For example we started discussing the value of S2039 Member variable visibility should be specified some time ago and it will be probably deprecated and later removed.

I would recommend to use the custom rules API, which was specifically designed for users who want a rule which we can’t accept in the default rule sets.

Does this make sense to you?

2 Likes
5

Hi Nicolas!

Yes, indeed. I could argue with some details of the points you raised, but your overall conclusion would still be reasonable and justified. Thanks for explaining the rationale in more detail!

Unfortunately, unless I’m mistaken, one does not have that option on SonarCloud.

Being a rather small company, we have a very strong preference to rely on cloud services instead of maintaining our own servers wherever possible. I would not want us to move to a self-hosted SonarQube instance unless there is a truly huge advantage. And as much as I would like to activate an “avoid bitwise operators” rule in my company, that is not at all important enough to warrant a move and the subsequent maintenance burden.

That said, since adopting SonarCloud, we have encountered quite a few cases where closed-source code had specific mistakes and undesired code patterns, but was too specific to the respective company/team to become a SonarCloud rule. So we would definitively appreciate being able to “inject/upload/connect/webhook/sandbox” a custom rule to SonarCloud in some way. Of course, I have no idea if that would even be possible or whether there are (enough) other SonarCloud subscribers interested in this. Also, I have to admit that I would consider most of my various other feature suggestions to be more important. (Sorry, I couldn’t resist advertising them here :wink: ).

1 Like