[clang-tidy] Add bugprone-empty-catch check

[PiotrZSL]

Summary

Detects and suggests addressing issues with empty catch statements.

[Eugene.Zelenko]

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

78

Excessive newline.

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

170

Mistake in default value formatting.

[PiotrZSL]

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

170

Yy, what mistake ? (empty) not needed, or something else should be used instead of `` ?

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

170

Sorry, I mistook (empty) as part of value. May be just say this in words to avoid such confusions?

[PiotrZSL]

Fix review comments

[PiotrZSL]

Ping, Rebase

[carlosgalvezp]

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

101

Assert that it's not null, or exit early.

105

Nit: I believe a semicolon or dot is more appropriate here

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

148–154

I'm not sure this option is worth the added complexity - if people want to allow an empty catch with a comment, they might as well just add a NOLINT to suppress the warning? It will be even more clear that it's actually a problem: it's not a regular TODO that doesn't necessarily imply a problem, but rather something that a static analyzer considers a problem.

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp

12

Fix indentation to 2 chars.

17

Align comments

[carlosgalvezp]

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

104

Use 2 spaces indentation.

[PiotrZSL]

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

101

I cannot be null.

105

I can use semicolon

[PiotrZSL]

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

101

It*

[PiotrZSL]

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

148–154

I added this so people could have ability to ignore some issues without putting NOLINT's. This can be also used when developer put some macro into catch block, that would be build dependent, like assert or something.

[PiotrZSL]

Add getCheckTraversalKind, format tests, fix some review comments

[PiotrZSL]

Ping

[PiotrZSL]

Ping

[xgupta]

Other than these three points everything looks good to me.

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

72

This can be defined in the header file itself like other checks.

77

This can be defined in the header file itself like other checks.

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp

4

If TODO is in default then it does not require to be in value here, right?
I tested without that, it gives the warning.

[PiotrZSL]

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

72

Can be, but as this is already virtual function we do not gain anything by putting it into header. vtable will be emited anyway only on this TU.

77

Can be, but as this is already virtual function we do not gain anything by putting it into header. vtable will be emited anyway only on this TU.
And I want to keep it close to registerMatchers, as it impact behavior of that method.

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp

4

Setting IgnoreCatchWithKeywords manually override default value.

[PiotrZSL]

Rebase (release notes)

[xgupta]

LGTM, but we can wait for a couple of before committing in case other reviewers have more review comments.

[PiotrZSL]

Add -fexceptions to tests

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

 #include "AssignmentInIfConditionCheck.h"
 #include "BadSignalToKillThreadCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
+#include "EmptyCatchCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<EasilySwappableParametersCheck>(
         "bugprone-easily-swappable-parameters");
+    CheckFactories.registerCheck<EmptyCatchCheck>("bugprone-empty-catch");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
     CheckFactories.registerCheck<FoldInitTypeCheck>("bugprone-fold-init-type");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
+  EmptyCatchCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
   SwitchMissingDefaultCaseCheck.cpp
   IncorrectRoundingsCheck.cpp

clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.h

