[clang-tidy] Assume that `noexcept` functions won't throw anything in `bugprone-exception-escape` check

[fwolff]

Summary

Fixes PR#52254. The ExceptionAnalyzer currently recursively checks called functions, but this does not make sense if the called function is marked as noexcept. If it does throw, there should be a warning for that function, but not for every caller. In particular, this can lead to false positives if the called noexcept function calls other, potentially throwing, functions, in a way that ensures they will never actually throw.

[aaron.ballman]

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

201–202

Hmmm, I'm not certain I agree that we should be optimistic. If the exception specification is unknown and we want to be conservative, we can't determine that it might not throw, we can only determine that it might throw.

So this might be the correct behavior for this check but the incorrect behavior for other checks.

One possible approach is to not give an answer until the noexcept specification is resolved (e.g., return an Optional<bool> where None means "I can't answer the question", and callers have to react to that), another would be to pass in an argument specifying whether the caller wants to be conservative or aggressive in this case (that's how we handle "does this expression have side effects" in Clang).

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

311

Do we still issue the diagnostic if you remove this instantiation?

Can you add a second instantiation: est_dependent_noexcept<ShouldThrow<false>>(); to show that it does not diagnose?

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

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

for (unsigned I = 0; I < Try->getNumHandlers(); ++I) {
                                                    CaughtExceptions, CallStack);
           Results.merge(Rethrown);
         }
       }
     }
     Results.merge(Uncaught);
   } else if (const auto *Call = dyn_cast<CallExpr>(St)) {
     if (const FunctionDecl *Func = Call->getDirectCallee()) {
+      bool MightThrow;
+
+      switch (Func->getExceptionSpecType()) {
+      case EST_DynamicNone:
+      case EST_NoThrow:
+      case EST_BasicNoexcept:
+      case EST_DependentNoexcept: // Let's be optimistic here (necessary e.g.
+                                  // for variant assignment; see PR#52254)
+      case EST_NoexceptTrue:
+        MightThrow = false;
+        break;
+
+      default:
+        MightThrow = true;
+        break;
+      }
+
+      if (MightThrow) {
         ExceptionInfo Excs = throwsException(Func, CallStack);
         Results.merge(Excs);
       }
+    }
   } else {
     for (const Stmt *Child : St->children()) {
       ExceptionInfo Excs = throwsException(Child, Caught, CallStack);
       Results.merge(Excs);
     }
   }
   return Results;
 }

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

 
 int indirectly_recursive(int n) noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'indirectly_recursive' which should not throw exceptions
   if (n == 0)
     thrower(n);
   return recursion_helper(n);
 }
 
+// The following functions all incorrectly throw exceptions, *but* calling them
+// should not yield a warning because they are marked as noexcept (or similar).
+void est_dynamic_none() throw() { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_dynamic_none' which should not throw exceptions
+void est_basic_noexcept() noexcept { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_basic_noexcept' which should not throw exceptions
+void est_noexcept_true() noexcept(true) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_noexcept_true' which should not throw exceptions
+template <typename T>
+void est_dependent_noexcept() noexcept(T::should_throw) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_dependent_noexcept<ShouldThrow<true>>' which should not throw exceptions
+template <bool B>
+struct ShouldThrow {
+  static const bool should_throw = B;
+};
+
+void only_calls_non_throwing() noexcept {
+  est_dynamic_none();
+  est_basic_noexcept();
+  est_noexcept_true();
+  est_dependent_noexcept<ShouldThrow<true>>();
+}
+
 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;
 }