[clang-tidy] Allow explicit throwing in bugprone-exception-escape for special functions

[PiotrZSL]

Summary

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 and assignment operators,
and destructors, it is a clear indication of the developer's intention and
should be respected.

Fixes: #40583, #55143

[Eugene.Zelenko]

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

29

Double period.

[isuckatcs]

Apart from some nits I think this patch looks good.

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

26

How about creating an isExplicitThrow() helper function inside ExceptionSpecificationType.h, where these enums are defined? That way if one more enum is added, it will be more convenient to update the explicit throw checks, not to mention it can also be reused in other checkers if needed.

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

14

Why remove noexcept(true)?

25–29

[PiotrZSL]

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

14

You right, this not need to be changed.

[PiotrZSL]

Review fixes

[PiotrZSL]

Ping

[carlosgalvezp]

LGTM, thanks for the fix!

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

 
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringSet.h"
 
 using namespace clang::ast_matchers;
 
-namespace clang {
+namespace clang::tidy::bugprone {
 namespace {
+
 AST_MATCHER_P(FunctionDecl, isEnabled, llvm::StringSet<>,
               FunctionsThatShouldNotThrow) {
   return FunctionsThatShouldNotThrow.count(Node.getNameAsString()) > 0;
 }
+
+AST_MATCHER(FunctionDecl, isExplicitThrow) {
+  return isExplicitThrowExceptionSpec(Node.getExceptionSpecType()) &&
+         Node.getExceptionSpecSourceRange().isValid();
+}
+
 } // namespace
 
-namespace tidy::bugprone {
 ExceptionEscapeCheck::ExceptionEscapeCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), RawFunctionsThatShouldNotThrow(Options.get(
                                          "FunctionsThatShouldNotThrow", "")),
       RawIgnoredExceptions(Options.get("IgnoredExceptions", "")) {
   llvm::SmallVector<StringRef, 8> FunctionsThatShouldNotThrowVec,
       IgnoredExceptionsVec;
   StringRef(RawFunctionsThatShouldNotThrow)
void ExceptionEscapeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "FunctionsThatShouldNotThrow",
                 RawFunctionsThatShouldNotThrow);
   Options.store(Opts, "IgnoredExceptions", RawIgnoredExceptions);
 }
 
 void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(isDefinition(),
-                   anyOf(isNoThrow(), cxxDestructorDecl(),
+                   anyOf(isNoThrow(),
+                         allOf(anyOf(cxxDestructorDecl(),
                                      cxxConstructorDecl(isMoveConstructor()),
                                      cxxMethodDecl(isMoveAssignmentOperator()),
-                         hasName("main"), hasName("swap"),
+                                     isMain(), hasName("swap")),
+                               unless(isExplicitThrow())),
                          isEnabled(FunctionsThatShouldNotThrow)))
           .bind("thrower"),
       this);
 }
 
 void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("thrower");
 
   if (!MatchedDecl)
     return;
 
   if (Tracer.analyze(MatchedDecl).getBehaviour() ==
       utils::ExceptionAnalyzer::State::Throwing)
     // FIXME: We should provide more information about the exact location where
     // the exception is thrown, maybe the full path the exception escapes
     diag(MatchedDecl->getLocation(), "an exception may be thrown in function "
                                      "%0 which should not throw exceptions")
         << MatchedDecl;
 }
 
