[PATCH] D81272: [clang-tidy] New check `misc-redundant-condition`

Thu Aug 13 08:18:51 PDT 2020

aaron.ballman added a comment.

Poking @alexfh for more opinions about check similarity.

================
Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:46
+    CheckFactories.registerCheck<RedundantConditionCheck>(
+        "misc-redundant-condition");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
----------------
baloghadamsoftware wrote:
> aaron.ballman wrote:
> > baloghadamsoftware wrote:
> > > aaron.ballman wrote:
> > > > baloghadamsoftware wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this check should probably live in the `bugprone` module, WDYT?
> > > > > 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`.
> > > > 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).
> > > 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?
> > > 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).
> 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.
> 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?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81272/new/

https://reviews.llvm.org/D81272