[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

25

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

22–23

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

22–23

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

[lebedev.ri]

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

22–23

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

25

yes. i use cxxUnresolvedConstructExpr since its shorter.

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

22–23

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

166

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

97–105

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

41–42

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...

97–105

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

[JonasToth]

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

41–42

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.

97–105

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

41–42

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

97–105

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

97–105

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

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–18

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

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–18

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

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–18

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–18

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–18

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–18

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–18

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!

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

 //===--- ExceptionBaseclassCheck.cpp - clang-tidy--------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ExceptionBaseclassCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
-#include <iostream>
-
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace hicpp {
 
 void ExceptionBaseclassCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   Finder->addMatcher(
-      cxxThrowExpr(
-          allOf(
-              has(expr(unless(hasType(cxxRecordDecl(
-                  isSameOrDerivedFrom(hasName("std::exception"))))))),
-              eachOf(has(expr(hasType(namedDecl().bind("decl")))), anything())))
+      cxxThrowExpr(allOf(has(expr(unless(hasType(qualType(hasCanonicalType(
+                             hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(
+                                 hasName("std::exception")))))))))),
+                         has(expr(unless(cxxUnresolvedConstructExpr()))),
+                         eachOf(has(expr(hasType(namedDecl().bind("decl")))),
+                                anything())))
           .bind("bad_throw"),
       this);
 }
 
 void ExceptionBaseclassCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *BadThrow = Result.Nodes.getNodeAs<CXXThrowExpr>("bad_throw");
-  diag(BadThrow->getLocStart(),
-       "throwing an exception whose type is not derived from 'std::exception'")
-      << BadThrow->getSourceRange();
+
+  diag(BadThrow->getSubExpr()->getLocStart(), "throwing an exception whose "
+                                              "type %0 is not derived from "
+                                              "'std::exception'")
+      << BadThrow->getSubExpr()->getType() << BadThrow->getSourceRange();
 
   const auto *TypeDecl = Result.Nodes.getNodeAs<NamedDecl>("decl");
   if (TypeDecl != nullptr)
     diag(TypeDecl->getLocStart(), "type defined here", DiagnosticIDs::Note);
 }
 
 } // namespace hicpp
 } // namespace tidy
 } // namespace clang

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 {};
+class terrible_idea : public non_derived_exception, public derived_exception {};
+
+// FIXME: More complicated kinds of inheritance should be checked later, but there is
+// currently no way use ASTMatchers for this kind of task.
+#if 0
+class bad_inheritance : private std::exception {};
+class no_good_inheritance : protected std::exception {};
+class really_creative : public non_derived_exception, private std::exception {};
+#endif
 
 void problematic() {
   try {
-    throw int(42); // Built in is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
+    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
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
+  throw int(42);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+  try {
+    throw 12;
+    // CHECK-MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  } catch (...) {
+    throw; // Ok, even if the type is not known, conforming code can never rethrow a non-std::exception object.
+  }
 
   try {
-    throw non_derived_exception(); // Some class is not allowed
-// CHECK-MESSAGES: [[@LINE-1]]:5: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+    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
-// CHECK-MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type is not derived from 'std::exception'
-// CHECK-MESSAGES: 8:1: note: type defined here
+  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
+
+// FIXME: More complicated kinds of inheritance should be checked later, but there is
+// currently no way use ASTMatchers for this kind of task.
+#if 0
+  // Handle private inheritance cases correctly.
+  try {
+    throw bad_inheritance();
+    // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+    // CHECK MESSAGES: 10:1: note: type defined here
+    throw no_good_inheritance();
+    // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+    // CHECK MESSAGES: 11:1: note: type defined here
+    throw really_creative();
+    // CHECK MESSAGES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+    // CHECK MESSAGES: 12:1: note: type defined here
+  } catch (...) {
+  }
+  throw bad_inheritance();
+  // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK MESSAGES: 10:1: note: type defined here
+  throw no_good_inheritance();
+  // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK MESSAGES: 11:1: note: type defined here
+  throw really_creative();
+  // CHECK MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK MESSAGES: 12:1: note: type defined here
+#endif
 }
 
 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
+  } catch (deep_hierarchy &e) { // Ok
+  }
+  throw deep_hierarchy(); // Ok
+
+  try {
+    throw terrible_idea();     // Ok, but multiple inheritance isn't clean
+  } catch (std::exception &e) { // Can be caught as std::exception, even with multiple inheritance
+  }
+  throw terrible_idea(); // Ok, but multiple inheritance
+}
+
+// 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: 120: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: 120: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: 123: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);
+  // CHECK MESSAGES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  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(derived_exception); // 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_EXCEPTION(generic_exception<float>); // Ok
+
+  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>();
+  // 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>);
+  // 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>);
+  // 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>();
+  // 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>);
+  // 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_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();
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'TypedefedBad' (aka 'int') is not derived from 'std::exception'
+  // CHECK-MESSAGES: 164:1: note: type defined here
+  throw TypedefedGood(); // Ok
+
+  throw UsingBad();
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: throwing an exception whose type 'UsingBad' (aka 'int') is not derived from 'std::exception'
+  // CHECK-MESSAGES: 166:1: note: type defined here
+  throw UsingGood(); // Ok
 }