Page MenuHomePhabricator

[clang-tidy] New check `bugprone-redundant-branch-condition`
ClosedPublic

Authored by baloghadamsoftware on Jun 5 2020, 9:20 AM.

Details

Summary

Checking the same condition again in a nested if usually make no sense, except if the value of the expression could have been changed between the two checks. Although compilers may optimize this out, such code is suspicious: the programmer may have meant to check something else. Therefore it is worth to find such places in the code and notify the user about the problem.

This patch implements a basic check for this problem. Currently it only detects redundant conditions where the condition is a variable of integral type. It also detects the possible bug if the variable is in an or or and logical expression in the inner if and/or the variable is in an and logical expression in the outer if statement. Negated cases are not handled yet.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 9:20 AM
baloghadamsoftware marked an inline comment as done.Jun 5 2020, 9:25 AM

This check was made upon user request. I think it is a good base that can later be extended also for the negated cases (where the code inside the inner if never executes). That case is even more suspicious. I put it into misc because it is not such a bug that it should go into bugprone but rather a suspicious thing. The type of the bug is somewhat similar to the bugs detected by misc-redundant-expression so I think misc is the best place also for this one.

clang-tools-extra/docs/clang-tidy/checks/list.rst
137

What are these changes? I surely did not make them? Maybe the check adder Python script?

baloghadamsoftware retitled this revision from [Clang-Tidy] New check `misc-redundant-condition` to [clang-tidy] New check `misc-redundant-condition`.Jun 5 2020, 9:27 AM

I feel the refactoring of Aliasing should be in its own PR, with this being a child of it.

clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
39

Small nit, but using string literals for bound nodes is error prone. may I suggest defining some static strings and using those to prevent any typos.

40

Recursive matchers are a pain, but this will miss:

if (A && B && ... Y && Z)
64

This logic can be moved to the matcher.

varDecl(anyOf(parmVarDecl(), hasLocalStorage()))
68

Ditto:

varDecl(hasType(isVolatileQualified()))
72

Ditto, you get the point

88–89

use llvm::dyn_cast.

98

Again CharSourceRange::getTokenRange(Body->getEndLoc())

110

use llvm::dyn_cast.

116

That's because you are getting the character range, if you get the token range it'll get the correct range: CharSourceRange::getTokenRange

clang-tools-extra/docs/clang-tidy/checks/list.rst
137

Yeah, just undo those unrelated changes, maybe one day the python script will get sorted

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
70

Please keep alphabetical order in new checks list.

73

Please use double back-ticks for language constructs. Same in documentation.

Updated according to the comments.

baloghadamsoftware marked 11 inline comments as done.Jun 8 2020, 7:33 AM
baloghadamsoftware added inline comments.
clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
40

I suppose that for this purpose we have to make our own matcher? Right? This also makes the fixits more complex. I think this could be done in a later phase. I added a FIXME now.

Eugene.Zelenko added inline comments.Jun 8 2020, 7:45 AM
clang-tools-extra/docs/clang-tidy/checks/misc-redundant-condition.rst
7

Please use double back-ticks for language constructs. Same below.

These test cases don't show it, from what i can see this will erroneously match:

if (onFire) {
  tryPutFireOut();
} else {
  if (isHot && onFire) {
    scream();
  }
}

It appears that this will change the second if to:

if (isHot) {
  scream();
}

Rather than simply deleting the if as it will always evaluate to false. To fix this you could wrap the hasDescendant matcher inside a hasThen matcher.

Another case is if the condition variable is on the RHS of a redundant inner if condition, you can't delete the inner if when the LHS potentially has side effects.

if (onFire) {
  if (callTheFD() || onFire) {
    scream();
  }
}

If callTheFD has side effects this can't be replaced with this:

if (onFire) {
  scream();
}

You could however replace it with:

if (onFire) {
  callTheFD();
  scream();
}

For this simply check if the LHS has side effects.

Thank you for your help, @njames93! I updated the patch according to your comments. (And of course, also thank you, @Eugene.Zelenko, I also incorporated the changes you suggested.)

I tested this check on several open-source projects: BitCoin, CURL, OpenSSL, PostGreS/, TMux/ and Xerces. I only got two warnings issued by this checker, the first one in BitCoin:

src/checkqueue.h:93:25: redundant condition 'fMaster' [misc-redundant-condition]
                        if (fMaster)
                        ^

