[clang-tidy] Support C++17/20 in bugprone-exception-escape

[PiotrZSL]

Summary

Added support for C++17 or later in tests
Pointers to member functions are now handled correctly in C++20

[isuckatcs]

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

160–162

Are you sure noexcept is stored in the type? The test case you modified throw_noexcept_catch_regular() tests this scenario and in that case the types seem to be the same even though one of the is noexcept an the other is not.

If the FIXME is valid the proper way would be to implement it in this patch.

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

414

The name of the function suggests that we throw a noexcept function pointer and catch a non-noexcept one. Please don't change it.

[PiotrZSL]

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

160–162

Yes I'm sure.
C++11 - no warning
C++14 - no warning
C++17 - warning
Looks like since C++17 noexcept is part of type.
Initially I wanted only to enable tests under C++17/20.
That's why "FIXME". I will look into this.

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

414

Ack.

[PiotrZSL]

TODO: Fix function pointer compare

[PiotrZSL]

Update, added support for comparing function pointers without
exception specification.

[PiotrZSL]

Remove unused include

[PiotrZSL]

Ping

[isuckatcs]

Please cover the changes in as much test cases as possible.

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

151

The naming suggests that we check for equality, however in reality we check for convertability here. For example if one of the prototypes has exception spec and the other doesn't have one, this can still return true.

Also instead of manually matching everything, you could take a look at ODRHash or StructuralEquivalenceContext. ODRHash for example doesn't take the FunctionProtoType into account when it generates the hashes AFAIK, so maybe you could use it to avoid these manual checks. I don't know if StructuralEquivalenceContext uses it or not, but you might consider taking a look at that too.

Maybe you can also consider pasting the appropriate section of the standard here, so that whenever someone reads the code they'll understand why are we doing what we're doing.

And please cover this function in positive and negative test cases too.

160–162

I did some investigation regarding this and if I dump the type of void no_throw() noexcept both with -std=c++11 and -std=c++17 I get the same result.

