[clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute

[PiotrZSL]

Summary

Ignore duplicated switch cases with [[fallthrough]] attribute to reduce false positives.

Fixes: #47588

[PiotrZSL]

Rebase, Ping

[donat.nagy]

Mostly LGTM, I suggested two cosmetic changes.

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

55

Bikeshedding: these comments are very useful, but put them after the return statements. The (meaningful content) / (number of lines) ratio is already abysmal in visitor classes like this, let's try to avoid what we can.

84

If I understand it correctly, Class::classof(Pointer) is an implementation detail of the more flexible and slightly magical isa<Class>(Obj) utility. The clang-tidy codebase contains lots of isa<> and only a single direct reference to classof() so I'd suggest using isa<> here for the sake of consistency.

[PiotrZSL]

Correct review comments

[donat.nagy]

LGTM.

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

 //===--- BranchCloneCheck.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 "BranchCloneCheck.h"
 #include "../utils/ASTUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/CloneDetection.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/Casting.h"
 
 using namespace clang;
 using namespace clang::ast_matchers;
 
 namespace {
 /// A branch in a switch may consist of several statements; while a branch in
 /// an if/else if/else chain is one statement (which may be a CompoundStmt).
 using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
 } // anonymous namespace
 
 /// Determines if the bodies of two branches in a switch statements are Type I
 /// clones of each other. This function only examines the body of the branch
 /// and ignores the `case X:` or `default:` at the start of the branch.
-static bool areSwitchBranchesIdentical(const SwitchBranch LHS,
-                                       const SwitchBranch RHS,
+static bool areSwitchBranchesIdentical(const SwitchBranch &LHS,
+                                       const SwitchBranch &RHS,
                                        const ASTContext &Context) {
   if (LHS.size() != RHS.size())
     return false;
 
   for (size_t I = 0, Size = LHS.size(); I < Size; I++) {
     // NOTE: We strip goto labels and annotations in addition to stripping
     // the `case X:` or `default:` labels, but it is very unlikely that this
     // would cause false positives in real-world code.
     if (!tidy::utils::areStatementsIdentical(LHS[I]->stripLabelLikeStatements(),
                                              RHS[I]->stripLabelLikeStatements(),
                                              Context)) {
       return false;
     }
   }
 
   return true;
 }
 
+static bool isFallthroughSwitchBranch(const SwitchBranch &Branch) {
+  struct SwitchCaseVisitor : RecursiveASTVisitor<SwitchCaseVisitor> {
+    using RecursiveASTVisitor<SwitchCaseVisitor>::DataRecursionQueue;
+
+    bool TraverseLambdaExpr(LambdaExpr *, DataRecursionQueue * = nullptr) {
+      return true; // Ignore lambdas
+    }
+
+    bool TraverseDecl(Decl *) {
+      return true; // No need to check declarations
+    }
+
+    bool TraverseSwitchStmt(SwitchStmt *, DataRecursionQueue * = nullptr) {
+      return true; // Ignore sub-switches
+    }
+
+    bool TraverseSwitchCase(SwitchCase *, DataRecursionQueue * = nullptr) {
+      return true; // Ignore cases
+    }
+
+    bool TraverseDefaultStmt(DefaultStmt *, DataRecursionQueue * = nullptr) {
+      return true; // Ignore defaults
+    }
+
+    bool TraverseAttributedStmt(AttributedStmt *S) {
+      if (!S)
+        return true;
+
+      for (const Attr *A : S->getAttrs()) {
+        if (isa<FallThroughAttr>(A))
+          return false;
+      }
+
+      return true;
+    }
+  } Visitor;
+
+  for (const Stmt *Elem : Branch) {
+    if (!Visitor.TraverseStmt(const_cast<Stmt *>(Elem)))
+      return true;
+  }
+  return false;
+}
+
 namespace clang::tidy::bugprone {
 
 void BranchCloneCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       ifStmt(unless(allOf(isConstexpr(), isInTemplateInstantiation())),
              stmt().bind("if"),
              hasParent(stmt(unless(ifStmt(hasElse(equalsBoundNode("if")))))),
              hasElse(stmt().bind("else"))),
for (const Stmt *S : Body->body()) {
       // store those statements in branches.
       if (!Branches.empty())
         Branches.back().push_back(S);
     }
 
     auto *End = Branches.end();
     auto *BeginCurrent = Branches.begin();
     while (BeginCurrent < End) {
+      if (isFallthroughSwitchBranch(*BeginCurrent)) {
+        ++BeginCurrent;
+        continue;
+      }
+
       auto *EndCurrent = BeginCurrent + 1;
       while (EndCurrent < End &&
              areSwitchBranchesIdentical(*BeginCurrent, *EndCurrent, Context)) {
         ++EndCurrent;
       }
       // At this point the iterator range {BeginCurrent, EndCurrent} contains a
       // complete family of consecutive identical branches.
-      if (EndCurrent > BeginCurrent + 1) {
-        diag(BeginCurrent->front()->getBeginLoc(),
-             "switch has %0 consecutive identical branches")
-            << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
+
+      if (EndCurrent == (BeginCurrent + 1)) {
+        // No consecutive identical branches that start on BeginCurrent
+        BeginCurrent = EndCurrent;
+        continue;
+      }
+
 
-        SourceLocation EndLoc = (EndCurrent - 1)->back()->getEndLoc();
-        // If the case statement is generated from a macro, it's SourceLocation
-        // may be invalid, resulting in an assertion failure down the line.
-        // While not optimal, try the begin location in this case, it's still
-        // better then nothing.
-        if (EndLoc.isInvalid())
-          EndLoc = (EndCurrent - 1)->back()->getBeginLoc();
-
-        if (EndLoc.isMacroID())
-          EndLoc = Context.getSourceManager().getExpansionLoc(EndLoc);
-        EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, *Result.SourceManager,
-                                            getLangOpts());
-
-        if (EndLoc.isValid()) {
-          diag(EndLoc, "last of these clones ends here", DiagnosticIDs::Note);
-        }
-      }
       BeginCurrent = EndCurrent;
     }
     return;
   }
 
   llvm_unreachable("No if statement and no switch statement.");
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New alias :doc:`cppcoreguidelines-use-default-member-init
   <clang-tidy/checks/cppcoreguidelines/use-default-member-init>` to
   :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+- Fixed false-positive in :doc:`bugprone-branch-clone
+  <clang-tidy/checks/bugprone/branch-clone>` check by ignoring duplicated
+  switch cases marked with the ``[[fallthrough]]`` attribute.
+
 - Improved :doc:`readability-redundant-string-cstr
   <clang-tidy/checks/readability/redundant-string-cstr>` check to recognise
   unnecessary ``std::string::c_str()`` and ``std::string::data()`` calls in
   arguments to ``std::print``, ``std::format`` or other functions listed in
   the ``StringParameterFunction`` check option.
 
 - Improved :doc:`bugprone-dangling-handle
   <clang-tidy/checks/bugprone/dangling-handle>` check enhancing detection of

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

.. code-block:: c++
     default:
       return 10;
     }
 
 Here the check does not warn for the repeated ``return 10;``, which is good if
 we want to preserve that ``'a'`` is before ``'b'`` and ``default:`` is the last
 branch.
 
