[clang-tidy] fix PR37913, templated exception factory diagnosed correctly

Summary:
PR37913 documents wrong behaviour for a templated exception factory function.
The check does misidentify dependent types as not derived from std::exception.

The fix to this problem is to ignore dependent types, the analysis works correctly
on the instantiated function.

Reviewers: aaron.ballman, alexfh, hokein, ilya-biryukov

Reviewed By: alexfh

Subscribers: lebedev.ri, nemanjai, mgorny, kbarton, xazax.hun, cfe-commits

Differential Revision: https://reviews.llvm.org/D48714

llvm-svn: 342393

diff --git a/clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp b/clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
index 3ea56d2..b299151 100644 (file)
--- a/clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
+++ b/clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
@@ -22,26 +22,44 @@ void ExceptionBaseclassCheck::registerMatchers(MatchFinder *Finder) {
     return;
 
   Finder->addMatcher(
-      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())))
+      cxxThrowExpr(
+          allOf(
+              unless(has(expr(anyOf(isTypeDependent(), isValueDependent())))),
+              // The thrown value is not derived from 'std::exception'.
+              has(expr(unless(hasType(
+                  qualType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+                      isSameOrDerivedFrom(hasName("::std::exception")))))))))),
+              // This condition is always true, but will bind to the
+              // template value if the thrown type is templated.
+              anyOf(has(expr(hasType(
+                        substTemplateTypeParmType().bind("templ_type")))),
+                    anything()),
+              // Bind to the declaration of the type of the value that
+              // is thrown. 'anything()' is necessary to always suceed
+              // in the 'eachOf' because builtin types are not
+              // 'namedDecl'.
+              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");
+  assert(BadThrow && "Did not match the throw expression");
 
   diag(BadThrow->getSubExpr()->getBeginLoc(), "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)
+  if (const auto *Template =
+          Result.Nodes.getNodeAs<SubstTemplateTypeParmType>("templ_type"))
+    diag(BadThrow->getSubExpr()->getBeginLoc(),
+         "type %0 is a template instantiation of %1", DiagnosticIDs::Note)
+        << BadThrow->getSubExpr()->getType()
+        << Template->getReplacedParameter()->getDecl();
+
+  if (const auto *TypeDecl = Result.Nodes.getNodeAs<NamedDecl>("decl"))
     diag(TypeDecl->getBeginLoc(), "type defined here", DiagnosticIDs::Note);
 }
 

diff --git a/clang-tools-extra/test/clang-tidy/hicpp-exception-baseclass.cpp b/clang-tools-extra/test/clang-tidy/hicpp-exception-baseclass.cpp
index fdf8093..b5e405a 100644 (file)
--- a/clang-tools-extra/test/clang-tidy/hicpp-exception-baseclass.cpp
+++ b/clang-tools-extra/test/clang-tidy/hicpp-exception-baseclass.cpp
@@ -2,6 +2,7 @@
 
 namespace std {
 class exception {};
+class invalid_argument : public exception {};
 } // namespace std
 
 class derived_exception : public std::exception {};