FunctionProtoType 'void (void) noexcept' cdecl
`-BuiltinType 'void'

I agree however that function pointer conversion was not properly implemented.

175

I think we want to call this function for member function pointers too.

The[[ https://github.com/cplusplus/draft/releases | C++ N4917 draft ]] says the following:

7.3.14 Function pointer conversions [conv.fctptr]
A prvalue of type “pointer to noexcept function” can be converted to a prvalue of type “pointer to function”.
The result is a pointer to the function. A prvalue of type “pointer to member of type noexcept function” can
be converted to a prvalue of type “pointer to member of type function”. The result designates the member
function.

293

Please cover this line with both positive and negative test cases.

Also upon looking up both N4849 (C++ 20 draft) and N4917 (C++ 23 draft), they both say for qualification conversion that

each P_i is ... pointer to member of class C_i of type, ...

Why are we not allowing them if the standard is at least C++ 20?

[PiotrZSL]

Update

[PiotrZSL]

Update

[PiotrZSL]

Ping

[isuckatcs]

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

293

Please resolve this thread.

[PiotrZSL]

Add one test.

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

293

Positive case is tested by throw_basefn_catch_derivedfn test and throw_derivedfn_catch_basefn.
Negative is covered by throw_basefn_catch_basefn.

For members there are tests throw_basem_catch_basem, throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers this correctly.

Tested this with GCC, and behavior is proper and independent to C++ standard.

[isuckatcs]

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

293

Tested this with GCC, and behavior is proper and independent to C++ standard.

I don't know how to deal with this tbh. In a static analyzer we usually want to consider what the standard says and not what a specific compiler implementation does, as the compiler can still be buggy, lack the proper support for the standard, etc.

Maybe we can add a FIXME here that explains, why this check is here and that it's the opposite of what the standard says. Then later if it causes issues, it will be easy to see why that happens.

@xazax.hun do you have a prefered method to deal with these situations in the analyzer? If there is one, we could also try it here.

[PiotrZSL]

Ping

[PiotrZSL]

Rebase + Ping

[isuckatcs]

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

293

@PiotrZSL this is still unanswered. The standard says these are allowed, so according to the standard we model something wrong here, unless I'm misunderstanding something.

[PiotrZSL]

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

293

If you talk about about '7.3.5 Qualification conversions' then this is only about cv conversion, not about overall conversion from base to derived type, and this is verify by SatisfiesCVRules lambda.

Without this line, check fails to detect issue in throw_basefn_catch_derivedfn function under C++20.

Other interesting thing is that even this code:

try {
  throw &baseMember::foo;
} catch(const void(baseMember::*)()) {
}

Causes exception not to be catch, but if you remove const then it's catch.
Same behavior is in GCC. I do not sure if 7.3.5 even apply to exceptions, and personally I do not care what standard says, but I care how compilers work.

Right now both GCC and Clang in C++20 and C++17 work in same way, and this `if` should be here so check would be align with how compiler works.
There is
This is anyway a insignificant scenario that most probably wont be used in field.

[PiotrZSL]

Add throw_basefn_catch_const_basefn test

[isuckatcs]

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

293

I do not sure if 7.3.5 (7.3.5 Qualification conversions) even apply to exceptions

[N4917 Post-Summer 2022 C++ working draft] 14.4 Handling an exception
[...]

• the handler is of type cv T or const T& where T is a pointer or pointer-to-member type and E is a pointer or pointer-to-member type that can be converted to T by one or more of
  • a standard pointer conversion (7.3.12) not involving conversions to pointers to private or protected or ambiguous classes
  • a function pointer conversion (7.3.14)
  • a qualification conversion (7.3.6), or <-- here it is

[...]

I do not care what standard says, but I care how compilers work.

Then how would you reason about this snippet? MSVC detects that the exception won't be caught and reports a warning, while gcc and clang fails to do it. In this case different compilers work differently. To be fair, the exception should be caught, since the thrown object is convertible to the handler (which is probably why gcc and clang don't report a warning).

Without this line, check fails to detect issue in throw_basefn_catch_derivedfn function under C++20.

Then do a proper investigation why that happens instead of adding random lines, which contradict what should happen, so that the test can pass. Probably the condition is wrong here and we should return false if !isSameP_i even if the standard is C++20 except for some cases with arrays. Look up the C++17 and the C++20 standards and compare them.

[PiotrZSL]

I do not plan to work on this check anymore, sorry for a wasted time.

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

if (RD->isCompleteDefinition() &&
                  To->getPointeeType().getTypePtr())) {
       return true;
     }
   }
 
   return false;
 }
 
 bool isFunctionPointerConvertible(QualType From, QualType To) {
   if (!From->isFunctionPointerType() && !From->isFunctionType() &&
       !From->isMemberFunctionPointerType())
     return false;
 
   if (!To->isFunctionPointerType() && !To->isMemberFunctionPointerType())
     return false;
 
   if (To->isFunctionPointerType()) {
+    // FIXME: Two function pointers can differ in 'noexcept', but they still
+    // should be considered to be same, now this triggers false-positive because
+    // Type* != Type*.
     if (From->isFunctionPointerType())
       return To->getPointeeType() == From->getPointeeType();
 
     if (From->isFunctionType())
       return To->getPointeeType() == From;
 
     return false;
   }
 
   if (To->isMemberFunctionPointerType()) {
     if (!From->isMemberFunctionPointerType())
       return false;
 
     const auto *FromMember = cast<MemberPointerType>(From);
     const auto *ToMember = cast<MemberPointerType>(To);
 
     // Note: converting Derived::* to Base::* is a different kind of conversion,
     // called Pointer-to-member conversion.
     return FromMember->getClass() == ToMember->getClass() &&
            FromMember->getPointeeType() == ToMember->getPointeeType();
   }
while (isValidP_i(From) && isValidP_i(To)) {
     // Remove every sugar.
     From = From.getCanonicalType();
     To = To.getCanonicalType();
 
     if (!SatisfiesCVRules(From, To))
       return false;
 
     if (!isSameP_i(From, To)) {
-      if (LangOpts.CPlusPlus20) {
-        if (From->isConstantArrayType() && !To->isIncompleteArrayType())
-          return false;
+      if (!LangOpts.CPlusPlus20)
+        return false;
+
 
-        if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
-          return false;
 
-      } else {
+      if (From->isMemberPointerType() || To->isMemberPointerType())
         return false;
-      }
-    }
 
     ++I;
     std::optional<QualType> FromPointeeOrElem =
         getPointeeOrArrayElementQualType(From);
     std::optional<QualType> ToPointeeOrElem =
         getPointeeOrArrayElementQualType(To);
 
     assert(FromPointeeOrElem &&

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

-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN:     -config="{CheckOptions: [ \
 // RUN:         {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN:         {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN:     ]}" \
 // RUN:     -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
     // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~throwing_destructor' which should not throw exceptions
     throw 1;
   }
 };
 

 }
 
 namespace c {
   inline int foo() noexcept { return 0; };
 
   void throw_noexcept_catch_regular() noexcept {
     try {
       throw &foo;
-    } catch(int (*)()) {
+    } catch(int (*)() noexcept) {
     }
   }
 }
 
 struct baseMember {
     int *iptr;
     virtual void foo(){};
 };