[clang-tidy] Add readability-nested-ifs check

[njames93]

Summary

Add a check to detect nested if statements which can be combined into one by ANDing the conditions.
This reduces indentation making code easier to read.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/readability/nested-ifs.rst

59

Excessive newline.

[njames93]

Remove Excessive Newline.
Hopefully fix the windows test failing.

[njames93]

Fix typo in test file.

[njames93]

Handle cases where the fix is incorrect due to operator precedence.

[LegalizeAdulthood]

How does this check interact with macros that expand to if statements?

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

add_clang_library(clangTidyReadabilityModule
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
   MakeMemberFunctionConstCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
+  NestedIfsCheck.cpp
   NonConstParameterCheck.cpp
   QualifiedAutoCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantAccessSpecifiersCheck.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp

clang-tools-extra/clang-tidy/readability/NestedIfsCheck.h

+//===--- NestedIfsCheck.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_READABILITY_NESTEDIFSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NESTEDIFSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Detects nested `if` statements that could be merged into one by ANDing the
+/// conditions.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/nested-ifs.html
+class NestedIfsCheck : public ClangTidyCheck {
+public:
+  NestedIfsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NESTEDIFSCHECK_H

clang-tools-extra/clang-tidy/readability/NestedIfsCheck.cpp

+//===--- NestedIfsCheck.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 "NestedIfsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+
+class NestedIfVisitor : public RecursiveASTVisitor<NestedIfVisitor> {
+  using Base = RecursiveASTVisitor<NestedIfVisitor>;
+  NestedIfsCheck &Check;
+  ASTContext &Ctx;
+
+public:
+  NestedIfVisitor(NestedIfsCheck &Check, ASTContext &Ctx)
+      : Check(Check), Ctx(Ctx) {}
+
+  void traverse() { TraverseAST(Ctx); }
+
+  static bool isCanditateIf(const IfStmt *If, bool IsFirstConstexpr,
+                            bool Outer = false) {
+    if (If->isConsteval())
+      return false;
+    if (!Outer && IsFirstConstexpr != If->isConstexpr())
+      return false;
+    if (If->hasElseStorage() || If->hasVarStorage())
+      return false;
+    if (!If->getThen())
+      return false;
+    // The first `if` in the chain can have an init statement, but any nested
+    // ones cannot.
+    return Outer || !If->hasInitStorage();
+  }
+
+  bool captureNested(IfStmt *If, SmallVectorImpl<const IfStmt *> &Capture,
+                     DataRecursionQueue *Queue) {
+    auto AllowConstexpr = If->isConstexpr();
+    while (true) {
+      auto *Nested = dyn_cast<IfStmt>(If->getThen());
+      if (!Nested) {
+        if (auto *CS = dyn_cast<CompoundStmt>(If->getThen())) {
+          if (CS->size() == 1)
+            Nested = dyn_cast<IfStmt>(CS->body_front());
+        }
+      }
+      if (!Nested || !isCanditateIf(Nested, AllowConstexpr))
+        return Base::TraverseIfStmt(If, Queue);
+
+      Capture.push_back(Nested);
+      If = Nested;
+    }
+    return true;
+  }
+
+  bool TraverseIfStmt(IfStmt *If, DataRecursionQueue *Queue = nullptr) {
+    if (!isCanditateIf(If, false, true))
+      return Base::TraverseIfStmt(If, Queue);
+    SmallVector<const IfStmt *, 3> Diagnose{If};
+    auto Result = captureNested(If, Diagnose, Queue);
+    if (Diagnose.size() == 1)
+      return Result;
+    auto D = Check.diag(If->getBeginLoc(), "Nested ifs can be combined");
+    for (const IfStmt *Item : Diagnose)
+      D << SourceRange(Item->getBeginLoc(), Item->getRParenLoc());
+    SmallString<128> CondAppend;
+    for (auto P = Diagnose.begin(), C = std::next(P), E = Diagnose.end();
+         C != E; P = C++) {
+      const IfStmt *Prev = *P;
+      if (const auto *CS = dyn_cast<CompoundStmt>(Prev->getThen()))
+        D << FixItHint::CreateRemoval(CS->getLBracLoc())
+          << FixItHint::CreateRemoval(CS->getRBracLoc());
+      const IfStmt *Cur = *C;
+      CondAppend.append(
+          {" && ",
+           Lexer::getSourceText(
+               CharSourceRange::getTokenRange(Cur->getCond()->getSourceRange()),
+               Ctx.getSourceManager(), Ctx.getLangOpts())});
+      D << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+          {Cur->getBeginLoc(), Cur->getRParenLoc()}));
+    }
+    D << FixItHint::CreateInsertion(If->getRParenLoc(), CondAppend);
+
+    return Result;
+  }
+};
+} // namespace
+
+void NestedIfsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(translationUnitDecl(), this);
+}
+
+void NestedIfsCheck::check(const MatchFinder::MatchResult &Result) {
+  NestedIfVisitor(*this, *Result.Context).traverse();
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "IsolateDeclarationCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MakeMemberFunctionConstCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
+#include "NestedIfsCheck.h"
 #include "NonConstParameterCheck.h"
 #include "QualifiedAutoCheck.h"
 #include "RedundantAccessSpecifiersCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantPreprocessorCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MagicNumbersCheck>(
         "readability-magic-numbers");
     CheckFactories.registerCheck<MakeMemberFunctionConstCheck>(
         "readability-make-member-function-const");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<NestedIfsCheck>("readability-nested-ifs");
     CheckFactories.registerCheck<QualifiedAutoCheck>(
         "readability-qualified-auto");
     CheckFactories.registerCheck<RedundantAccessSpecifiersCheck>(
         "readability-redundant-access-specifiers");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`readability-nested-ifs
+  <clang-tidy/checks/readability/nested-ifs>` check.
+
+  Detects nested ``if`` statements that could be merged into one by ANDing the
+  conditions.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Removed checks
 ^^^^^^^^^^^^^^

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

