[clang-tidy] Improve bugprone-branch-clone with support for fallthrou…

…gh attribute

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

Fixes: #47588

Reviewed By: donat.nagy

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

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

@@ -9,6 +9,7 @@
 #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"
@@ -26,8 +27,8 @@ using SwitchBranch = llvm::SmallVector<const Stmt *, 2>;
 /// 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;
@@ -46,6 +47,50 @@ static bool areSwitchBranchesIdentical(const SwitchBranch LHS,
   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) {
@@ -182,34 +227,42 @@ void BranchCloneCheck::check(const MatchFinder::MatchResult &Result) {
     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));
-
-        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);
-        }
+
+      if (EndCurrent == (BeginCurrent + 1)) {
+        // No consecutive identical branches that start on BeginCurrent
+        BeginCurrent = EndCurrent;
+        continue;
+      }
+
+      diag(BeginCurrent->front()->getBeginLoc(),
+           "switch has %0 consecutive identical branches")
+          << static_cast<int>(std::distance(BeginCurrent, EndCurrent));
+
+      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;
     }

clang-tools-extra/docs/ReleaseNotes.rst

@@ -201,6 +201,11 @@ New check aliases
 
 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

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

@@ -77,6 +77,8 @@ 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++

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

@@ -0,0 +1,65 @@
+// 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;
+  }
+}