[Clang-Tidy] New check `bugprone-redundant-branch-condition`

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.

Differential Revision: https://reviews.llvm.org/D81272

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -38,6 +38,7 @@
 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
+#include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
@@ -119,6 +120,8 @@ public:
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<RedundantBranchConditionCheck>(
+        "bugprone-redundant-branch-condition");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -33,6 +33,7 @@ add_clang_library(clangTidyBugproneModule
   NotNullTerminatedResultCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
+  RedundantBranchConditionCheck.cpp
   ReservedIdentifierCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp

clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp

@@ -0,0 +1,153 @@
+//===--- RedundantBranchConditionCheck.cpp - clang-tidy -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantBranchConditionCheck.h"
+#include "../utils/Aliasing.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+using clang::tidy::utils::hasPtrOrReferenceInFunc;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+static const char CondVarStr[] = "cond_var";
+static const char OuterIfStr[] = "outer_if";
+static const char InnerIfStr[] = "inner_if";
+static const char FuncStr[] = "func";
+
+/// Returns whether `Var` is changed in `S` before `NextS`.
+static bool isChangedBefore(const Stmt *S, const Stmt *NextS,
+                            const VarDecl *Var, ASTContext *Context) {
+  ExprMutationAnalyzer MutAn(*S, *Context);
+  const auto &SM = Context->getSourceManager();
+  const Stmt *MutS = MutAn.findMutation(Var);
+  return MutS &&
+         SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc());
+}
+
+void RedundantBranchConditionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ImmutableVar =
+      varDecl(anyOf(parmVarDecl(), hasLocalStorage()), hasType(isInteger()),
+              unless(hasType(isVolatileQualified())))
+          .bind(CondVarStr);
+  Finder->addMatcher(
+      ifStmt(
+          hasCondition(ignoringParenImpCasts(anyOf(
+              declRefExpr(hasDeclaration(ImmutableVar)),
+              binaryOperator(hasOperatorName("&&"),
+                             hasEitherOperand(ignoringParenImpCasts(declRefExpr(
+                                 hasDeclaration(ImmutableVar)))))))),
+          hasThen(hasDescendant(
+              ifStmt(hasCondition(ignoringParenImpCasts(
+                         anyOf(declRefExpr(hasDeclaration(
+                                   varDecl(equalsBoundNode(CondVarStr)))),
+                               binaryOperator(
+                                   hasAnyOperatorName("&&", "||"),
+                                   hasEitherOperand(ignoringParenImpCasts(
+                                       declRefExpr(hasDeclaration(varDecl(
+                                           equalsBoundNode(CondVarStr)))))))))))
+                  .bind(InnerIfStr))),
+          forFunction(functionDecl().bind(FuncStr)))
+          .bind(OuterIfStr),
+      this);
+  // FIXME: Handle longer conjunctive and disjunctive clauses.
+}
+
+void RedundantBranchConditionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *OuterIf = Result.Nodes.getNodeAs<IfStmt>(OuterIfStr);
+  const auto *InnerIf = Result.Nodes.getNodeAs<IfStmt>(InnerIfStr);
+  const auto *CondVar = Result.Nodes.getNodeAs<VarDecl>(CondVarStr);
+  const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>(FuncStr);
+
+  // If the variable has an alias then it can be changed by that alias as well.
+  // FIXME: could potentially support tracking pointers and references in the
+  // future to improve catching true positives through aliases.
+  if (hasPtrOrReferenceInFunc(Func, CondVar))
+    return;
+
+  if (isChangedBefore(OuterIf->getThen(), InnerIf, CondVar, Result.Context))
+    return;
+
+  auto Diag = diag(InnerIf->getBeginLoc(), "redundant condition %0") << CondVar;
+
+  // For standalone condition variables and for "or" binary operations we simply
+  // remove the inner `if`.
+  const auto *BinOpCond = dyn_cast<BinaryOperator>(InnerIf->getCond());
+  if (isa<DeclRefExpr>(InnerIf->getCond()->IgnoreParenImpCasts()) ||
+      (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
+    SourceLocation IfBegin = InnerIf->getBeginLoc();
+    const Stmt *Body = InnerIf->getThen();
+    const Expr *OtherSide = nullptr;
+    if (BinOpCond) {
+      const auto *LeftDRE =
+          dyn_cast<DeclRefExpr>(BinOpCond->getLHS()->IgnoreParenImpCasts());
+      if (LeftDRE && LeftDRE->getDecl() == CondVar)
+        OtherSide = BinOpCond->getRHS();
+      else
+        OtherSide = BinOpCond->getLHS();
+    }
+
+    SourceLocation IfEnd = Body->getBeginLoc().getLocWithOffset(-1);
+
+    // For compound statements also remove the left brace.
+    if (isa<CompoundStmt>(Body))
+      IfEnd = Body->getBeginLoc();
+
+    // If the other side has side effects then keep it.
+    if (OtherSide && OtherSide->HasSideEffects(*Result.Context)) {
+      SourceLocation BeforeOtherSide =
+          OtherSide->getBeginLoc().getLocWithOffset(-1);
+      SourceLocation AfterOtherSide =
+          Lexer::findNextToken(OtherSide->getEndLoc(), *Result.SourceManager,
+                               getLangOpts())
+              ->getLocation();
+      Diag << FixItHint::CreateRemoval(
+                  CharSourceRange::getTokenRange(IfBegin, BeforeOtherSide))
+           << FixItHint::CreateInsertion(AfterOtherSide, ";")
+           << FixItHint::CreateRemoval(
+                  CharSourceRange::getTokenRange(AfterOtherSide, IfEnd));
+    } else {
+      Diag << FixItHint::CreateRemoval(
+          CharSourceRange::getTokenRange(IfBegin, IfEnd));
+    }
+
+    // For comound statements also remove the right brace at the end.
+    if (isa<CompoundStmt>(Body))
+      Diag << FixItHint::CreateRemoval(
+          CharSourceRange::getTokenRange(Body->getEndLoc(), Body->getEndLoc()));
+
+    // For "and" binary operations we remove the "and" operation with the
+    // condition variable from the inner if.
+  } else {
+    const auto *CondOp = cast<BinaryOperator>(InnerIf->getCond());
+    const auto *LeftDRE =
+        dyn_cast<DeclRefExpr>(CondOp->getLHS()->IgnoreParenImpCasts());
+    if (LeftDRE && LeftDRE->getDecl() == CondVar) {
+      SourceLocation BeforeRHS =
+          CondOp->getRHS()->getBeginLoc().getLocWithOffset(-1);
+      Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+          CondOp->getLHS()->getBeginLoc(), BeforeRHS));
+    } else {
+      SourceLocation AfterLHS =
+          Lexer::findNextToken(CondOp->getLHS()->getEndLoc(),
+                               *Result.SourceManager, getLangOpts())
+              ->getLocation();
+      Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+          AfterLHS, CondOp->getRHS()->getEndLoc()));
+    }
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.h