+Switch cases marked with the ``[[fallthrough]]`` attribute are ignored.
+
 Finally, the check also examines conditional operators and reports code like:
 
 .. code-block:: c++
 
     return test_value(x) ? x : x;
 
 Unlike if statements, the check does not detect chains of conditional
 operators.
 
 Note: This check also reports situations where branches become identical only
 after preprocessing.

clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-fallthrough.cpp

+// RUN: %check_clang_tidy %s bugprone-branch-clone %t -- -- -std=c++17
+
+void handle(int);
+
+void testSwitchFallthroughAttribute(int value) {
+  switch(value) {
+    case 1: [[fallthrough]];
+    case 2: [[fallthrough]];
+    case 3:
+      handle(value);
+      break;
+    default:
+      break;
+  }
+}
+
+void testSwitchFallthroughAttributeAndBraces(int value) {
+  switch(value) {
+    case 1: { [[fallthrough]]; }
+    case 2: { [[fallthrough]]; }
+    case 3: {
+      handle(value);
+      break;
+    }
+    default: {
+      break;
+    }
+  }
+}
+
+void testSwitchWithFallthroughAttributeAndCode(int value) {
+  switch(value) {
+    case 1: value += 1; [[fallthrough]];
+    case 2: value += 1; [[fallthrough]];
+    case 3:
+      handle(value);
+      break;
+    default:
+      break;
+  }
+}
+
+void testSwitchWithFallthroughAndCode(int value) {
+  switch(value) {
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
+    case 1: value += 1;
+    case 2: value += 1;
+    // CHECK-MESSAGES: :[[@LINE-1]]:23: note: last of these clones ends here
+    case 3:
+      handle(value);
+      break;
+    default:
+      break;
+  }
+}
+
+void testSwitchFallthroughAttributeIntoDefault(int value) {
+  switch(value) {
+    case 1: [[fallthrough]];
+    case 2: [[fallthrough]];
+    default:
+      handle(value);
+      break;
+  }
+}