[clang-tidy] Model noexcept more properly in bugprone-exception-escape

[PiotrZSL]

Summary

During call stack analysis skip called noexcept functions
as they wont throw exceptions, they will crash.
Check will emit warnings for those functions separately.

Fixes: #43667, #49151, #51596, #54668, #54956

[isuckatcs]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

319

Put this in the anonymous namespace above please to remain consistent.

453

Is cannotThrow(Func) really needed here? Isn't it possible to bail out after the body of the function has been analyzed?

I understand that you want to prevent some recursive calls and bail out early, but I don't think that it worths adding some additional logic, which is not needed anyway.

If you really want to optimize this or you're worried about stack overflows, consider rewriting the recursive solution to an iterative one.

[PiotrZSL]

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

319

well, llvm style require static, if we want to be consistent, I can change all other into static later.

453

Yes, it should be here because it's called in 3 places so that an best place to put it.
It's not about recursion or performance, it's already exist and I simply do no change it.

Simply if we analyse callExpr to function/destructor/constructor/... and that FunctionDecl is noexcept, then we should jump to that function and analyse statements in that function body at all.
And this is what is done here. If this isn't first function !CallStack.empty() (aka: call from check code), and it's noexcept, then assume that it cannot throw and do not go deeper.

[PiotrZSL]

Ping

[carlosgalvezp]

Looks good, great to see all these issues fixed! Have a couple small comments.

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

319

Nit: I personally find it more readable as "canThrow". People with issues reading double negations will probably appreciate that too :) But if you strongly prefer this name I won't oppose.

330–332

Would be good to write a small comment documenting this logic.

332

Would be good to use the syntax /* Parameter = */true to document what this magic true means.

[PiotrZSL]

Fix review comments

[carlosgalvezp]

LGTM, thanks!

clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp

bool isQualificationConvertiblePointer(QualType From, QualType To,
   // We hit U.
   if (!SatisfiesCVRules(From, To))
     return false;
 
   return From.getTypePtr() == To.getTypePtr();
 }
 } // namespace
 
