From 490bf27e53445fc4514c85142dec33ddf5bdcfe2 Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Sun, 23 Jul 2023 18:12:47 +0000 Subject: [PATCH] Revert "[clang-tidy] Add bugprone-empty-catch check" CI failed on "ubuntu-fast" due to disabled exceptions. This reverts commit f256fee5343033bf8a31aee06a80f3e982b76f82. --- .../clang-tidy/bugprone/BugproneTidyModule.cpp | 2 - .../clang-tidy/bugprone/CMakeLists.txt | 1 - .../clang-tidy/bugprone/EmptyCatchCheck.cpp | 110 --------------- .../clang-tidy/bugprone/EmptyCatchCheck.h | 37 ----- clang-tools-extra/docs/ReleaseNotes.rst | 5 - .../clang-tidy/checks/bugprone/empty-catch.rst | 149 --------------------- clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 - .../clang-tidy/checkers/bugprone/empty-catch.cpp | 67 --------- .../clang-tools-extra/clang-tidy/bugprone/BUILD.gn | 1 - 9 files changed, 373 deletions(-) delete mode 100644 clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp delete mode 100644 clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.h delete mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst delete mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 0cb0924..7509e94 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -20,7 +20,6 @@ #include "DanglingHandleCheck.h" #include "DynamicStaticInitializersCheck.h" #include "EasilySwappableParametersCheck.h" -#include "EmptyCatchCheck.h" #include "ExceptionEscapeCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" @@ -107,7 +106,6 @@ public: "bugprone-dynamic-static-initializers"); CheckFactories.registerCheck( "bugprone-easily-swappable-parameters"); - CheckFactories.registerCheck("bugprone-empty-catch"); CheckFactories.registerCheck( "bugprone-exception-escape"); CheckFactories.registerCheck("bugprone-fold-init-type"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 4076e0d..8bd892e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -15,7 +15,6 @@ add_clang_library(clangTidyBugproneModule DanglingHandleCheck.cpp DynamicStaticInitializersCheck.cpp EasilySwappableParametersCheck.cpp - EmptyCatchCheck.cpp ExceptionEscapeCheck.cpp FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp deleted file mode 100644 index 865c883..0000000 --- a/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp +++ /dev/null @@ -1,110 +0,0 @@ -//===--- 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 - -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, InnerMatcher) { - Stmt *Handler = Node.getHandlerBlock(); - if (!Handler) - return false; - return InnerMatcher.matches(*Handler, Finder, Builder); -} - -AST_MATCHER_P(CXXCatchStmt, hasCaughtType, Matcher, InnerMatcher) { - return InnerMatcher.matches(Node.getCaughtType(), Finder, Builder); -} - -AST_MATCHER_P(CompoundStmt, hasAnyTextFromList, std::vector, - 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 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("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 diff --git a/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.h b/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.h deleted file mode 100644 index b069438..0000000 --- a/clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.h +++ /dev/null @@ -1,37 +0,0 @@ -//===--- 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 - -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 getCheckTraversalKind() const override; - -private: - std::vector IgnoreCatchWithKeywords; - std::vector AllowEmptyCatchForExceptions; -}; - -} // namespace clang::tidy::bugprone - -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EMPTYCATCHCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 38a4053..2cc0010 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -114,11 +114,6 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ -- New :doc:`bugprone-empty-catch - ` check. - - Detects and suggests addressing issues with empty catch statements. - - New :doc:`bugprone-multiple-new-in-one-expression ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst deleted file mode 100644 index fd1db59..0000000 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst +++ /dev/null @@ -1,149 +0,0 @@ -.. 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. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index af147db..e13b675 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -86,7 +86,6 @@ Clang-Tidy Checks `bugprone-dangling-handle `_, `bugprone-dynamic-static-initializers `_, `bugprone-easily-swappable-parameters `_, - `bugprone-empty-catch `_, `bugprone-exception-escape `_, `bugprone-fold-init-type `_, `bugprone-forward-declaration-namespace `_, 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 deleted file mode 100644 index cb264eeb..0000000 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp +++ /dev/null @@ -1,67 +0,0 @@ -// 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'}]}" - -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 - } -} diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/bugprone/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/bugprone/BUILD.gn index e9a9ed1..6a34222 100644 --- a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/bugprone/BUILD.gn +++ b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/bugprone/BUILD.gn @@ -27,7 +27,6 @@ static_library("bugprone") { "DanglingHandleCheck.cpp", "DynamicStaticInitializersCheck.cpp", "EasilySwappableParametersCheck.cpp", - "EmptyCatchCheck.cpp", "ExceptionEscapeCheck.cpp", "FoldInitTypeCheck.cpp", "ForwardDeclarationNamespaceCheck.cpp", -- 2.7.4