[clang-tidy] Ignore declarations in bugprone-exception-escape

[PiotrZSL]

Summary

Warnings will now only be printed for function definitions, not declarations

[isuckatcs]

I think the original behaviour was fine. The warning was emitted at every occurence of the function. It might be confusing if it's only emitted for the definition.

Also what happens in the following scenario:

int indirectly_recursive(int n) noexcept;

int recursion_helper(int n) noexcept {
  indirectly_recursive(n);
}

We know that indirectly_recursive(int n) throws when it shouldn't and that means recursion_helper(int n) will also throw when it shouldn't either.

Is it reported properly with this change?

[PiotrZSL]

@isuckatcs
No it were not fine, check function were executed, ExceptionAnalyzer created only to bail out due to nullptr getBody for functions with only declaration.
For functions with separate declaration and definition entire analysis is executed twice. simply because FunctionDecl::getBody jumps to function definition.
And this simply isn't needed, as we analyse definition anyway, so lets just emit warning for definition.
Now it forces developer to put NOLINT's into both .cpp and .hpp for same issue.

"The warning was emitted at every occurence of the function. It might be confusing if it's only emitted for the definition."
Why ? Issue is in definition, not declaration.

Example with indirectly_recursive & recursion_helper behave in same way, only difference is that warning is emitted only for definition.
"We know that indirectly_recursive(int n) throws when it shouldn't and that means recursion_helper(int n) will also throw when it shouldn't either."
Well no indirectly_recursive throws when it shouldn't but only because thrower throws, recursion_helper does not throw, it crashes. This is other bug that I'm fixing (not under this patch) related to properly handling noexcept keyword.

[isuckatcs]

"The warning was emitted at every occurence of the function. It might be confusing if it's only emitted for the definition."
Why ? Issue is in definition, not declaration.

For me it would be confusing, because the forward declaration is naming the same function as the definition.
If I see that something is reported for the definition but not for the declaration I might think it's a bug in the
tool, like once there is a problem with the function and the other time there isn't. Note that this particular
warning is reported for the function and not for something inside the definition.

Also we have cross translation unit analysis, though I'm not sure this particular checker works there too.
Assuming it does, what happens if I forward declare the function in one translation unit and define it in
a different one? With this change the warning will only be output in the translation unit,the function is
defined in and this might silently hide some other problems in the TU the function is forward declared in.

recursion_helper does not throw, it crashes.

Technically the exception is propagated through the function until a handler is found that catches it.

Example with indirectly_recursive & recursion_helper behave in same way, only difference is that warning is emitted only for definition.

Please add a test case for this regardless of the behaviour to see that the checker handles exception propagation.

This is other bug that I'm fixing (not under this patch) related to properly handling noexcept keyword.

I'm not sure what you mean by bug here.

int recursion_helper(int n) noexcept {
  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in
  // function 'recursion_helper' which should not throw exceptions
  indirectly_recursive(n);
}

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)
    throw n;
  return recursion_helper(n);
}

Because recursion_helper() propagates the thrown object it makes sense to emit a warning for that too.
Also because the warning is emitted upon every propagation it is easy to trace where the exception actually
comes from. Think of it like a stack trace for example.

[PiotrZSL]

@isuckatcs
"Technically the exception is propagated through the function until a handler is found that catches it."
No because indirectly_recursive called from recursion_helper is noexcept, so there will be std::terminate called.

"Also we have cross translation unit analysis"
Not in clang-tidy, and that work based on AST merge, so even if someone manage to run it here, it will work fine.

"Please add a test case for this regardless of the behaviour to see that the checker handles exception propagation."
There is test for that. The one with recursion_helper + indirectly_recursive. Be more specific if you want something else.

"Because recursion_helper() propagates the thrown object it makes sense to emit a warning for that too."
No it's not propagating thrown object. Bug: https://github.com/llvm/llvm-project/issues/54956

As for warnings for forward declaration, what developer have to do with such information ? There is nothing he can change in forward declaration to fix this issue. And putting 2 or more NOLINTS to silence single issue is stupid. Other checks validate only definitions. Forward declarations are forward declarations, they transparent for an exception propagation. And current behaviour introduce also performance penalty, as same thing is done multiple times. If you somehow want to point to developer issue about function declaration, there are notes for that, but still developer know about declarations, and to fix issue He/She don't need to touch this, only definition need to be changed (unlles decides to remove noexcept keyword, but then compiler will tell him about issues). Same goes for pure virtual functions, check does not emit warnings for them just because some implementation throw exception.

[PiotrZSL]

@isuckatcs
"Note that this particular warning is reported for the function and not for something inside the definition."

Function declaration is not a function.

A function declaration is a statement in programming languages that declares the existence of a function, including its name, parameters, and return type (if applicable). It is used to define the function and make it available for use in the program.
On the other hand, a function is a set of instructions that performs a specific task and can be called by other parts of the program. When a function declaration is executed, it creates a function object that can be called as a function. So, while a function declaration is a necessary step in creating a function, it is not the function itself.

[isuckatcs]

No because indirectly_recursive called from recursion_helper is noexcept, so there will be std::terminate called.

I missed that in noexcept functions thrown exceptions are not propagated. In this case I agree that recursion_helper() shouldn't emit a warning.

As for the forward declaration part I think we should wait and see what others think about it.

[PiotrZSL]

@njames93 What do you thing ? Should bugprone-exception-escape provide warnings for all forward declarations and definition, or only for definition.

[carlosgalvezp]

I would agree that the fact that a function throws or not can only be found out when analyzing the function definition (it's impossible to know from the declaration). There's also a duplicate diagnostic that I don't see much value on, so I would agree with this patch unless I'm missing some concrete use case I'm not aware of.

[isuckatcs]

I think there's no point of holding back this patch. Even though I'm not 100% sure we want this change, I say let's merge it and see how our users react.

It's a one line change anyway, so if we receive a lot of complaints, it will be easy to revert.

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

 void ExceptionEscapeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "FunctionsThatShouldNotThrow",
                 RawFunctionsThatShouldNotThrow);
   Options.store(Opts, "IgnoredExceptions", RawIgnoredExceptions);
 }
 
 void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(),
+      functionDecl(isDefinition(),
+                   anyOf(isNoThrow(), cxxDestructorDecl(),
                          cxxConstructorDecl(isMoveConstructor()),
                          cxxMethodDecl(isMoveAssignmentOperator()),
                          hasName("main"), hasName("swap"),
                          isEnabled(FunctionsThatShouldNotThrow)))
           .bind("thrower"),
       this);
 }
 

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-dangling-handle
   <clang-tidy/checks/bugprone/dangling-handle>` check enhancing detection of
   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.
+
 - 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/test/clang-tidy/checkers/bugprone/exception-escape.cpp

 int directly_recursive(int n) noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'directly_recursive' which should not throw exceptions
   if (n == 0)
     thrower(n);
   return directly_recursive(n);
 }
 
 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
 
 int recursion_helper(int n) {
   indirectly_recursive(n);
 }
 
 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)