+static bool canThrow(const FunctionDecl *Func) {
+  const auto *FunProto = Func->getType()->getAs<FunctionProtoType>();
+  if (!FunProto)
+    return true;
+
+  switch (FunProto->canThrow()) {
+  case CT_Cannot:
+    return false;
+  case CT_Dependent: {
+    const Expr *NoexceptExpr = FunProto->getNoexceptExpr();
+    if (!NoexceptExpr)
+      return true; // no noexept - can throw
+
+    if (NoexceptExpr->isValueDependent())
+      return true; // depend on template - some instance can throw
+
+    bool Result = false;
+    if (!NoexceptExpr->EvaluateAsBooleanCondition(Result, Func->getASTContext(),
+                                                  /*InConstantContext=*/true))
+      return true;  // complex X condition in noexcept(X), cannot validate,
+                    // assume that may throw
+    return !Result; // noexcept(false) - can throw
+  }
+  default:
+    return true;
+  };
+}
+
 bool ExceptionAnalyzer::ExceptionInfo::filterByCatch(
     const Type *HandlerTy, const ASTContext &Context) {
   llvm::SmallVector<const Type *, 8> TypesToDelete;
   for (const Type *ExceptionTy : ThrownExceptions) {
     CanQualType ExceptionCanTy = ExceptionTy->getCanonicalTypeUnqualified();
     CanQualType HandlerCanTy = HandlerTy->getCanonicalTypeUnqualified();
 
     // The handler is of type cv T or cv T& and E and T are the same type
else
       Behaviour = State::NotThrowing;
   else
     Behaviour = State::Throwing;
 }
 
 ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
     const FunctionDecl *Func, const ExceptionInfo::Throwables &Caught,
     llvm::SmallSet<const FunctionDecl *, 32> &CallStack) {
-  if (CallStack.count(Func))
+  if (!Func || CallStack.count(Func) || (!CallStack.empty() && !canThrow(Func)))
     return ExceptionInfo::createNonThrowing();
 
   if (const Stmt *Body = Func->getBody()) {
     CallStack.insert(Func);
     ExceptionInfo Result = throwsException(Body, Caught, CallStack);
 
     // For a constructor, we also have to check the initializers.
     if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Func)) {
       for (const CXXCtorInitializer *Init : Ctor->inits()) {

clang-tools-extra/docs/ReleaseNotes.rst

 
 - 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.
+  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.
 
 - Improved :doc:`bugprone-fold-init-type
   <clang-tidy/checks/bugprone/fold-init-type>` to handle iterators that do not
   define `value_type` type aliases.

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

void throw_catch_multi_ptr_4() noexcept {
   try {
     char **p = 0;
     throw p;
   } catch (volatile const char *const *) {
   }
 }
 
 // FIXME: In this case 'a' is convertible to the handler and should be caught
-// but in reality it's thrown. Note that clang doesn't report a warning for 
+// but in reality it's thrown. Note that clang doesn't report a warning for
 // this either.
 void throw_catch_multi_ptr_5() noexcept {
   try {
     double *a[2][3];
     throw a;
   } catch (double *(*)[3]) {
   }
 }
try {
     throw derived();
   } catch(alias &) {
   }
 }
 
 void throw_derived_catch_base_ptr_c() noexcept {
   try {
     derived d;
-    throw &d; 
+    throw &d;
   } catch(const base *) {
   }
 }
 
 void throw_derived_catch_base_ptr() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ptr' which should not throw exceptions
   try {
     derived d;
     const derived *p = &d;
-    throw p; 
+    throw p;
   } catch(base *) {
   }
 }
 
 class A {};
 class B : A {};
 
 // The following alias hell is deliberately created for testing.
 using aliasedA = A;
 class C : protected aliasedA {};
 
 typedef aliasedA moreAliasedA;
 class D : public moreAliasedA {};
 
 using moreMoreAliasedA = moreAliasedA;
 using aliasedD = D;
 class E : public moreMoreAliasedA, public aliasedD {};
 
 void throw_derived_catch_base_private() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private' which should not throw exceptions
   try {
     B b;
-    throw b; 
+    throw b;
   } catch(A) {
   }
 }
 
 void throw_derived_catch_base_private_ptr() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private_ptr' which should not throw exceptions
   try {
     B b;
-    throw &b; 
+    throw &b;
   } catch(A *) {
   }
 }
 
 void throw_derived_catch_base_protected() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_protected' which should not throw exceptions
   try {
     C c;
-    throw c; 
+    throw c;
   } catch(A) {
   }
 }
 
 void throw_derived_catch_base_protected_ptr() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_protected_ptr' which should not throw exceptions
   try {
     C c;
-    throw &c; 
+    throw &c;
   } catch(A *) {
   }
 }
 
 void throw_derived_catch_base_ambiguous() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ambiguous' which should not throw exceptions
   try {
     E e;
-    throw e; 
+    throw e;
   } catch(A) {
   }
 }
 
 void throw_derived_catch_base_ambiguous_ptr() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ambiguous_ptr' which should not throw exceptions
   try {
     E e;
-    throw e; 
+    throw e;
   } catch(A) {
   }
 }
 
 void throw_alias_catch_original() noexcept {
   using alias = int;
 
   try {
struct in_class_init_throws {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'in_class_init_throws' which should not throw exceptions
 };
 
 int main() {
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'main' which should not throw exceptions
   throw 1;
   return 0;
 }
+
+// The following function all incorrectly throw exceptions, *but* calling them
+// should not yield a warning because they are marked as noexcept.
+
+void test_basic_no_throw() noexcept { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'test_basic_no_throw' which should not throw exceptions
+
+void test_basic_throw() noexcept(false) { throw 42; }
+
+void only_calls_non_throwing() noexcept {
+  test_basic_no_throw();
+}
+
+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();
+}
+