And indeed, this was a true positive that is fixed by now by this Commit. (I do not update these projects very often since I only use them for testing checks.)

The other one is in PostGreS:

src/backend/access/transam/xlog.c:7231:6: redundant condition 'switchedTLI' [misc-redundant-condition]
          if (switchedTLI && AllowCascadeReplication())
          ^

Althought the line numbers are changed by now, the bug is still there.

Thus the result is zero false positives and two true positives so far.

As a future work, when something support ifs, it should also support ?:, and eliminate redundant conditionals from there, too.

I believe this check (together with misc-redundant-expr) should go into the readability- group.

True positive confirmed in PostGreS: Mailing List Archive. It is fixed by now, so you cannot see it on the link in my previous comment anymore.

6 findings in the LLVM Project. All of them confirmed as trues positives, 5 of them already fixed. Fix pending for the last one.

D82555, D8556, D82557, D82558, D82559, D82563

@aaron.ballman, @gribozavr2 or someone please review this patch.

aaron.ballman added inline comments.Aug 11 2020, 9:28 AM
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

I think this check should probably live in the bugprone module, WDYT?

clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
33

This code can be simplified into:

const Stmt *MutS = MutAn.findMutation(Var);
return MutS && SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc());
74

This FIXME makes me worried that what we really want is a clang static analyzer check, since the check is already flow sensitive and needs to be data sensitive as well. Have you considered implementing this as a static analyzer check?

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp
1098

Can you add some tests that exercise GNU expression statements, just to make sure we get the behavior correct?

if (({foo;})) {
} else if (({foo;})) {
}

if (foo) ({bar;});
else if (foo) ({bar;});

Another thing that's interesting to test is whether the redundant expression is in a subexpression which doesn't contribute to the value of the control expression:

if (foo(), val) {
} else if (foo(), other_val) {
}

(Similar can be tested with GNU statement expressions.)

clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
74

Actually, it is only a proposal for improvement. Maybe FIXME is not the best wording for it. This is not a bug to fix, just a possibility to find more true positives. This check is very similar to the infinite loop check which is also in Tidy. In the Static Analyzer this check would be too heavyweight and would probably produce false positives.

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp
1098

Do you mean that I should handle these cases as well? (Detect a bug, provide a fix.)

aaron.ballman added inline comments.Aug 12 2020, 8:33 AM
clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
74

Ah, thank you for clarifying (though I think it's interesting that we come down on opposite sides of the FP question -- I would argue that the static analyzer check would have considerably fewer false positives than the tidy check when handling this case).

If the idea is that pointers and references need to be supported in a future patch, I think FIXME is a fine comment. If the idea is that pointers and references are a potential improvement, I'd probably clarify the comment to something more like FIXME: could potentially support tracking pointers and references in the future to improve catching true positives through aliases.

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp
1098

If the check misses these cases, I think that's reasonable (just comment that the tests aren't currently handled). If the check diagnoses these cases but the fixit causes an issue, then I think that should be corrected in this patch. If it's trivial to support these cases (diagnosing or fixing), it's your call on whether you want to do so. I mostly just want the test coverage so I can know what to expect.

Updated according to the comments.

baloghadamsoftware marked 3 inline comments as done.Aug 13 2020, 3:29 AM
baloghadamsoftware added inline comments.
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

Based on my experience, bugpronbe is for checks whose findings are bugs that lead to undefined illegal memory access, behavior etc. This one is somewhere between that and readability. For example, redundant-expression is also in misc. But if you wish, I can move this checker into bugprone.

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp
1098

The check currently misses these cases completely, so I left a FIXME there. I can do it in the near future, but first I would like to commit a first version so I can tell the user who requested it that his request is fulfilled.

aaron.ballman added inline comments.Aug 13 2020, 5:39 AM
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

The bugprone module has less to do with memory access or undefined behavior specifically and more to do with checks that should expose bugs in your code but don't belong to other categories. We try to keep checks out of misc as much as possible these days and this code pattern is attempting to find cases where the user potentially has a bug, so I think bugprone is the correct home for it.

However, bugprone has a similar check and I sort of wonder whether we should be extending that check rather than adding a separate one. See bugprone-branch-clone which catches the highly related situation where you have a chain of conditionals and one of the conditions is repeated. e.g.,

