[clang-tidy] Add check 'bugprone-unsafe-to-allow-exceptions' (llvm#176430)

`ExceptionEscapeCheck` does not warn if a function is `noexcept(false)`
(may throw) but is one of the functions where throwing exceptions is
unsafe. This check can be used to find such cases.

Author: balazske

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

@@ -106,6 +106,7 @@
 #include "UnintendedCharOstreamOutputCheck.h"
 #include "UniquePtrArrayMismatchCheck.h"
 #include "UnsafeFunctionsCheck.h"
+#include "UnsafeToAllowExceptionsCheck.h"
 #include "UnusedLocalNonTrivialVariableCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
@@ -308,6 +309,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-crtp-constructor-accessibility");
     CheckFactories.registerCheck<UnsafeFunctionsCheck>(
         "bugprone-unsafe-functions");
+    CheckFactories.registerCheck<UnsafeToAllowExceptionsCheck>(
+        "bugprone-unsafe-to-allow-exceptions");
     CheckFactories.registerCheck<UnusedLocalNonTrivialVariableCheck>(
         "bugprone-unused-local-non-trivial-variable");
     CheckFactories.registerCheck<UnusedRaiiCheck>("bugprone-unused-raii");

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

@@ -108,6 +108,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   UnhandledSelfAssignmentCheck.cpp
   UniquePtrArrayMismatchCheck.cpp
   UnsafeFunctionsCheck.cpp
+  UnsafeToAllowExceptionsCheck.cpp
   UnusedLocalNonTrivialVariableCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp

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

@@ -0,0 +1,61 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "UnsafeToAllowExceptionsCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+namespace {
+
+AST_MATCHER(FunctionDecl, isExplicitThrow) {
+  return isExplicitThrowExceptionSpec(Node.getExceptionSpecType()) &&
+         Node.getExceptionSpecSourceRange().isValid();
+}
+
+} // namespace
+
+UnsafeToAllowExceptionsCheck::UnsafeToAllowExceptionsCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      CheckedSwapFunctions(utils::options::parseStringList(
+          Options.get("CheckedSwapFunctions", "swap;iter_swap;iter_move"))) {}
+
+void UnsafeToAllowExceptionsCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "CheckedSwapFunctions",
+                utils::options::serializeStringList(CheckedSwapFunctions));
+}
+
+void UnsafeToAllowExceptionsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(isDefinition(), isExplicitThrow(),
+                   anyOf(cxxDestructorDecl(),
+                         cxxConstructorDecl(isMoveConstructor()),
+                         cxxMethodDecl(isMoveAssignmentOperator()),
+                         allOf(hasAnyName(CheckedSwapFunctions),
+                               unless(parameterCountIs(0))),
+                         isMain()))
+          .bind("f"),
+      this);
+}
+
+void UnsafeToAllowExceptionsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("f");
+  assert(MatchedDecl);
+
+  diag(MatchedDecl->getLocation(),
+       "function %0 should not throw exceptions but "
+       "it is still marked as potentially throwing")
+      << MatchedDecl;
+}
+
+} // namespace clang::tidy::bugprone

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

@@ -0,0 +1,37 @@
+//===----------------------------------------------------------------------===//
+//
+// 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_UNSAFETOALLOWEXCEPTIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFETOALLOWEXCEPTIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds functions where throwing exceptions is unsafe but the function is
+/// still marked as potentially throwing.
+///
+/// For the user-facing documentation see:
+/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-to-allow-exceptions.html
+class UnsafeToAllowExceptionsCheck : public ClangTidyCheck {
+public:
+  UnsafeToAllowExceptionsCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus && LangOpts.CXXExceptions;
+  }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::vector<llvm::StringRef> CheckedSwapFunctions;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFETOALLOWEXCEPTIONSCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -102,6 +102,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-unsafe-to-allow-exceptions
+  <clang-tidy/checks/bugprone/unsafe-to-allow-exceptions>` check.
+
+  Finds functions where throwing exceptions is unsafe but the function is still
+  marked as potentially throwing.
+
 - New :doc:`llvm-type-switch-case-types
   <clang-tidy/checks/llvm/type-switch-case-types>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst

@@ -28,7 +28,10 @@ Functions declared explicitly with ``noexcept(false)`` or ``throw(exception)``
 will be excluded from the analysis, as even though it is not recommended for
 functions like ``swap()``, ``main()``, move constructors, move assignment
 operators and destructors, it is a clear indication of the developer's
-intention and should be respected.
+intention and should be respected. To check if these special functions are
+marked as potentially throwing, the check
+:doc:`bugprone-unsafe-to-allow-exceptions <unsafe-to-allow-exceptions>` can be
+used.
 
 WARNING! This check may be expensive on large source files.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-to-allow-exceptions.rst

