[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

1 files changed, 26 insertions, 8 deletions

diff --git a/clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp b/clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
index 3ea56d2f734..b299151ccea 100644
--- 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);
 }