[clang-tidy] Improve hicpp-exception-baseclass to handle generic code better

[JonasToth]

Summary

This patch is a followup to the first revision D36583, that had problems with
generic code and its diagnostic messages, which were found by @lebedev.ri

[lebedev.ri]

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

29 ↗

(On Diff #112333)

As discussed, the second option would be to check for cxxUnresolvedConstructExpr

docs/ReleaseNotes.rst

60 ↗

(On Diff #112333)

Duplicate

test/clang-tidy/hicpp-exception-baseclass.cpp

14

Ok, this is where i got fully confused.
Looks like all the checks are outdated (do not contain the exception type).

[JonasToth]

• remove duplicated release note

[JonasToth]

docs/ReleaseNotes.rst

60 ↗

(On Diff #112333)

true!

test/clang-tidy/hicpp-exception-baseclass.cpp

14

what do you mean? Should i write something like ...; got %0" << type?

[lebedev.ri]

test/clang-tidy/hicpp-exception-baseclass.cpp

14

I don't understand how these checks could match.
Your diagnostic is:

"throwing an exception whose type %0 is not derived from 'std::exception'"

So the checks should be

// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type int is not derived from 'std::exception'

Or does check_clang_tidy silently ignore substrings, too?
(i know it does ignore strings at the end of check lines, by not automatically appending $)

[JonasToth]

• adjusting diagnostics

[JonasToth]

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

29 ↗

(On Diff #112333)

yes. i use cxxUnresolvedConstructExpr since its shorter.

test/clang-tidy/hicpp-exception-baseclass.cpp

14

yes. the type was missing in the tests. fixed, ty :)

[JonasToth]

as noted in one comment, there are false positives with the current matcher, since it does not look through typedefs/aliases. is there a way to do this easily?

test/clang-tidy/hicpp-exception-baseclass.cpp

123

here and in line 127 are false positives.
the matcher does look through typedefs. is there a way to accomplish this?

[JonasToth]

• handle typedefs correctly now

[JonasToth]

test/clang-tidy/hicpp-exception-baseclass.cpp

54–62

these diagnostic come from the many instantiations of this function.
do you think, they should exist or rather not?

[lebedev.ri]

I did not test this yet, but looks better :)

test/clang-tidy/hicpp-exception-baseclass.cpp

26–27

Could you please update the comments?
I find them misleading. What does this bad mean?
That this line is not handled correctly? Then please use FIXME: wrong handling
That this line is invalid? The // CHECK-MESSAGES: already states that...

54–62

They definitively should exist.
But they also should ideally point to the origin of the T.

[JonasToth]

test/clang-tidy/hicpp-exception-baseclass.cpp

26–27

yes, they are left over when i started writing the test and did not know the diagnostics. These comments help, when something is reported from clang-tidy, to instantly see if this is correctly found, since the source line is shown. I can remove them.

54–62

they kinda do. when templates are used, the template class is pointed to, but not the instantiation. At least they shouldn't point to the <typename T> anymore.

[JonasToth]

• removing trailing comments

[JonasToth]

fix patch, to diff against master again

[JonasToth]

struggling with arc...

[lebedev.ri]

Thank you for working on this!
I just tried, and the original false-positive i was hitting is now gone.

So as far i'm concerned, this is good to go.

test/clang-tidy/hicpp-exception-baseclass.cpp

26–27

These comments help, when something is reported from clang-tidy, to instantly see if this is correctly found, since the source line is shown. I can remove them.

Aha. I'd use some less confusing comments then, maybe

54–62

but not the instantiation

It would be best if they would, but looking at the AST, i'm not sure how to do that...
https://godbolt.org/g/ejScyZ

Maybe someone else knows.

[JonasToth]

test/clang-tidy/hicpp-exception-baseclass.cpp

54–62

it is probably too much magic anyway. I thinnk you would need to trace the callgraph for that, which is probably not worth it (any i am not able to program it either :D )

[JonasToth]

@aaron.ballman is it ok for you as well? otherwise i would commit it.

[aaron.ballman]

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

41 ↗

(On Diff #113214)

Can you add a test that uses a rethrow? e.g.,

try {
  throw 12; // Diagnose this
} catch (...) {
  throw; // Does not diagnose this
}

test/clang-tidy/hicpp-exception-baseclass.cpp

9

Can you add a test that uses multiple inheritance? e.g.,

class terrible_idea : public non_derived_exception, public derived_exception {};

Also, is private inheritance also acceptable, or does it need to be public inheritance? I kind of get the impression it needs to be public, because the goal appears to be that you should always be able to catch a std::exception instance, and you can't do that if it's privately inherited. That should have a test as well.

[JonasToth]

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

41 ↗

(On Diff #113214)

I added this case, but i have questions on this one.

The type of the caught exception is not know in general right?
In this case, a deeper analysis would find that the second throw is problematic, too.
But since the rethrow depends on the original thrown type, conforming code could never rethrow a bad exception.

test/clang-tidy/hicpp-exception-baseclass.cpp

9

The rules do not state directly, that it must be inherited public, but i dont see a good reason to allow non-public inheritance.
Another thing is, that you can always call e.what() on public derived exceptions.

Multiple inheritance is harder, since the type is still a std::exception. One could catch it and use its interface, so these reasons are gone to disallow it.

[JonasToth]

• added inheritance cases, adjusted matcher is required

[aaron.ballman]

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

41 ↗

(On Diff #113214)

I think it's fine to not diagnose the rethrow -- as you mentioned, the only way for it to be a problem is when the original throw is a problem and that will get diagnosed elsewhere.

test/clang-tidy/hicpp-exception-baseclass.cpp

9

The rules do not state directly, that it must be inherited public, but i dont see a good reason to allow non-public inheritance.

Agreed.

Another thing is, that you can always call e.what() on public derived exceptions.

Agreed.

Multiple inheritance is harder, since the type is still a std::exception. One could catch it and use its interface, so these reasons are gone to disallow it.

I think the multiple inheritance case should not diagnose because it meets the HIC++ requirement of being derived from std::exception.

[JonasToth]

test/clang-tidy/hicpp-exception-baseclass.cpp

9

I have a problem with implementing the inheritance rules.

From the Matchers, there seems to be no way to test, if the inheritance is public. Should i work a new matcher for that, or rather move the tests, if the type holds all conditions into the callback function. This would mean, that every throw gets matched.

[aaron.ballman]

test/clang-tidy/hicpp-exception-baseclass.cpp

9

I would say you can handle private inheritance in a follow-up patch. I would look into changing the isPublic() (and related) matchers to handle inheritance (might as well handle isVirtual() at the same time, too), though I've not given this interface a ton of thought.

[JonasToth]

test/clang-tidy/hicpp-exception-baseclass.cpp

9

Ok. I will commit this one with according FIXME: sections and will #if 0 the currently offending check-messages.

from the usability, something like:
isSameOrDerivedFrom("std::exception", isPublic()) would be nice, but i dont know if this is even possible.
In that sense, the other modifiers could be used as well (isVirtual(), isProtected()...).

[aaron.ballman]

test/clang-tidy/hicpp-exception-baseclass.cpp

9

I'm not too certain (maybe @klimek or @sbenza knows more), but I think you can modify isDerivedFrom() to accept an additional matcher so that could can write isDerivedFrom(hasName("X"), allOf(isPublic(), isVirtual())) and then thread that change through to the other derived matchers. I would guess this means isPublic() (et al) would need to accept a CXXBaseSpecifier as well as the declarations they currently accept.

[JonasToth]

• adjusted expected diagnostics
• adjust diagnostics and remove private inheritance cases

[aaron.ballman]

LGTM!

test/clang-tidy/hicpp-exception-baseclass.cpp

 // RUN: %check_clang_tidy %s hicpp-exception-baseclass %t -- -- -fcxx-exceptions
 
 namespace std {
 class exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
 class deep_hierarchy : public derived_exception {};
 class non_derived_exception {};
 
 void problematic() {
   try {
-    throw int(42); // Built in is not allowed
+    throw int(42);
     // CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   } catch (int e) {
   }
-  throw int(42); // Bad
+  throw int(42);
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
 
   try {
-    throw non_derived_exception(); // Some class is not allowed
+    throw non_derived_exception();
     // CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
     // CHECK-MESSAGES: 9:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
-  throw non_derived_exception(); // Bad
+  throw non_derived_exception();
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
   // CHECK-MESSAGES: 9:1: note: type defined here
 }
 
 void allowed_throws() {
   try {
-    throw std::exception(); // Ok
+    throw std::exception();     // Ok
   } catch (std::exception &e) { // Ok
   }
   throw std::exception();
 
   try {
-    throw derived_exception(); // Ok
+    throw derived_exception();     // Ok
   } catch (derived_exception &e) { // Ok
   }
   throw derived_exception(); // Ok
 
   try {
-    throw deep_hierarchy(); // Ok, multiple levels of inheritance
+    throw deep_hierarchy();     // Ok, multiple levels of inheritance
   } catch (deep_hierarchy &e) { // Ok
   }
   throw deep_hierarchy(); // Ok
 }
 
 // Templated function that throws exception based on template type
 template <typename T>
 void ThrowException() { throw T(); }
 // CHECK-MESSAGES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception<int>' is not derived from 'std::exception'
 // CHECK-MESSAGES: 71:1: note: type defined here
 // CHECK-MESSAGES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception<std::exception>' is not derived from 'std::exception'
 // CHECK-MESSAGES: 71:1: note: type defined here
 // CHECK-MESSAGES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception<non_derived_exception>' is not derived from 'std::exception'
 // CHECK-MESSAGES: 74:1: note: type defined here
 // CHECK-MESSAGES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
 // CHECK-MESSAGES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
 // CHECK-MESSAGES: 9:1: note: type defined here
 #define THROW_EXCEPTION(CLASS) ThrowException<CLASS>()
 #define THROW_BAD_EXCEPTION throw int(42);
 #define THROW_GOOD_EXCEPTION throw std::exception();
 #define THROW_DERIVED_EXCEPTION throw deep_hierarchy();
 
 template <typename T>
 class generic_exception : std::exception {};
 
 template <typename T>
 class bad_generic_exception {};
 
 template <typename T>
 class exotic_exception : public T {};
 
 void generic_exceptions() {
-  THROW_EXCEPTION(int); // Bad
+  THROW_EXCEPTION(int);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-  THROW_EXCEPTION(non_derived_exception); // Bad
+  THROW_EXCEPTION(non_derived_exception);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
   // CHECK MESSAGES: 9:1: note: type defined here
-  THROW_EXCEPTION(std::exception); // Ok
+  THROW_EXCEPTION(std::exception);    // Ok
   THROW_EXCEPTION(derived_exception); // Ok
-  THROW_EXCEPTION(deep_hierarchy); // Ok
+  THROW_EXCEPTION(deep_hierarchy);    // Ok
 
   THROW_BAD_EXCEPTION;
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
   // CHECK-MESSAGES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
   THROW_GOOD_EXCEPTION;
   THROW_DERIVED_EXCEPTION;
 
-  throw generic_exception<int>(); // Ok,
+  throw generic_exception<int>();            // Ok,
   THROW_EXCEPTION(generic_exception<float>); // Ok
 
-  throw bad_generic_exception<int>(); // Bad, not derived
+  throw bad_generic_exception<int>();
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_generic_exception<int>' is not derived from 'std::exception'
-  throw bad_generic_exception<std::exception>(); // Bad as well, since still not derived
+  throw bad_generic_exception<std::exception>();
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_generic_exception<std::exception>' is not derived from 'std::exception'
-  THROW_EXCEPTION(bad_generic_exception<int>); // Bad
+  THROW_EXCEPTION(bad_generic_exception<int>);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'bad_generic_exception<int>' is not derived from 'std::exception'
-  THROW_EXCEPTION(bad_generic_exception<std::exception>); // Bad
+  THROW_EXCEPTION(bad_generic_exception<std::exception>);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'bad_generic_exception<std::exception>' is not derived from 'std::exception'
 
-  throw exotic_exception<non_derived_exception>(); // Bad
+  throw exotic_exception<non_derived_exception>();
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'exotic_exception<non_derived_exception>' is not derived from 'std::exception'
-  THROW_EXCEPTION(exotic_exception<non_derived_exception>); // Bad
+  THROW_EXCEPTION(exotic_exception<non_derived_exception>);
   // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'exotic_exception<non_derived_exception>' is not derived from 'std::exception'
 
-  throw exotic_exception<derived_exception>(); // Ok
+  throw exotic_exception<derived_exception>();          // Ok
   THROW_EXCEPTION(exotic_exception<derived_exception>); // Ok
 }
 
 // Test for typedefed exception types
 typedef int TypedefedBad;
 typedef derived_exception TypedefedGood;
 using UsingBad = int;
 using UsingGood = deep_hierarchy;
 
 void typedefed() {
-  throw TypedefedBad(); // Bad
+  throw TypedefedBad();
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'TypedefedBad' (aka 'int') is not derived from 'std::exception'
   throw TypedefedGood(); // Ok
 
-  throw UsingBad(); // Bad
+  throw UsingBad();
   // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'UsingBad' (aka 'int') is not derived from 'std::exception'
   throw UsingGood(); // Ok
 }