@@ -0,0 +1,41 @@
+.. title:: clang-tidy - bugprone-unsafe-to-allow-exceptions
+
+bugprone-unsafe-to-allow-exceptions
+===================================
+
+Finds functions where throwing exceptions is unsafe but the function is still
+marked as potentially throwing. Throwing exceptions from the following
+functions can be problematic:
+
+* Destructors
+* Move constructors
+* Move assignment operators
+* The ``main()`` functions
+* ``swap()`` functions
+* ``iter_swap()`` functions
+* ``iter_move()`` functions
+
+A destructor throwing an exception may result in undefined behavior, resource
+leaks or unexpected termination of the program. Throwing move constructor or
+move assignment also may result in undefined behavior or resource leak. The
+``swap()`` operations expected to be non throwing most of the cases and they
+are always possible to implement in a non throwing way. Non throwing ``swap()``
+operations are also used to create move operations. A throwing ``main()``
+function also results in unexpected termination.
+
+The check finds any of these functions if it is marked with ``noexcept(false)``
+or ``throw(exception)``. This would indicate that the function is expected to
+throw exceptions. Only the presence of these keywords is checked, not if the
+function actually throws any exception. To check if the function actually
+throws exception, the check :doc:`bugprone-exception-escape <exception-escape>`
+can be used (but it does not warn if a function is explicitly marked as
+throwing).
+
+Options
+-------
+
+.. option:: CheckedSwapFunctions
+
+   Semicolon-separated list of checked swap function names (where throwing
+   exceptions is unsafe). These functions are checked if the parameter count is
+   at least 1. Default value is `swap;iter_swap;iter_move`.

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

@@ -176,6 +176,7 @@ Clang-Tidy Checks
    :doc:`bugprone-unintended-char-ostream-output <bugprone/unintended-char-ostream-output>`, "Yes"
    :doc:`bugprone-unique-ptr-array-mismatch <bugprone/unique-ptr-array-mismatch>`, "Yes"
    :doc:`bugprone-unsafe-functions <bugprone/unsafe-functions>`,
+   :doc:`bugprone-unsafe-to-allow-exceptions <bugprone/unsafe-to-allow-exceptions>`,
    :doc:`bugprone-unused-local-non-trivial-variable <bugprone/unused-local-non-trivial-variable>`,
    :doc:`bugprone-unused-raii <bugprone/unused-raii>`, "Yes"
    :doc:`bugprone-unused-return-value <bugprone/unused-return-value>`,

clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-to-allow-exceptions-precpp17.cpp

@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-unsafe-to-allow-exceptions %t -- \
+// RUN:     -config="{CheckOptions: { \
+// RUN:         bugprone-unsafe-to-allow-exceptions.CheckedSwapFunctions: 'swap', \
+// RUN:     }}" \
+// RUN: -- -fexceptions
+
+class Exception {};
+
+struct may_throw {
+  may_throw(may_throw&&) throw(int) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'may_throw' should not throw exceptions but it is still marked as potentially throwing [bugprone-unsafe-to-allow-exceptions]
+  }
+  may_throw& operator=(may_throw&&) throw(Exception) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: function 'operator=' should not throw exceptions but it is still marked as potentially throwing
+  }
+  ~may_throw() throw(char, Exception) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function '~may_throw' should not throw exceptions but it is still marked as potentially throwing
+  }
+
+  void f() noexcept(false) {
+  }
+};
+
+struct no_throw {
+  no_throw(no_throw&&) throw() {
+  }
+  no_throw& operator=(no_throw&&) noexcept(true) {
+  }
+  ~no_throw() noexcept(true) {
+  }
+};
+
+int main() throw(char) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'main' should not throw exceptions but it is still marked as potentially throwing
+  return 0;
+}
+
+void swap(no_throw&, no_throw&) throw(bool) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'swap' should not throw exceptions but it is still marked as potentially throwing
+}
+
+void iter_swap(int&, int&) throw(bool) {
+}
+
+void iter_move(int&) throw(bool) {
+}
+
+void swap(double&, double&) {
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-to-allow-exceptions.cpp

@@ -0,0 +1,54 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-unsafe-to-allow-exceptions %t -- -- -fexceptions
+// RUN: %check_clang_tidy -std=c++17-or-later -check-suffix=,CUSTOMSWAP %s bugprone-unsafe-to-allow-exceptions %t -- \
+// RUN:     -config="{CheckOptions: { \
+// RUN:         bugprone-unsafe-to-allow-exceptions.CheckedSwapFunctions: 'swap;iter_swap;iter_move;swap1', \
+// RUN:     }}" \
+// RUN: -- -fexceptions
+
+struct may_throw {
+  may_throw(may_throw&&) noexcept(false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'may_throw' should not throw exceptions but it is still marked as potentially throwing [bugprone-unsafe-to-allow-exceptions]
+  }
+  may_throw& operator=(may_throw&&) noexcept(false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: function 'operator=' should not throw exceptions but it is still marked as potentially throwing
+  }
+  ~may_throw() noexcept(false) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function '~may_throw' should not throw exceptions but it is still marked as potentially throwing
+  }
+
+  void f() noexcept(false) {
+  }
+};
+
+struct no_throw {
+  no_throw(no_throw&&) throw() {
+  }
+  no_throw& operator=(no_throw&&) noexcept(true) {
+  }
+  ~no_throw() noexcept(true) {
+  }
+};
+
+int main() noexcept(false) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'main' should not throw exceptions but it is still marked as potentially throwing
+  return 0;
+}
+
+void swap(int&, int&) noexcept(false) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'swap' should not throw exceptions but it is still marked as potentially throwing
+}
+
+void iter_swap(int&, int&) noexcept(false) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'iter_swap' should not throw exceptions but it is still marked as potentially throwing
+}
+
+void iter_move(int&) noexcept(false) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'iter_move' should not throw exceptions but it is still marked as potentially throwing
+}
+
+void swap(double&, double&) {
+}
+
+void swap1(long&) noexcept(false) {
+  // CHECK-MESSAGES-CUSTOMSWAP: :[[@LINE-1]]:6: warning: function 'swap1' should not throw exceptions but it is still marked as potentially throwing
+}