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

[JonasToth]

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.

[JonasToth]

• remove bad code snippet which was dead

[hokein]

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

195

I think giving message on the template function here is confusing to users even it gets instantiated somewhere in this TU -- because users have to find the location that triggers the template instantiation.

Maybe

1. Add a note which gives the instantiation location to the message, or
2. ignore template case (some clang-tidy checks do this)

[alexfh]

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

195

In this particular case it seems to be useful to get warnings from template instantiations. But the message will indeed be confusing.

Ideally, the message should have "in instantiation of xxx requested here" notes attached, as clang warnings do. But this is not working automatically, and it's implemented in Sema (Sema::PrintInstantiationStack() in lib/Sema/SemaTemplateInstantiate.cpp).

I wonder whether it's feasible to produce similar notes after Sema is dead? Maybe not the whole instantiation stack, but at least it should be possible to figure out that the enclosing function is a template instantiation or is a member function of an type that is an instantiation of a template. That would be useful for other checks as well.

[JonasToth]

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

195

It should be possible to figure out, that the type comes from template instantiation and that information could be added to the warning.

I will take a look at Sema and think about something like this. Unfortunatly i dont have a lot of time :/

[JonasToth]

rebase to master

[JonasToth]

rebase to master

[JonasToth]

correct rebase.

[JonasToth]

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

195

I did look further into the issue, i think it is non-trivial.

The newly added case is not a templated exception perse, but there is a exception-factory, which is templated, that creates a normal exception.

I did add another note for template instantiations, but i could not figure out a way to give better diagnostics for the new use-case.

[JonasToth]

• add better diagnostics about template instantiated exception types

[JonasToth]

• update to current master of clang introduce CHECK-NOTES
• use new BeginLoc api

[JonasToth]

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

195

@hokein and @alexfh Do you still have your concerns (the exception is not a template value, but the factory creating them) or is this fix acceptable?

[hokein]

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

195

I agree this is non-trivial. If we can't find a good solution at the moment, I'd prefer to ignore this case instead of adding some workarounds in the check, what do you think?

[JonasToth]

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

195

Honestly I would let it as is. This test case is not well readable, but if we have something similar to

template <typename T>
void SomeComplextFunction() {
    T ExceptionFactory;

   if (SomeCondition) 
     throw ExceptionFactory();
}

It is not that bad. And the check is still correct, just the code triggering this condition just hides whats happening.

[JonasToth]

ping @alexfh what is your take on the issue?

[JonasToth]

• Merge branch 'master' into fix_exception

[aaron.ballman]

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

30–32

This is a strange formulation where you have anyOf(..., anything()); can you explain why that's needed?

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

39

Spurious newline change? It seems to cause a lot of churn in the test.

195

I don't think the diagnostic in this test is too confusing. Having the instantiation stack would be great, but that requires Sema support that we don't have access to, unfortunately.

The instantiation note currently isn't being printed in the test case, but I suspect that will add a bit of extra clarity to the message.

[JonasToth]

• [Misc] comment the matcher to better understand it
• [Fix] adjust the test cases to properly function now

[JonasToth]

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

30–32

I added comments for each part of the matcher. Do you think it clarifies? It is just a small hack to conditionally match on something :)

But I honestly had to think a little until i remembered why i did it :D

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

39

Tests were broken as well. more in the other comment.

195

The template note does not apply here, because the thrown value is not templated.

[JonasToth]

• [Test] use CHECK-NOTES again based on the fix in check_clang_tidy

[lebedev.ri]

In D48714#1216537, @JonasToth wrote:

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

So what was the issue? Do you get the same results if you undo the D51381 and s/CHECK-MESSAGES/CHECK-NOTES/?

[JonasToth]

In D48714#1216989, @lebedev.ri wrote:

In D48714#1216537, @JonasToth wrote:

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

So what was the issue? Do you get the same results if you undo the D51381 and s/CHECK-MESSAGES/CHECK-NOTES/?

You are right that replacing all of it with CHECK-NOTES works as well.
FileCheck is run twice if you have FIXMES as well. Having another run for the notes is consistent with how it worked before.
If we go for the catch-all-with-one approach it might be a good idea to ensure that only one of CHECK-MESSAGES or CHECK-NOTES is present in the file and adjust the check_clang_tidy.py script a little.

[JonasToth]

• [Misc] migrate to CHECK-NOTES
• fix outcommented check messages as well

[JonasToth]

• Fix typedependant expressions are ignored

This one took me way longer then it should have, but I discovered new templated
code constructs and added test accordingly.

[JonasToth]

• use the global ASTMatchers for dependent expressions

[JonasToth]

I added more testcases for templates and improved the diagnostics with notes. This includes newly discovered false positives related to uninstantiated templates.

@alexfh, @hokein Would you like to see better diagnostics?

[JonasToth]

I do consider the diagnostic thing as resolved given the lack of further comments.

[alexfh]

LG

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

195

The diagnostic here wouldn't be ideal, but if a proper fix is not feasible, I'd probably leave this as is. It's not the only check where instantiation stack in diagnostics would be helpful, and I don't expect this case to be frequent - if it is, let's wait for bug reports.

[JonasToth]

get up to date to master

clang-tidy/hicpp/ExceptionBaseclassCheck.cpp

 namespace tidy {
 namespace hicpp {
 
 void ExceptionBaseclassCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     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(
+              has(expr(unless(hasType(
+                  qualType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+                      isSameOrDerivedFrom(hasName("std::exception")))))))))),
+              anyOf(has(expr(hasType(
+                        substTemplateTypeParmType().bind("templ_type")))),
+                    anything()),
+              has(expr(allOf(
+                  unless(cxxUnresolvedConstructExpr()),
+                  unless(callExpr(callee(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");
+  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 *Template =
+      Result.Nodes.getNodeAs<SubstTemplateTypeParmType>("templ_type");
+  if (Template) {
+    diag(BadThrow->getSubExpr()->getLocStart(),
+         "type %0 is a template instantiation of %1", DiagnosticIDs::Note)
+        << BadThrow->getSubExpr()->getType()
+        << Template->getReplacedParameter()->getDecl();
+  }
   const auto *TypeDecl = Result.Nodes.getNodeAs<NamedDecl>("decl");
   if (TypeDecl != nullptr)
     diag(TypeDecl->getBeginLoc(), "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 {};
+class invalid_argument : public 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
try {
     throw 12;
     // CHECK-NOTES: [[@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();
+
     // 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
   } 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
 
 // 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
+    // CHECK MESSAGES: 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 MESSAGES: 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 MESSAGES: 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 MESSAGES: 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 MESSAGES: 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 MESSAGES: 13:1: note: type defined here
 #endif
 }
 
 void allowed_throws() {
   try {
     throw std::exception();     // Ok
   } catch (std::exception &e) { // Ok
   }
   throw std::exception();
 
   try {
     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
+    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(); }

 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
+  // CHECK MESSAGES: 10: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'
   THROW_GOOD_EXCEPTION;
void typedefed() {
   // CHECK-NOTES: 167: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
   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-MESSAGES: [[@LINE-1]]:34: 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>();
+
+  throw invalid_argument_maker{}();
+}