if (foo) {
  if (foo) { // Caught by misc-redundant-condition
  }
} else if (foo) { // Caught by bugprone-branch-clone
}

Even if we don't combine the checks, we should ensure their behaviors work well together (catch the same scenarios, don't repeat diagnostics, etc).

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-condition.cpp
1098

Sounds great to me, thanks!

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

OK, I will put this into bugprone. The two checks may look similar, but this one is more complex because it does not check for the same condition in multiple branches of the same branch statement but checks whether the condition expression could be mutated between the two branch statements. Therefore the the whole logic is totally different, I see no point in merging the two. Should I create a test case then, where both are enabled?

aaron.ballman added inline comments.Aug 13 2020, 7:16 AM
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

Therefore the the whole logic is totally different, I see no point in merging the two.

I'm approaching the question from the perspective of the user, not a check author. These two checks do the same thing (find redundant conditions in flow control which look like they could be a logical mistake), so why should they be two separate checks? "Because the code looks different" isn't super compelling from that perspective, so I'm trying to figure out what the underlying principles are for the checks. If they're the same principle, they should be the same check. If they're fundamentally different principles, we should be able to explain when to use each check as part of their documentation without it sounding contrived. (Note, I'm not saying the checks have to be combined, but I am pushing back on adding an entirely new check that seems to be redundant from a user perspective.)

As a litmus test: can you think of a situation where you'd want only one of these two checks enabled? I can't think of a case where I'd care about redundancy in nested conditionals but not in chained conditionals (or vice versa) unless one of the checks had a really high false positive rate (which isn't much of a reason to split the checks anyway).

Should I create a test case then, where both are enabled?

If we wind up keeping the checks separate, then probably yes (also, the documentation for the checks should be updated to explain how they're different and why that's useful).

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

There are many checks that users almost always keep enabled together, but they are still separate checks. Now I looked into the branch clone check, combining them means simply copying them together because the logic is so much different.

Even from the user's perspective I see that branches with identical conditions are different from redundant checks. While the first one is a more serious bug (the second branch with the same condition is never executed) this one is slightly more than a readability error.

Poking @alexfh for more opinions about check similarity.

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

There are many checks that users almost always keep enabled together, but they are still separate checks.

I cannot find an instance with two checks that are this strongly related. The closest I can come are some of the C++ Core Guideline checks, but those are a different beast because they're part of a set of guidelines.

Now I looked into the branch clone check, combining them means simply copying them together because the logic is so much different.

This is not a very compelling reason to make a decision to split the checks, to me. We have plenty of checks with complex matchers and checking logic.

Even from the user's perspective I see that branches with identical conditions are different from redundant checks. While the first one is a more serious bug (the second branch with the same condition is never executed) this one is slightly more than a readability error.

I don't view the proposed check as having anything to do with readability. Readability is "how do I make the code do the same thing but look prettier?" and other stylistic choices. This check is finding a case where the programmer has potentially made a logical mistake with their code and is considerably more serious than a matter of style. To me, these are identical problems of programmer confusion.

The more I consider this, the more strongly I feel about combining the checks. I would have a hard time understanding why this code should require two different checks to be enabled to catch what amounts to the same logical confusion:

if (!foo) {
} else if (foo) { // This is a chain of conditionals with a redundant check
}

if (!foo) {
} else {
  if (foo) { // This is not a chain of conditionals, but it still has a redundant check
  }
}

@alexfh do you have thoughts on this?

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

I almost started to copy the two checks together when one of my colleagues told me that bugprone-branch-clone is not at all about redundant conditions but redundant bodies in different branches. (It was created by a formal student intern of our team.) Thus even from the user's perspective these checkers have nothing to do with each other.

baloghadamsoftware retitled this revision from [clang-tidy] New check `misc-redundant-condition` to [clang-tidy] New check `bugprone-redundant-branch-condition`.
baloghadamsoftware edited the summary of this revision. (Show Details)

Check renamed.

It seems that release notes were not renamed automatically. I updated it manually now.

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
46

Whoa, I had not noticed that about bugprone-branch-clone, but I reread the docs and you're absolutely right. Great catch and I'm sorry for the accidental noise.

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

Not even equality is handled yet, just single booleans.

aaron.ballman accepted this revision.Aug 14 2020, 7:16 AM

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

Not even equality is handled yet, just single booleans.

That's what I was understanding too, thanks! LG with additional negative tests + doc changes.