-} // namespace tidy::bugprone
-} // namespace clang
+} // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-dangling-handle
   handles behind type aliases.
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`bugprone-dynamic-static-initializers
   <clang-tidy/checks/bugprone/dynamic-static-initializers>` check.
   Global options of the same name should be used instead.
 
 - Improved :doc:`bugprone-exception-escape
-  <clang-tidy/checks/bugprone/exception-escape>` to not emit warnings for
-  forward declarations of functions and additionally modified it to skip
-  ``noexcept`` functions during call stack analysis.
-
-- Fixed :doc:`bugprone-exception-escape<clang-tidy/checks/bugprone/exception-escape>`
-  for coroutines where previously a warning would be emitted with coroutines
-  throwing exceptions in their bodies.
+  <clang-tidy/checks/bugprone/exception-escape>` check to not emit warnings for
+  forward declarations of functions, explicitly declared throwing functions,
+  coroutines throwing exceptions in their bodies and skip ``noexcept``
+  functions during call stack analysis.
 
 - Improved :doc:`bugprone-fold-init-type
   <clang-tidy/checks/bugprone/fold-init-type>` to handle iterators that do not
   define `value_type` type aliases.
 
 - Improved :doc:`bugprone-incorrect-roundings
   <clang-tidy/checks/bugprone/incorrect-roundings>` check by adding support for
   other floating point representations in float constant like ``0.5L``.

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

 .. title:: clang-tidy - bugprone-exception-escape
 
 bugprone-exception-escape
 =========================
 
 Finds functions which may throw an exception directly or indirectly, but they
 should not. The functions which should not throw exceptions are the following:
 
 * Destructors
 * Move constructors
 * Move assignment operators
 * The ``main()`` functions
 * ``swap()`` functions
 * Functions marked with ``throw()`` or ``noexcept``
 * Other functions given as option
 
 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.
 
+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.
+
 WARNING! This check may be expensive on large source files.
 
 Options
 -------
 
 .. option:: FunctionsThatShouldNotThrow
 
    Comma separated list containing function names which should not throw. An
    example value for this parameter can be ``WinMain`` which adds function
    ``WinMain()`` in the Windows API to the list of the functions which should
    not throw. Default value is an empty string.
 
 .. option:: IgnoredExceptions
 
    Comma separated list containing type names which are not counted as thrown
    exceptions in the check. Default value is an empty string.

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp

void unhandled_exception() {
     }
   }
 
   Task *return_val;
 };
 
 struct Evil {
   ~Evil() noexcept(false) {
-    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~Evil' which should not throw exceptions
     throw 42;
   }
 };
 
 Task<int> returnOne() { co_return 1; }
 
 namespace function {
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp

 }
 
 void calls_non_and_throwing() noexcept {
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'calls_non_and_throwing' which should not throw exceptions
   test_basic_no_throw();
   test_basic_throw();
 }
 
+namespace PR55143 { namespace PR40583 {
+
+struct test_explicit_throw {
+    test_explicit_throw() throw(int) { throw 42; }
+    test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
+    test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
+    test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
+    test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
+    ~test_explicit_throw() throw(int) { throw 42; }
+};
+
+struct test_implicit_throw {
+    test_implicit_throw() { throw 42; }
+    test_implicit_throw(const test_implicit_throw&) { throw 42; }
+    test_implicit_throw(test_implicit_throw&&) { throw 42; }
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
+    test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
+    test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; }
+    // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
+    ~test_implicit_throw() { throw 42; }
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions
+};
+
+}}

clang/include/clang/Basic/ExceptionSpecificationType.h

inline bool isNoexceptExceptionSpec(ExceptionSpecificationType ESpecType) {
   return ESpecType == EST_BasicNoexcept || ESpecType == EST_NoThrow ||
          isComputedNoexcept(ESpecType);
 }
 
 inline bool isUnresolvedExceptionSpec(ExceptionSpecificationType ESpecType) {
   return ESpecType == EST_Unevaluated || ESpecType == EST_Uninstantiated;
 }
 
+inline bool isExplicitThrowExceptionSpec(ExceptionSpecificationType ESpecType) {
+  return ESpecType == EST_Dynamic || ESpecType == EST_MSAny ||
+         ESpecType == EST_NoexceptFalse;
+}
+
 /// Possible results from evaluation of a noexcept expression.
 enum CanThrowResult {
   CT_Cannot,
   CT_Dependent,
   CT_Can
 };
 
 inline CanThrowResult mergeCanThrow(CanThrowResult CT1, CanThrowResult CT2) {
   // CanThrowResult constants are ordered so that the maximum is the correct
   // merge result.
   return CT1 > CT2 ? CT1 : CT2;
 }
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_BASIC_EXCEPTIONSPECIFICATIONTYPE_H