This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by JonasToth on Jun 28 2018, 2:30 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

JonasToth created this revision.Jun 28 2018, 2:30 AM
JonasToth updated this revision to Diff 153280.Jun 28 2018, 2:32 AM
  • remove bad code snippet which was dead
Harbormaster completed remote builds in B19818: Diff 153280.
hokein added inline comments.Jun 28 2018, 4:45 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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 added inline comments.Jun 28 2018, 7:09 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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 added inline comments.Jul 20 2018, 2:40 PM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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 updated this revision to Diff 158282.Jul 31 2018, 8:31 AM

rebase to master

JonasToth updated this revision to Diff 158284.Jul 31 2018, 8:33 AM

rebase to master

JonasToth updated this revision to Diff 158286.Jul 31 2018, 8:35 AM

correct rebase.

JonasToth added inline comments.Aug 3 2018, 2:00 PM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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 updated this revision to Diff 159099.Aug 3 2018, 2:47 PM
  • add better diagnostics about template instantiated exception types
JonasToth updated this revision to Diff 160242.Aug 11 2018, 4:15 AM
  • update to current master of clang introduce CHECK-NOTES
  • use new BeginLoc api
JonasToth added inline comments.Aug 14 2018, 8:40 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

@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 added inline comments.Aug 16 2018, 8:15 AM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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 added inline comments.Aug 16 2018, 1:18 PM
test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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.

ping @alexfh what is your take on the issue?

  • Merge branch 'master' into fix_exception
aaron.ballman added inline comments.Aug 27 2018, 5:47 AM
clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
30–32 ↗(On Diff #162448)

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 ↗(On Diff #162448)

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

191 ↗(On Diff #153280)

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 updated this revision to Diff 162945.Aug 28 2018, 1:39 PM
  • [Misc] comment the matcher to better understand it
  • [Fix] adjust the test cases to properly function now

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 ↗(On Diff #162448)

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 ↗(On Diff #162448)

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

191 ↗(On Diff #153280)

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

JonasToth updated this revision to Diff 162950.Aug 28 2018, 1:57 PM
  • [Test] use CHECK-NOTES again based on the fix in check_clang_tidy
JonasToth marked 2 inline comments as done.Aug 28 2018, 2:10 PM

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/?

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.

  • [Misc] migrate to CHECK-NOTES
  • fix outcommented check messages as well
JonasToth marked 2 inline comments as done.Aug 30 2018, 12:54 AM
JonasToth updated this revision to Diff 164690.Sep 10 2018, 9:56 AM
  • 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.

  • use the global ASTMatchers for dependent expressions

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 marked 9 inline comments as done.Sep 14 2018, 6:56 AM

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

alexfh accepted this revision.Sep 17 2018, 6:35 AM

LG

test/clang-tidy/hicpp-exception-baseclass.cpp
191 ↗(On Diff #153280)

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.

This revision is now accepted and ready to land.Sep 17 2018, 6:35 AM
JonasToth updated this revision to Diff 165756.Sep 17 2018, 6:55 AM

get up to date to master

This revision was automatically updated to reflect the committed changes.