@@ -0,0 +1,35 @@
+//===--- RedundantBranchConditionCheck.h - clang-tidy -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_REDUNDANTBRANCHCONDITIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_REDUNDANTBRANCHCONDITIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds condition variables in nested `if` statements that were also checked
+/// in the outer `if` statement and were not changed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-redundant-branch-condition.html
+class RedundantBranchConditionCheck : public ClangTidyCheck {
+public:
+  RedundantBranchConditionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_REDUNDANTBRANCHCONDITIONCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -67,8 +67,11 @@ The improvements are...
 Improvements to clang-tidy
 --------------------------
 
-Changes in existing checks
-^^^^^^^^^^^^^^^^^^^^^^^^^^
+- New :doc:`bugprone-redundant-branch-condition
+  <clang-tidy/checks/bugprone-redundant-branch-condition>` check.
+
+  Finds condition variables in nested ``if`` statements that were also checked
+  in the outer ``if`` statement and were not changed.
 
 - New :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
@@ -76,6 +79,9 @@ Changes in existing checks
   Finds member initializations in the constructor body which can be placed into
   the initialization list instead.
 
+Changes in existing checks
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-redundant-branch-condition.rst

@@ -0,0 +1,104 @@
+.. title:: clang-tidy - bugprone-redundant-branch-condition
+
+bugprone-redundant-branch-condition
+===================================
+
+Finds condition variables in nested ``if`` statements that were also checked in
+the outer ``if`` statement and were not changed.
+
+Simple example:
+
+.. code-block:: c
+
+  bool onFire = isBurning();
+  if (onFire) {
+    if (onFire)
+      scream();
+  }
+
+Here `onFire` is checked both in the outer ``if`` and the inner ``if`` statement
+without a possible change between the two checks. The check warns for this code
+and suggests removal of the second checking of variable `onFire`.
+
+The checker also detects redundant condition checks if the condition variable
+is an operand of a logical "and" (``&&``) or a logical "or" (``||``) operator:
+
+.. code-block:: c
+
+  bool onFire = isBurning();
+  if (onFire) {
+    if (onFire && peopleInTheBuilding > 0)
+      scream();
+  }
+
+.. code-block:: c
+
+  bool onFire = isBurning();
+  if (onFire) {
+    if (onFire || isCollapsing())
+      scream();
+  }
+
+In the first case (logical "and") the suggested fix is to remove the redundant
+condition variable and keep the other side of the ``&&``. In the second case
+(logical "or") the whole ``if`` is removed similarily to the simple case on the
+top.
+
+The condition of the outer ``if`` statement may also be a logical "and" (``&&``)
+expression:
+
+.. code-block:: c
+
+  bool onFire = isBurning();
+  if (onFire && fireFighters < 10) {
+    if (someOtherCondition()) {
+      if (onFire)
+        scream();
+    }
+  }
+
+The error is also detected if both the outer statement is a logical "and"
+(``&&``) and the inner statement is a logical "and" (``&&``) or "or" (``||``).
+The inner ``if`` statement does not have to be a direct descendant of the outer
+one.
+
+No error is detected if the condition variable may have been changed between the
+two checks:
+
+.. code-block:: c
+
+  bool onFire = isBurning();
+  if (onFire) {
+    tryToExtinguish(onFire);
+    if (onFire && peopleInTheBuilding > 0)
+      scream();
+  }
+
+Every possible change is considered, thus if the condition variable is not
+a local variable of the function, it is a volatile or it has an alias (pointer
+or reference) then no warning is issued.
+
+Known limitations
+^^^^^^^^^^^^^^^^^
+
+The ``else`` branch is not checked currently for negated condition variable:
+
+  bool onFire = isBurning();
+  if (onFire) {
+    scream();
+  } else {
+    if (!onFire) {
+      continueWork();
+    }
+  }
+
+The checker currently only detects redundant checking of single condition
+variables. More complex expressions are not checked:
+
+.. code-block:: c
+
+  if (peopleInTheBuilding == 1) {
+    if (peopleInTheBuilding == 1) {
+      doSomething();
+    }
+  }

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

@@ -70,10 +70,11 @@ Clang-Tidy Checks
    `bugprone-misplaced-widening-cast <bugprone-misplaced-widening-cast.html>`_,
    `bugprone-move-forwarding-reference <bugprone-move-forwarding-reference.html>`_, "Yes"
    `bugprone-multiple-statement-macro <bugprone-multiple-statement-macro.html>`_,
-   `bugprone-no-escape <bugprone-no-escape.html>`_, "Yes"
+   `bugprone-no-escape <bugprone-no-escape.html>`_,
    `bugprone-not-null-terminated-result <bugprone-not-null-terminated-result.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone-parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone-posix-return.html>`_, "Yes"
+   `bugprone-redundant-branch-condition <bugprone-redundant-branch-condition.html>`_, "Yes"
    `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
    `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone-sizeof-container.html>`_,