.. csv-table::
    `readability-implicit-bool-conversion <readability/implicit-bool-conversion.html>`_, "Yes"
    `readability-inconsistent-declaration-parameter-name <readability/inconsistent-declaration-parameter-name.html>`_, "Yes"
    `readability-isolate-declaration <readability/isolate-declaration.html>`_, "Yes"
    `readability-magic-numbers <readability/magic-numbers.html>`_,
    `readability-make-member-function-const <readability/make-member-function-const.html>`_, "Yes"
    `readability-misleading-indentation <readability/misleading-indentation.html>`_,
    `readability-misplaced-array-index <readability/misplaced-array-index.html>`_, "Yes"
    `readability-named-parameter <readability/named-parameter.html>`_, "Yes"
+   `readability-nested-ifs <readability/nested-ifs.html>`_, "Yes"
    `readability-non-const-parameter <readability/non-const-parameter.html>`_, "Yes"
    `readability-qualified-auto <readability/qualified-auto.html>`_, "Yes"
    `readability-redundant-access-specifiers <readability/redundant-access-specifiers.html>`_, "Yes"
    `readability-redundant-control-flow <readability/redundant-control-flow.html>`_, "Yes"
    `readability-redundant-declaration <readability/redundant-declaration.html>`_, "Yes"
    `readability-redundant-function-ptr-dereference <readability/redundant-function-ptr-dereference.html>`_, "Yes"
    `readability-redundant-member-init <readability/redundant-member-init.html>`_, "Yes"
    `readability-redundant-preprocessor <readability/redundant-preprocessor.html>`_,

clang-tools-extra/docs/clang-tidy/checks/readability/nested-ifs.rst

+.. title:: clang-tidy - readability-nested-ifs
+
+readability-nested-ifs
+======================
+
+Detects nested ``if`` statements that could be merged into one by ANDing the  
+conditions.
+
+This transformation requires that the outer ``if`` contains a single ``if`` as
+its then branch. Both ``if`` statements cannot have an ``else`` branch, 
+condition variable or have a ``consteval`` specifier.
+
+Example:
+
+.. code-block:: c++
+
+  void foo() {
+    if (Condition1) {
+      if (Condition2) {
+        Process();
+      }
+    }
+  }
+
+Would be transformed to:
+
+.. code-block:: c++
+
+  void foo() {
+    if (Condition1 && Condition2) {
+      Process();
+    }
+  }
+
+This will also recurse to enable merging `N` nested ifs.
+
+.. code-block:: c++
+
+  void foo() {
+    if (Condition1) {
+      if (Condition...) {
+        if (ConditionN) {
+          Process()
+        }
+      }
+    }
+  }
+
+Would be transformed to:
+
+.. code-block:: c++
+
+  void foo() {
+    if (Condition1 && Condition... && ConditionN) {
+      Process()
+    }
+  }

clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs-cxx17.cpp