This revision is now accepted and ready to land.Aug 14 2020, 7:16 AM

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

I'm afraid handling such cases will eventually invoke the same problems the RangeConstraintSolver has, and then the discussion about whether this is good in Tidy instead of CSA will be resurrected... 😦

One could come up with even more elaborate cases. Just hit something like below a few minutes ago while working:

const CallExpr* const C;

if (auto* CalledFun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) {
  // ...
  if (const auto* Fun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) {
    // ...
  }
  // ...
}

and the rabbit hole goes deeper. But it's pretty interesting how many cases the current check found as-is.

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

I'm afraid handling such cases will eventually invoke the same problems the RangeConstraintSolver has, and then the discussion about whether this is good in Tidy instead of CSA will be resurrected... 😦

One could come up with even more elaborate cases. Just hit something like below a few minutes ago while working:

const CallExpr* const C;

if (auto* CalledFun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) {
  // ...
  if (const auto* Fun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) {
    // ...
  }
  // ...
}

and the rabbit hole goes deeper. But it's pretty interesting how many cases the current check found as-is.

While I agree with your observation about data and flow sensitivity, I approved on the belief that the check as-is provides enough utility to warrant adding it as-is. If someone wants to improve the check into being a CSA check, we can always deprecate this one at that point. However, if there are strong opinions that the check should start out as a CSA check because it requires that sensitivity for your needs, now's a good time to bring up those concerns.

Tests andd for not yet handled cases. Known limiatations included in the docs.

whisperity added a comment.EditedAug 14 2020, 8:09 AM

While I agree with your observation about data and flow sensitivity, I approved on the belief that the check as-is provides enough utility to warrant adding it as-is. If someone wants to improve the check into being a CSA check, we can always deprecate this one at that point. However, if there are strong opinions that the check should start out as a CSA check because it requires that sensitivity for your needs, now's a good time to bring up those concerns.

It's generally harder to create big logic mistakes when it comes to more complex expressions, assuming the user does't copy-paste (which I might have done, in the above example). We do not need to solve every potentially equivalent conditional (it is unsolvable in the generic case anyways). I'm sure this check can be improved later with handling trivial comparisons (such as standard library int results not being 0, -1, etc.).

Edit: what I'm trying to say is catching cases like foo > 5 and then check for foo > 3 being trivial, or catching foo > 5 and then foo == 5 being impossible is a useful addition later on to this check. But having the ability to catch things like bar >= 2u and then foo > 5u and then if (foo - bar < 4u) is rather extravagant, and highly unneeded. I'm not even sure if directly catching this latter extravagant case (or the thing with cast pointers in my previous example!) by itself is needed... SA could deal with such things inside the engine if and only if their use later down the line results in something terrible (such as giving foo - bar as a file descriptor, etc.).

aaron.ballman accepted this revision.Aug 15 2020, 7:00 AM

While I agree with your observation about data and flow sensitivity, I approved on the belief that the check as-is provides enough utility to warrant adding it as-is. If someone wants to improve the check into being a CSA check, we can always deprecate this one at that point. However, if there are strong opinions that the check should start out as a CSA check because it requires that sensitivity for your needs, now's a good time to bring up those concerns.

It's generally harder to create big logic mistakes when it comes to more complex expressions, assuming the user does't copy-paste (which I might have done, in the above example). We do not need to solve every potentially equivalent conditional (it is unsolvable in the generic case anyways). I'm sure this check can be improved later with handling trivial comparisons (such as standard library int results not being 0, -1, etc.).

Great, then we're on the same page!

The new documentation and test cases LG, so re-approving. Thank you!

Thanks for this checker. FYI, it found (at least) 3 defects in Firefox code (fixed with the autofix):
https://hg.mozilla.org/mozilla-central/rev/651e68f628d0

https://bugzilla.mozilla.org/show_bug.cgi?id=1664747
(unlikely that it was causing any actual bugs in the product)

This comment was removed by sylvestre.ledru.

Thanks for this checker. FYI, it found (at least) 3 defects in Firefox code (fixed with the autofix):
https://hg.mozilla.org/mozilla-central/rev/651e68f628d0

https://bugzilla.mozilla.org/show_bug.cgi?id=1664747
(unlikely that it was causing any actual bugs in the product)

Thank you for your info. These defects do not cause any bug directly, but they make code understanding harder which may cause bugs in the future.