+//===--- EmptyCatchCheck.h - clang-tidy -------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EMPTYCATCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EMPTYCATCHCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include <vector>
+
+namespace clang::tidy::bugprone {
+
+/// Detects and suggests addressing issues with empty catch statements.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/empty-catch.html
+class EmptyCatchCheck : public ClangTidyCheck {
+public:
+  EmptyCatchCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override;
+
+private:
+  std::vector<llvm::StringRef> IgnoreCatchWithKeywords;
+  std::vector<llvm::StringRef> AllowEmptyCatchForExceptions;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EMPTYCATCHCHECK_H

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

+//===--- EmptyCatchCheck.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 "EmptyCatchCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+using ::clang::ast_matchers::internal::Matcher;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+AST_MATCHER(CXXCatchStmt, isInMacro) {
+  return Node.getBeginLoc().isMacroID() || Node.getEndLoc().isMacroID() ||
+         Node.getCatchLoc().isMacroID();
+}
+
+AST_MATCHER_P(CXXCatchStmt, hasHandler, Matcher<Stmt>, InnerMatcher) {
+  Stmt *Handler = Node.getHandlerBlock();
+  if (!Handler)
+    return false;
+  return InnerMatcher.matches(*Handler, Finder, Builder);
+}
+
+AST_MATCHER_P(CXXCatchStmt, hasCaughtType, Matcher<QualType>, InnerMatcher) {
+  return InnerMatcher.matches(Node.getCaughtType(), Finder, Builder);
+}
+
+AST_MATCHER_P(CompoundStmt, hasAnyTextFromList, std::vector<llvm::StringRef>,
+              List) {
+  if (List.empty())
+    return false;
+
+  ASTContext &Context = Finder->getASTContext();
+  SourceManager &SM = Context.getSourceManager();
+  StringRef Text = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(Node.getSourceRange()), SM,
+      Context.getLangOpts());
+  return std::any_of(List.begin(), List.end(), [&](const StringRef &Str) {
+    return Text.contains_insensitive(Str);
+  });
+}
+
+} // namespace
+
+EmptyCatchCheck::EmptyCatchCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreCatchWithKeywords(utils::options::parseStringList(
+          Options.get("IgnoreCatchWithKeywords", "@TODO;@FIXME"))),
+      AllowEmptyCatchForExceptions(utils::options::parseStringList(
+          Options.get("AllowEmptyCatchForExceptions", ""))) {}
+
+void EmptyCatchCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreCatchWithKeywords",
+                utils::options::serializeStringList(IgnoreCatchWithKeywords));
+  Options.store(
+      Opts, "AllowEmptyCatchForExceptions",
+      utils::options::serializeStringList(AllowEmptyCatchForExceptions));
+}
+
+bool EmptyCatchCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus;
+}
+
+std::optional<TraversalKind> EmptyCatchCheck::getCheckTraversalKind() const {
+  return TK_IgnoreUnlessSpelledInSource;
+}
+
+void EmptyCatchCheck::registerMatchers(MatchFinder *Finder) {
+  auto AllowedNamedExceptionDecl =
+      namedDecl(matchers::matchesAnyListedName(AllowEmptyCatchForExceptions));
+  auto AllowedNamedExceptionTypes =
+      qualType(anyOf(hasDeclaration(AllowedNamedExceptionDecl),
+                     references(AllowedNamedExceptionDecl),
+                     pointsTo(AllowedNamedExceptionDecl)));
+  auto IgnoredExceptionType =
+      qualType(anyOf(AllowedNamedExceptionTypes,
+                     hasCanonicalType(AllowedNamedExceptionTypes)));
+
+  Finder->addMatcher(
+      cxxCatchStmt(unless(isExpansionInSystemHeader()), unless(isInMacro()),
+                   unless(hasCaughtType(IgnoredExceptionType)),
+                   hasHandler(compoundStmt(
+                       statementCountIs(0),
+                       unless(hasAnyTextFromList(IgnoreCatchWithKeywords)))))
+          .bind("catch"),
+      this);
+}
+
+void EmptyCatchCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCatchStmt = Result.Nodes.getNodeAs<CXXCatchStmt>("catch");
+
+  diag(
+      MatchedCatchStmt->getCatchLoc(),
+      "empty catch statements hide issues; to handle exceptions appropriately, "
+      "consider re-throwing, handling, or avoiding catch altogether");
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

 - `WarningsAsErrors` (`--warnings-as-errors=`) no longer promotes unlisted
   warnings to errors. Only the warnings listed in `Checks` (`--checks=`) will
   be promoted to errors. For custom error promotion, use `-Werror=<warning>`
   on the compiler command-line, irrespective of `Checks` (`--checks=`) settings.
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-empty-catch
+  <clang-tidy/checks/bugprone/empty-catch>` check.
+
+  Detects and suggests addressing issues with empty catch statements.
+
 - New :doc:`bugprone-multiple-new-in-one-expression
   <clang-tidy/checks/bugprone/multiple-new-in-one-expression>` check.
 
   Finds multiple ``new`` operator calls in a single expression, where the allocated
   memory by the first ``new`` may leak if the second allocation fails and throws exception.
 
 - New :doc:`bugprone-non-zero-enum-to-bool-conversion
   <clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check.

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst

+.. title:: clang-tidy - bugprone-empty-catch
+
+bugprone-empty-catch
+====================
+
+Detects and suggests addressing issues with empty catch statements.
+
+.. code-block:: c++
+
+  try {
+    // Some code that can throw an exception
+  } catch(const std::exception&) {
+  }
+
+Having empty catch statements in a codebase can be a serious problem that
+developers should be aware of. Catch statements are used to handle exceptions
+that are thrown during program execution. When an exception is thrown, the
+program jumps to the nearest catch statement that matches the type of the
+exception.
+
+Empty catch statements, also known as "swallowing" exceptions, catch the
+exception but do nothing with it. This means that the exception is not handled
+properly, and the program continues to run as if nothing happened. This can
+lead to several issues, such as:
+
+* *Hidden Bugs*: If an exception is caught and ignored, it can lead to hidden
+  bugs that are difficult to diagnose and fix. The root cause of the problem
+  may not be apparent, and the program may continue to behave in unexpected
+  ways.
+
+* *Security Issues*: Ignoring exceptions can lead to security issues, such as
+  buffer overflows or null pointer dereferences. Hackers can exploit these
+  vulnerabilities to gain access to sensitive data or execute malicious code.
+
+* *Poor Code Quality*: Empty catch statements can indicate poor code quality
+  and a lack of attention to detail. This can make the codebase difficult to
+  maintain and update, leading to longer development cycles and increased
+  costs.
+
+* *Unreliable Code*: Code that ignores exceptions is often unreliable and can
+  lead to unpredictable behavior. This can cause frustration for users and
+  erode trust in the software.
+
+To avoid these issues, developers should always handle exceptions properly.
+This means either fixing the underlying issue that caused the exception or
+propagating the exception up the call stack to a higher-level handler.
+If an exception is not important, it should still be logged or reported in
+some way so that it can be tracked and addressed later.
+
+If the exception is something that can be handled locally, then it should be
+handled within the catch block. This could involve logging the exception or
+taking other appropriate action to ensure that the exception is not ignored.
+
+Here is an example:
+
+.. code-block:: c++
+
+  try {
+    // Some code that can throw an exception
+  } catch (const std::exception& ex) {
+    // Properly handle the exception, e.g.:
+    std::cerr << "Exception caught: " << ex.what() << std::endl;
+  }
+
+If the exception cannot be handled locally and needs to be propagated up the
+call stack, it should be re-thrown or new exception should be thrown.
+
+Here is an example:
+
+.. code-block:: c++
+
+  try {
+    // Some code that can throw an exception
+  } catch (const std::exception& ex) {
+    // Re-throw the exception
+    throw;
+  }
+
+In some cases, catching the exception at this level may not be necessary, and
+it may be appropriate to let the exception propagate up the call stack.
+This can be done simply by not using ``try/catch`` block.
+
+Here is an example:
+
+.. code-block:: c++
+
+  void function() {
+    // Some code that can throw an exception
+  }
+
+  void callerFunction() {
+    try {
+      function();
+    } catch (const std::exception& ex) {
+      // Handling exception on higher level
+      std::cerr << "Exception caught: " << ex.what() << std::endl;
+    }
+  }
+
+Other potential solution to avoid empty catch statements is to modify the code
+to avoid throwing the exception in the first place. This can be achieved by
+using a different API, checking for error conditions beforehand, or handling
+errors in a different way that does not involve exceptions. By eliminating the
+need for try-catch blocks, the code becomes simpler and less error-prone.
+
+Here is an example:
+
+.. code-block:: c++
+
+  // Old code:
+  try {
+    mapContainer["Key"].callFunction();
+  } catch(const std::out_of_range&) {
+  }
+
+  // New code
+  if (auto it = mapContainer.find("Key"); it != mapContainer.end()) {
+    it->second.callFunction();
+  }
+
+In conclusion, empty catch statements are a bad practice that can lead to hidden
+bugs, security issues, poor code quality, and unreliable code. By handling
+exceptions properly, developers can ensure that their code is robust, secure,
+and maintainable.
+
+Options
+-------
+
+.. option:: IgnoreCatchWithKeywords
+
+  This option can be used to ignore specific catch statements containing
+  certain keywords. If a ``catch`` statement body contains (case-insensitive)
+  any of the keywords listed in this semicolon-separated option, then the
+  catch will be ignored, and no warning will be raised.
+  Default value: `@TODO;@FIXME`.
+
+.. option:: AllowEmptyCatchForExceptions
+
+  This option can be used to ignore empty catch statements for specific
+  exception types. By default, the check will raise a warning if an empty
+  catch statement is detected, regardless of the type of exception being
+  caught. However, in certain situations, such as when a developer wants to
+  intentionally ignore certain exceptions or handle them in a different way,
+  it may be desirable to allow empty catch statements for specific exception
+  types.
+  To configure this option, a semicolon-separated list of exception type names
+  should be provided. If an exception type name in the list is caught in an
+  empty catch statement, no warning will be raised.
+  Default value: empty string.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition.html>`_,
    `bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread.html>`_,
    `bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion.html>`_, "Yes"
    `bugprone-branch-clone <bugprone/branch-clone.html>`_,
    `bugprone-copy-constructor-init <bugprone/copy-constructor-init.html>`_, "Yes"
    `bugprone-dangling-handle <bugprone/dangling-handle.html>`_,
    `bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers.html>`_,
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.html>`_,
+   `bugprone-empty-catch <bugprone/empty-catch.html>`_,
    `bugprone-exception-escape <bugprone/exception-escape.html>`_,
    `bugprone-fold-init-type <bugprone/fold-init-type.html>`_,
    `bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace.html>`_,
    `bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload.html>`_,
    `bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result.html>`_, "Yes"
    `bugprone-inaccurate-erase <bugprone/inaccurate-erase.html>`_, "Yes"
    `bugprone-incorrect-roundings <bugprone/incorrect-roundings.html>`_,
    `bugprone-infinite-loop <bugprone/infinite-loop.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp

+// RUN: %check_clang_tidy -std=c++98-or-later %s bugprone-empty-catch %t -- \
+// RUN: -config="{CheckOptions: [{key: bugprone-empty-catch.AllowEmptyCatchForExceptions, value: '::SafeException;WarnException'}, \
+// RUN:        {key: bugprone-empty-catch.IgnoreCatchWithKeywords, value: '@IGNORE;@TODO'}]}" -- -fexceptions
+
+struct Exception {};
+struct SafeException {};
+struct WarnException : Exception {};
+
+int functionWithThrow() {
+  try {
+    throw 5;
+  } catch (const Exception &) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
+  } catch (...) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
+  }
+  return 0;
+}
+
+int functionWithHandling() {
+  try {
+    throw 5;
+  } catch (const Exception &) {
+    return 2;
+  } catch (...) {
+    return 1;
+  }
+  return 0;
+}
+
+int functionWithReThrow() {
+  try {
+    throw 5;
+  } catch (...) {
+    throw;
+  }
+}
+
+int functionWithNewThrow() {
+  try {
+    throw 5;
+  } catch (...) {
+    throw Exception();
+  }
+}
+
+void functionWithAllowedException() {
+  try {
+
+  } catch (const SafeException &) {
+  } catch (WarnException) {
+  }
+}
+
+void functionWithComment() {
+  try {
+  } catch (const Exception &) {
+    // @todo: implement later, check case insensitive
+  }
+}
+
+void functionWithComment2() {
+  try {
+  } catch (const Exception &) {
+    // @IGNORE: relax its safe
+  }
+}

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/bugprone/BUILD.gn

sources = [
     "BadSignalToKillThreadCheck.cpp",
     "BoolPointerImplicitConversionCheck.cpp",
     "BranchCloneCheck.cpp",
     "BugproneTidyModule.cpp",
     "CopyConstructorInitCheck.cpp",
     "DanglingHandleCheck.cpp",
     "DynamicStaticInitializersCheck.cpp",
     "EasilySwappableParametersCheck.cpp",
+    "EmptyCatchCheck.cpp",
     "ExceptionEscapeCheck.cpp",
     "FoldInitTypeCheck.cpp",
     "ForwardDeclarationNamespaceCheck.cpp",
     "ForwardingReferenceOverloadCheck.cpp",
     "ImplicitWideningOfMultiplicationResultCheck.cpp",
     "InaccurateEraseCheck.cpp",
     "IncorrectRoundingsCheck.cpp",
     "InfiniteLoopCheck.cpp",