Author: Victor Chernyakin Date: 2026-03-14T09:29:51-07:00 New Revision: 4cc040f86be4f62150efdab6988cba0d54b9f7da
URL: https://github.com/llvm/llvm-project/commit/4cc040f86be4f62150efdab6988cba0d54b9f7da DIFF: https://github.com/llvm/llvm-project/commit/4cc040f86be4f62150efdab6988cba0d54b9f7da.diff LOG: [clang-tidy] Fix false positive in `readability-else-after-return` on `return` jumped over by `goto` (#186370) Given this code: ```cpp if (...) { goto skip_over_return; return; skip_over_return: foo(); } else { ... } ``` ...the check suggests removing the `else`, which is not a valid transformation. This is because it looks at *all* the substatements of the then-branch for interrupting statements. This PR changes it to only look at the *final* substatement. Technically, this introduces a false negative on code like this: ```cpp if (...) { return; dead_code(); } else { // <-- Could in theory remove this 'else' ... } ``` But, that code is objectively bad, so I don't think we're losing anything. This change has the side effect of making the check a bit more general; it now recognizes attributed interrupting statements (e.g. `[[clang::musttail]] return f();`). Added: Modified: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return-cxx20.cpp clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp index 342b57840533d..8e1162ff8b073 100644 --- a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp @@ -45,6 +45,20 @@ AST_MATCHER_P(Stmt, stripLabelLikeStatements, return InnerMatcher.matches(*S, Finder, Builder); } +AST_MATCHER_P(Stmt, hasFinalStmt, ast_matchers::internal::Matcher<Stmt>, + InnerMatcher) { + for (const Stmt *S = &Node;;) { + S = S->stripLabelLikeStatements(); + if (const auto *Compound = dyn_cast<CompoundStmt>(S)) { + if (Compound->body_empty()) + return false; + S = Compound->body_back(); + } else { + return InnerMatcher.matches(*S, Finder, Builder); + } + } +} + } // namespace static constexpr char InterruptingStr[] = "interrupting"; @@ -172,16 +186,13 @@ void ElseAfterReturnCheck::registerPPCallbacks(const SourceManager &SM, } void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) { - const auto InterruptsControlFlow = stmt(anyOf( - returnStmt().bind(InterruptingStr), continueStmt().bind(InterruptingStr), - breakStmt().bind(InterruptingStr), cxxThrowExpr().bind(InterruptingStr), - callExpr(callee(functionDecl(isNoReturn()))).bind(InterruptingStr))); + const auto InterruptsControlFlow = + stmt(anyOf(returnStmt(), continueStmt(), breakStmt(), cxxThrowExpr(), + callExpr(callee(functionDecl(isNoReturn()))))); const auto IfWithInterruptingThenElse = ifStmt(unless(isConstexpr()), unless(isConsteval()), - hasThen(stripLabelLikeStatements( - stmt(anyOf(InterruptsControlFlow, - compoundStmt(has(InterruptsControlFlow)))))), + hasThen(hasFinalStmt(InterruptsControlFlow.bind(InterruptingStr))), hasElse(stmt().bind("else"))) .bind("if"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 4b207609d598d..bdffdb8709405 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -338,6 +338,9 @@ Changes in existing checks - Fixed missed diagnostics when ``if`` statements appear in unbraced ``switch`` case labels. + - Fixed a false positive involving ``if`` statements which contain + a ``return``, ``break``, etc., jumped over by a ``goto``. + - Added support for handling attributed ``if`` then-branches such as ``[[likely]]`` and ``[[unlikely]]``. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return-cxx20.cpp index 589d2b6e2bb68..c83b48dcfdb8d 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return-cxx20.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return-cxx20.cpp @@ -38,4 +38,26 @@ void f() { // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return' // CHECK-FIXES: {{^}} {{[[][[]}}unlikely{{[]][]]}} // comment-4 g(); + + if (false) + [[clang::musttail]] return f(); + else // comment-5 + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} // comment-5 + g(); + + if (false) [[likely]] + [[clang::musttail]] return f(); + else // comment-6 + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} // comment-6 + g(); + + if (false) [[likely]] { + [[clang::musttail]] return f(); + } else { // comment-7 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-7 + g(); + } } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp index 7ab5acfe9d966..1bbbdbc2a5683 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/else-after-return.cpp @@ -437,6 +437,45 @@ void testLabels(bool b) { // CHECK-FIXES: {{^}} // comment-27 f(0); } + + if (true) { + goto skip_over_return; + return; +skip_over_return: + f(0); + } else { + f(0); + } + + if (true) { + goto skip_over_return2; + return; +skip_over_return2: + // No statement after label. Valid since C++23/C23. + } else { + f(0); + } + + if (true) { + goto skip_over_return3; + return; +skip_over_return3: + return; + } else { // comment-28 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-28 + f(0); + } +} + +void testExcessiveBracing() { + if (false) { + {{{ return; }}} + } else { // comment-29 + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + // CHECK-FIXES: {{^}} } // comment-29 + return; + } } [[noreturn]] void noReturn(); @@ -448,18 +487,18 @@ struct NoReturnMember { void testNoReturn() { if (true) { noReturn(); - } else { // comment-28 + } else { // comment-30 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after calling a function that doesn't return - // CHECK-FIXES: {{^}} } // comment-28 + // CHECK-FIXES: {{^}} } // comment-30 f(0); } if (true) { NoReturnMember f; f.noReturn(); - } else { // comment-29 + } else { // comment-31 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after calling a function that doesn't return - // CHECK-FIXES: {{^}} } // comment-29 + // CHECK-FIXES: {{^}} } // comment-31 f(0); } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