@@ -36,12 +37,12 @@ void problematic() {
   try {
     throw non_derived_exception();
     // CHECK-NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-    // CHECK-NOTES: 9:1: note: type defined here
+    // CHECK-NOTES: 10:1: note: type defined here
   } catch (non_derived_exception &e) {
   }
   throw non_derived_exception();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-  // CHECK-NOTES: 9:1: note: type defined here
+  // CHECK-NOTES: 10: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.
@@ -49,25 +50,25 @@ void problematic() {
   // 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
+    // CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+    // CHECK NOTES: 11: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
+    // CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+    // CHECK NOTES: 12: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
+    // CHECK NOTES: [[@LINE-1]]:11: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+    // CHECK NOTES: 13: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 11: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'no_good_inheritance' is not derived from 'std::exception'
+  // CHECK NOTES: 12: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
+  // CHECK NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'really_creative' is not derived from 'std::exception'
+  // CHECK NOTES: 13:1: note: type defined here
 #endif
 }
 
@@ -91,24 +92,40 @@ void allowed_throws() {
   throw deep_hierarchy(); // Ok
 
   try {
-    throw terrible_idea();     // Ok, but multiple inheritance isn't clean
+    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
 }
 
+void test_lambdas() {
+  auto BadLambda = []() { throw int(42); };
+  // CHECK-NOTES: [[@LINE-1]]:33: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  auto GoodLambda = []() { throw derived_exception(); };
+}
+
 // Templated function that throws exception based on template type
 template <typename T>
 void ThrowException() { throw T(); }
 // CHECK-NOTES: [[@LINE-1]]:31: warning: throwing an exception whose type 'bad_generic_exception<int>' is not derived from 'std::exception'
-// CHECK-NOTES: 120:1: note: type defined here
-// CHECK-NOTES: [[@LINE-3]]:31: warning: throwing an exception whose type 'bad_generic_exception<std::exception>' is not derived from 'std::exception'
-// CHECK-NOTES: 120:1: note: type defined here
-// CHECK-NOTES: [[@LINE-5]]:31: warning: throwing an exception whose type 'exotic_exception<non_derived_exception>' is not derived from 'std::exception'
-// CHECK-NOTES: 123:1: note: type defined here
-// CHECK-NOTES: [[@LINE-7]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-// CHECK-NOTES: [[@LINE-8]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
-// CHECK-NOTES: 9:1: note: type defined here
+// CHECK-NOTES: [[@LINE-2]]:31: note: type 'bad_generic_exception<int>' is a template instantiation of 'T'
+// CHECK-NOTES: [[@LINE+25]]:1: note: type defined here
+
+// CHECK-NOTES: [[@LINE-5]]:31: warning: throwing an exception whose type 'bad_generic_exception<std::exception>' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-6]]:31: note: type 'bad_generic_exception<std::exception>' is a template instantiation of 'T'
+// CHECK-NOTES: [[@LINE+21]]:1: note: type defined here
+
+// CHECK-NOTES: [[@LINE-9]]:31: warning: throwing an exception whose type 'exotic_exception<non_derived_exception>' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-10]]:31: note: type 'exotic_exception<non_derived_exception>' is a template instantiation of 'T'
+// CHECK-NOTES: [[@LINE+20]]:1: note: type defined here
+
+// CHECK-NOTES: [[@LINE-13]]:31: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-14]]:31: note: type 'int' is a template instantiation of 'T'
+
+// CHECK-NOTES: [[@LINE-16]]:31: warning: throwing an exception whose type 'non_derived_exception' is not derived from 'std::exception'
+// CHECK-NOTES: [[@LINE-17]]:31: note: type 'non_derived_exception' is a template instantiation of 'T'
+// CHECK-NOTES: 10: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();
@@ -125,17 +142,14 @@ 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-NOTES: [[@LINE-1]]:3: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
-  // CHECK-NOTES: [[@LINE-25]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
+  // CHECK-NOTES: [[@LINE-22]]:35: note: expanded from macro 'THROW_BAD_EXCEPTION'
   THROW_GOOD_EXCEPTION;
   THROW_DERIVED_EXCEPTION;
 
@@ -144,20 +158,17 @@ void generic_exceptions() {
 
   throw bad_generic_exception<int>();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_generic_exception<int>' is not derived from 'std::exception'
-  // CHECK-NOTES: 120:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-24]]:1: note: type defined here
   throw bad_generic_exception<std::exception>();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'bad_generic_exception<std::exception>' is not derived from 'std::exception'
-  // CHECK-NOTES: 120:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-27]]:1: note: type defined here
   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-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'exotic_exception<non_derived_exception>' is not derived from 'std::exception'
-  // CHECK-NOTES: 123:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-30]]:1: note: type defined here
   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
@@ -172,11 +183,102 @@ using UsingGood = deep_hierarchy;
 void typedefed() {
   throw TypedefedBad();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'TypedefedBad' (aka 'int') is not derived from 'std::exception'
-  // CHECK-NOTES: 167:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-8]]:1: note: type defined here
   throw TypedefedGood(); // Ok
 
   throw UsingBad();
   // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'UsingBad' (aka 'int') is not derived from 'std::exception'
-  // CHECK-NOTES: 169:1: note: type defined here
+  // CHECK-NOTES: [[@LINE-11]]:1: note: type defined here
   throw UsingGood(); // Ok
 }
+
+// Fix PR37913
+struct invalid_argument_maker {
+  ::std::invalid_argument operator()() const;
+};
+struct int_maker {
+  int operator()() const;
+};
+
+template <typename T>
+void templated_thrower() {
+  throw T{}();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+}
+template <typename T>
+void templated_thrower2() {
+  T ExceptionFactory; // This test found a <dependant-type> which did not happend with 'throw T{}()'
+  throw ExceptionFactory();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+}
+
+void exception_created_with_function() {
+  templated_thrower<invalid_argument_maker>();
+  templated_thrower<int_maker>();
+
+  templated_thrower2<invalid_argument_maker>();
+  templated_thrower2<int_maker>();
+
+  throw invalid_argument_maker{}();
+  throw int_maker{}();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+}
+
+struct invalid_argument_factory {
+  ::std::invalid_argument make_exception() const;
+};
+
+struct int_factory {
+  int make_exception() const;
+};
+
+template <typename T>
+void templated_factory() {
+  T f;
+  throw f.make_exception();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+}
+template <typename T>
+void templated_factory2() {
+  throw T().make_exception();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+}
+
+void exception_from_factory() {
+  templated_factory<invalid_argument_factory>();
+  templated_factory<int_factory>();
+
+  templated_factory2<invalid_argument_factory>();
+  templated_factory2<int_factory>();
+
+  throw invalid_argument_factory().make_exception();
+  throw int_factory().make_exception();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+
+  invalid_argument_factory inv_f;
+  throw inv_f.make_exception();
+
+  int_factory int_f;
+  throw int_f.make_exception();
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+}
+
+template <typename T>
+struct ThrowClassTemplateParam {
+  ThrowClassTemplateParam() { throw T(); }
+  // CHECK-NOTES: [[@LINE-1]]:37: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+  // CHECK-NOTES: [[@LINE-2]]:37: note: type 'int' is a template instantiation of 'T'
+};
+
+template <int V>
+struct ThrowValueTemplate {
+  ThrowValueTemplate() { throw V; }
+  // CHECK-NOTES: [[@LINE-1]]:32: warning: throwing an exception whose type 'int' is not derived from 'std::exception'
+};
+
+void class_templates() {
+  ThrowClassTemplateParam<int> IntThrow;
+  ThrowClassTemplateParam<std::invalid_argument> ArgThrow;
+
+  ThrowValueTemplate<42> ValueThrow;
+}