[clang-tidy]: Ignore empty catch blocks in destructors in bugprone-empty-catch check

[negativ]

While flagging empty catch blocks is generally a great rule, it produces false positives for destructors, where this pattern is often the only correct implementation.

The Rationale:

• Destructors are frequently called during stack unwinding after another exception has already been thrown.
• If a destructor itself throws while another exception is active, the C++ runtime immediately calls std::terminate.
• Therefore, to guarantee program stability, any code within a destructor that could potentially throw must be wrapped in a try...catch block.
• Since there's often no adequate way to recover or report an error from a destructor (for e.g. for std::bad_alloc), "swallowing" the exception is the standard/safest approach.

Proposal:

Skip checks for catch blocks within destructors.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Andrey Karlov (negativ)

Changes

While flagging empty catch blocks is generally a great rule, it produces false positives for destructors, where this pattern is often the only correct implementation.

The Rationale:

• Destructors are frequently called during stack unwinding after another exception has already been thrown.
• If a destructor itself throws while another exception is active, the C++ runtime immediately calls std::terminate.
• Therefore, to guarantee program stability, any code within a destructor that could potentially throw must be wrapped in a try...catch block.
• Since there's often no adequate way to recover or report an error from a destructor (for e.g. for std::bad_alloc), "swallowing" the exception is the standard/safest approach.

Proposal:

Skip checks for catch blocks within destructors.

---

Full diff: https://github.com/llvm/llvm-project/pull/161379.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp (+1)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp (+9)

diff --git a/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
index eebab847d1070..48dc3bdfdf49e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
@@ -90,6 +90,7 @@ void EmptyCatchCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       cxxCatchStmt(unless(isExpansionInSystemHeader()), unless(isInMacro()),
                    unless(hasCaughtType(IgnoredExceptionType)),
+                   unless(hasAncestor(cxxDestructorDecl())),
                    hasHandler(compoundStmt(
                        statementCountIs(0),
                        unless(hasAnyTextFromList(IgnoreCatchWithKeywords)))))
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index c3a6d2f9b2890..426c97225c0e1 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -244,6 +244,10 @@ Changes in existing checks
   correcting a spelling mistake on its option
   ``NamePrefixSuffixSilenceDissimilarityTreshold``.
 
+- Improved :doc:`bugprone-empty-catch
+  <clang-tidy/checks/bugprone/empty-catch>` check by allowing empty 
+  ``catch`` blocks in destructors.
+
 - Improved :doc:`bugprone-exception-escape
   <clang-tidy/checks/bugprone/exception-escape>` check's handling of lambdas:
   exceptions from captures are now diagnosed, exceptions in the bodies of
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp
index 8ab38229b6dbf..1319496269d86 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp
@@ -65,3 +65,12 @@ void functionWithComment2() {
     // @IGNORE: relax its safe
   }
 }
+
+struct StructWithEmptyCatchInDestructor {
+  ~StructWithEmptyCatchInDestructor() {
+    try {
+    } 
+    catch (...) {
+    }
+  }
+};

[vbvictor]

I think this should be an option instead.
If there is a global logger in program, it could be used to at least write a warning message that something was caught inside destructor. I believe it's something that people could enforce with this check and disabling such functionality unconditionally is not good IMO.

[negativ]

I think this should be an option instead. If there is a global logger in program, it could be used to at least write a warning message that something was caught inside destructor. I believe it's something that people could enforce with this check and disabling such functionality unconditionally is not good IMO.

That's a fair point, but from my perspective, the logic should actually be inverted.

Instead of enabling this check by default and requiring users to disable it for destructors, the check should be disabled for destructors by default, with an option to turn it on if a user explicitly needs it.

My reasoning is that an empty catch block inside a destructor is almost always a conscious and deliberate decision. The developer who wrote it likely had a very strong reason - for example, the valid concern that even a logging function could throw a second exception and terminate the program.

IMHO, destructors are critical code paths that are already written with heightened attention to their reliability, so we should trust the developer's intent by default in this specific context.

[vbvictor]

Instead of enabling this check by default and requiring users to disable it for destructors, the check should be disabled for destructors by default, with an option to turn it on if a user explicitly needs it.

There is a partial consensus in clang-tidy reviewers (at least carlosgalvezp and me) that we should make checks strict by default with options to be less strict.
The rationale is: if don't like some option - you can disable it easily, but if an option is off-by-default you may not even ever find about it - and this is potentially bad for end-user.

I hope @PiotrZSL would take a look at this PR since he was the author of the check.

[5chmidti]

I'm also of the opinion that being stricter is the much better default. I think we are also quite consistent with it. Some checks have some matching disabled that would technically be more strict, and this could be said about this option as well but matching empty catches in destructors is the better default IMO.

[github-actions]

⚠️ C/C++ code linter clang-tidy found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 origin/main...HEAD -- clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp |
python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \
  -path build -p1 -quiet

View the output from clang-tidy here.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1363:13: error: calling a private constructor of class 'clang::ast_matchers::internal::Matcher<clang::CXXCatchStmt>' [clang-diagnostic-error]
 1363 |     return {Matcher<T>(std::get<Is>(Params))...};
      |             ^
clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1348:16: note: in instantiation of function template specialization 'clang::ast_matchers::internal::VariadicOperatorMatcher<clang::ast_matchers::internal::Matcher<clang::Decl>>::getMatchers<clang::CXXCatchStmt, 0UL>' requested here
 1348 |                getMatchers<T>(std::index_sequence_for<Ps...>()))
      |                ^
clang/include/clang/ASTMatchers/ASTMatchersInternal.h:128:52: note: in instantiation of function template specialization 'clang::ast_matchers::internal::VariadicOperatorMatcher<clang::ast_matchers::internal::Matcher<clang::Decl>>::operator Matcher<clang::CXXCatchStmt>' requested here
  128 |     return Execute(Arg1, static_cast<const ArgT &>(Args)...);
      |                                                    ^
clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:91:19: note: in instantiation of function template specialization 'clang::ast_matchers::internal::VariadicFunction<clang::ast_matchers::internal::BindableMatcher<clang::Stmt>, clang::ast_matchers::internal::Matcher<clang::CXXCatchStmt>, &clang::ast_matchers::internal::makeDynCastAllOfComposite>::operator()<clang::ast_matchers::internal::VariadicOperatorMatcher<clang::ast_matchers::internal::Matcher<clang::CXXCatchStmt>>, clang::ast_matchers::internal::VariadicOperatorMatcher<clang::ast_matchers::internal::Matcher<clang::CXXCatchStmt>>, clang::ast_matchers::internal::VariadicOperatorMatcher<clang::ast_matchers::internal::Matcher<clang::Decl>>, clang::ast_matchers::internal::Matcher<clang::CXXCatchStmt>>' requested here
   91 |       cxxCatchStmt(unless(isExpansionInSystemHeader()), unless(isInMacro()),
      |                   ^
clang/include/clang/ASTMatchers/ASTMatchersInternal.h:666:12: note: declared private here
  666 |   explicit Matcher(const DynTypedMatcher &Implementation)
      |            ^