[clang-tools-extra] 4cc040f - [clang-tidy] Fix false positive in `readability-else-after-return` on `return` jumped over by `goto` (#186370)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Mar 14 09:29:56 PDT 2026
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);
}
}
More information about the cfe-commits
mailing list