+// RUN: %check_clang_tidy -std=c++17-or-later %s readability-nested-ifs %t -- -- -fno-delayed-template-parsing
+
+// Ensure the check handles if initializers and constexpr if statements
+// correctly.
+
+void foo();
+void bar();
+
+bool cond(int X = 0);
+
+void good() {
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined
+  if (bool X = cond(); X) {
+    if (cond()) {
+      if (cond(1))
+        bar();
+    }
+  } // End
+  // CHECK-FIXES: if (bool X = cond(); X && cond() && cond(1))
+  // CHECK-FIXES-NEXT: {{    }}
+  // CHECK-FIXES-NEXT: {{      }}
+  // CHECK-FIXES-NEXT: bar();
+  // CHECK-FIXES-NEXT: {{    }}
+  // CHECK-FIXES-NEXT: // End
+
+  // The initializer doesn't have to be a variable declaration.
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined
+  if (foo(); true) {
+    if (cond())
+      bar();
+  }
+}
+
+void bad() {
+  // Only the outermost `if` statement can have an initializer.
+  if (bool X = cond(); X) {
+    if (bool Y = cond(1); Y) {
+      bar();
+    }
+  }
+
+  if (cond()) {
+    if (bool X = cond(1); X) {
+      bar();
+    }
+  }
+}
+
+template <bool B, bool C> void constexprIfs() {
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined
+  if constexpr (B) {
+    if constexpr (C) {
+      bar();
+    }
+  } // End
+  // CHECK-FIXES: if constexpr (B && C)
+  // CHECK-FIXES-NEXT: {
+  // CHECK-FIXES-NEXT: bar();
+  // CHECK-FIXES-NEXT: }
+  // CHECK-FIXES-NEXT: // End
+
+  // constexpr ifs are only converted if both ifs are constexpr.
+  if constexpr (B) {
+    if (C) {
+      bar();
+    }
+  }
+
+  if (B) {
+    if constexpr (C) {
+      bar();
+    }
+  }
+}

clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs.cpp

+// RUN: %check_clang_tidy %s readability-nested-ifs %t
+
+void foo();
+void bar();
+
+bool cond(int X = 0);
+
+void good() {
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined
+  if (cond()) {
+    if (cond(1)) {
+      foo();
+      bar();
+    }
+  } // End
+  //      CHECK-FIXES: if (cond() && cond(1))
+  // CHECK-FIXES-NEXT: {
+  // CHECK-FIXES-NEXT:   foo();
+  // CHECK-FIXES-NEXT:   bar();
+  // CHECK-FIXES-NEXT: }
+  // CHECK-FIXES-NEXT: // End
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: Nested ifs can be combined
+  // CHECK-MESSAGES: :[[@LINE+4]]:7: warning: Nested ifs can be combined
+  if (cond()) {
+    if (cond(1)) {
+      foo();
+      if (cond(2)) {
+        if (cond(3)) {
+          bar();
+        }
+      }
+    }
+  } // End
+
+  //      CHECK-FIXES:  if (cond() && cond(1))
+  // CHECK-FIXES-NEXT:    {
+  // CHECK-FIXES-NEXT:      foo();
+  // CHECK-FIXES-NEXT:      if (cond(2) && cond(3))
+  // CHECK-FIXES-NEXT:        {
+  // CHECK-FIXES-NEXT:          bar();
+  // CHECK-FIXES-NEXT:        }
+  // CHECK-FIXES-NEXT: {{     }}
+  // CHECK-FIXES-NEXT:    }
+  // CHECK-FIXES-NEXT:  // End
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:5: warning: Nested ifs can be combined
+  if (bool B = cond()) {
+    if (cond(1)) {
+      if (cond(2)) {
+        foo();
+      }
+    }
+  } // End
+
+  //      CHECK-FIXES:  if (bool B = cond()) {
+  // CHECK-FIXES-NEXT:    if (cond(1) && cond(2))
+  // CHECK-FIXES-NEXT:        {
+  // CHECK-FIXES-NEXT:          foo();
+  // CHECK-FIXES-NEXT:        }
+  // CHECK-FIXES-NEXT: {{    }}
+  // CHECK-FIXES-NEXT:  } // End
+}
+
+void bad() {
+  // Condition variable on either nesting can't be converted.
+  if (bool B = cond())
+    if (cond())
+      foo();
+  if (cond()) {
+    if (bool B = cond())
+      bar();
+  }
+
+  // Can only have a single if statement as the body of the outer if.
+  if (cond()) {
+    foo();
+    if (cond())
+      bar();
+  }
+  if (cond()) {
+    if (cond())
+      foo();
+    bar();
+  }
+
+  // Can't have an else statement on either if.
+  if (cond()) {
+    if (cond())
+      foo();
+    else
+      bar();
+  }
+
+  if (cond()) {
+    if (cond())
+      foo();
+  } else {
+    bar();
+  }
+}