Page MenuHomePhabricator

[clang-tidy] misc-throw-with-noexcept
AcceptedPublic

Authored by sbarzowski on Apr 17 2016, 1:25 PM.

Details

Summary

New check that warns about throw in noexcept function.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman requested changes to this revision.Jun 1 2016, 6:41 AM
aaron.ballman edited edge metadata.
aaron.ballman added inline comments.
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
26

Hmm, I wonder if it's trivial enough to filter out throw statements that are inside a try block scope (even without checking the expression and catch block types)?

33

I prefer this be gated on D20428; we should be pushing this functionality down where it belongs instead of replicating it in several different clang-tidy checks.

92

This function will not work with dynamic exception specifications that also specify the function is nonthrowing, e.g., throw().

131

How about wording the diagnostic: "'throw' expression in a function declared with a non-throwing exception specification"

132

const auto *, please (assuming Redecl is a pointer type).

clang-tidy/misc/ThrowWithNoexceptCheck.h
21

What is the false positive rate with this check over a large codebase that uses exceptions?

test/clang-tidy/misc-throw-with-noexcept.cpp
71

There are no tests for dynamic exception specifications.

This revision now requires changes to proceed.Jun 1 2016, 6:41 AM
sbarzowski updated this revision to Diff 59277.Jun 1 2016, 2:20 PM
sbarzowski edited edge metadata.

Thanks for review, guys. I fixed as many easy problems as I had time for today, will continue later.

Prazek added inline comments.Jun 1 2016, 2:25 PM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
26

unless(hasAncestor(cxxTryStmt())) should do the work for almost all cases.
Maybe even something like this to catch only valid try blocks

cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow()).bind("func"))),
unless(hasAncestor(cxxTryStmt().bind("try"), forFunction(hasBody(hasDescendant(equalsBoundNode("try")))))

+ it should check that the throw is not in CXXCatchStmt (unless it is in another try block).

sbarzowski marked 11 inline comments as done.Jun 1 2016, 2:26 PM
sbarzowski added inline comments.
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
34

I agree 100%. I see the code here as a temporary solution, before we have a proper AST functionality for doing that cleanly.

alexfh requested changes to this revision.Jun 17 2016, 5:43 AM
alexfh edited edge metadata.

Removing from my dashboard until D20428 is submitted.

This revision now requires changes to proceed.Jun 17 2016, 5:43 AM
sbarzowski edited edge metadata.
sbarzowski removed rL LLVM as the repository for this revision.

I took advantage of new getExceptionSpecSourceRange (it wasn't available before) instead of getting exception specification manually with the lexer.

Prazek added inline comments.Jan 16 2017, 1:20 PM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
53–54

just add DiagnosticIDs::Note as third argument
This might also change the order of diagnostics as we discussed ofline

alexfh requested changes to this revision.Jan 17 2017, 4:55 AM
alexfh added inline comments.
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
45

Use line comments (//).

54

nit: no leading capitalization in diagnostic messages needed.

test/clang-tidy/misc-throw-with-noexcept.cpp
89

Please add test cases where the function in question is a template itself (and another one where it's a member of a template class) and has a few instantiations.

This revision now requires changes to proceed.Jan 17 2017, 4:55 AM
sbarzowski updated this revision to Diff 85882.Jan 26 2017, 3:27 AM
sbarzowski edited edge metadata.
sbarzowski marked 3 inline comments as done.

Improved messages, added tests with templates, fixed some typos.

sbarzowski marked 5 inline comments as done.Jan 26 2017, 3:28 AM
alexfh requested changes to this revision.Feb 3 2017, 5:04 AM

A couple of nits. Please address Aaron's comment as well.

clang-tidy/misc/ThrowWithNoexceptCheck.cpp
54

nit: remove the FIXME

55

nit: nothrow (without a dash), no colon needed (it will look weird, since the location is mentioned _before_ the message, not after it)

This revision now requires changes to proceed.Feb 3 2017, 5:04 AM
sbarzowski added inline comments.Feb 7 2017, 11:17 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
55

No, it's after the message now. When I changed the level to note the order of messages changed as well.

It looks like that:

/Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
    throw 5;
    ^
/Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24: note: FIX-IT applied suggested code changes
void f_throw_with_ne() noexcept(true) {
                       ^
/Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24: note: in a function declared nothrow here:
void f_throw_with_ne() noexcept(true) {
                       ^

So, should I leave the colon or remove it anyway?

Prazek added inline comments.Feb 7 2017, 1:11 PM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
55

I think that the best way would be to have warnings in order:

warning function declared nothrow here have throw statement inside:
Note: throw statement here

Note: Fixit applied for every other declaration

What do you think Alex?

sbarzowski added inline comments.Feb 7 2017, 1:22 PM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
55

@Prazek
So, do you suggest that we don't emit anything for additional declarations (without -fix)?

BTW (in the current version) I don't know if I can control if FIX-IT goes first or the location message. As you can see in the code the FIX-IT goes after the location.

Prazek added inline comments.Feb 8 2017, 11:43 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
55

Yep, there is no need for user to know all the locations if he doesn't want to perform fixit. This way it is easier to read the warning.

alexfh added inline comments.Feb 15 2017, 8:12 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
55

The FIX-IT applied suggested code changes notes are shown by clang-tidy itself and only when the user passes -fix flag. There's no way to control them from the check (apart from removing fixes or attaching them to a different warning).

Prazek added inline comments.Mar 6 2017, 2:43 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
55

That's right, but does it make sense to show user all the places where the function was declared?
I am not sure what clang would normally do

clang-tidy/misc/ThrowWithNoexceptCheck.h
21

Do you know any good project to check it?

vmiklos added inline comments.
clang-tidy/misc/ThrowWithNoexceptCheck.h
21

LibreOffice might be a candidate, see https://wiki.documentfoundation.org/Development/ClangTidy for details on how to generate a compile database for it (since it does not use cmake), then you can start testing.

sbarzowski added inline comments.Mar 13 2017, 6:43 AM
clang-tidy/misc/ThrowWithNoexceptCheck.h
21

Ok, I was able to run it on most of the HHVM codebase + deps. There were some issues that looked like some autogenerated pieces missing, so it may have been not 100% covered.

The results:
3 occurences in total

  1. rethrow in destructor (http://pastebin.com/JRNMZGev)
  2. false positive "throw and catch in the same function" (http://pastebin.com/14y1AJgM)
  3. noexcept decided during compilation (http://pastebin.com/1jZzRAbC)

That's not too many, but this is a kind of check that I guess would be most useful earlier in the development - ideally before the initial code review.

I'm not sure if we should count (3) as false positive. We could potentially eliminate it, by checking if the expression in noexcept is empty or true literal.

(2) is def. a false positive. The potential handling of this case was already proposed, but I'm not sure if it's worth it. The code in example (2) is ugly and extracting these throwing parts to separate functions looks like a good way to start refactoring.

What do you think?

aaron.ballman added inline comments.Mar 13 2017, 3:09 PM
clang-tidy/misc/ThrowWithNoexceptCheck.h
21

The fact that there's a false positive in the first large project you checked suggests that the false positive is likely worth it to fix. The code may be ugly, but it's not uncommon -- some coding guidelines explicitly disallow use of gotos, and this is a reasonable(ish) workaround for that.

As for #3, I would consider that to be a false-positive as well. A computed noexcept clause is going to be very common, especially in generic code. I think it's probably more important to get this one right than #2.

sbarzowski updated this revision to Diff 97859.May 4 2017, 11:49 AM
sbarzowski edited edge metadata.

Fixed false positive issues

jbcoe added a subscriber: jbcoe.May 4 2017, 12:29 PM
Prazek added inline comments.May 6 2017, 3:04 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
23

If we handle throw() then it should be CPlusPlus

29

is this cland-formatted?

52–54

Here and in many other places - remove unnecessary braces (llvm coding style)

118

clang-format whole file

docs/clang-tidy/checks/misc-throw-with-noexcept.rst
10–11

This is probably outdated. Also mention the other features that you developed here.

sbarzowski updated this revision to Diff 99500.May 18 2017, 2:55 PM
sbarzowski marked 3 inline comments as done.

Docs and cosmetic issues

sbarzowski updated this revision to Diff 99502.May 18 2017, 3:11 PM
sbarzowski marked 8 inline comments as done.

Removed unnecessary colon from message

Prazek added inline comments.May 20 2017, 2:53 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
77

unnecessary braces

114–116

unnecessary braces

127–132

same here

Prazek added inline comments.May 20 2017, 3:23 AM
docs/ReleaseNotes.rst
58

I think this should be in alphabetical order.

As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.

sbarzowski updated this revision to Diff 99670.May 20 2017, 6:35 AM
sbarzowski marked 5 inline comments as done.

Cosmetic

sbarzowski marked 3 inline comments as done.May 20 2017, 6:37 AM
sbarzowski added inline comments.
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
26

Now even the caught type is checked.

clang-tidy/misc/ThrowWithNoexceptCheck.h
21

I have fixed these issues.

sbarzowski marked an inline comment as done.May 20 2017, 6:55 AM
sbarzowski marked 3 inline comments as done.May 20 2017, 7:13 AM
Prazek edited edge metadata.May 21 2017, 1:42 AM

As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.

I think that clang is the right place for this feature, but I am not sure if the patch has all the features (like checking if something will be catched, or not showing warning for conditional noexcepts, which as we have seen
could be a problem for some projects. There also might be some other corner cases that we didn't think about.
Assuming that this patch is ready to land, I would propose to send it to trunk and remove it when the clang's patch will make it to the trunk. I am not sure how much time it will take for other patch to be ready, but maybe we could gather some usefull bug reports in the meantime and also Stanislaw would have a contribution.

Prazek accepted this revision.May 21 2017, 1:43 AM

LGTM, but wait if Aaron will accept it.

As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.

I think that clang is the right place for this feature, but I am not sure if the patch has all the features (like checking if something will be catched, or not showing warning for conditional noexcepts, which as we have seen
could be a problem for some projects. There also might be some other corner cases that we didn't think about.
Assuming that this patch is ready to land, I would propose to send it to trunk and remove it when the clang's patch will make it to the trunk. I am not sure how much time it will take for other patch to be ready, but maybe we could gather some usefull bug reports in the meantime and also Stanislaw would have a contribution.

I don't think it's a good idea to commit this and remove it when the frontend patch lands -- that's just extra code churn and runs the risk of breaking people who are using features out of trunk. I think the better approach is to find the correct home for the functionality based on what *both* patches are trying to accomplish, and combine the test cases from both patches to ensure we get the desired functionality. The two patches appear to be almost entirely overlapping, from my cursory look (of course, I may have missed something).

I think that the frontend is likely the better place for this functionality to go in terms of where users might expect to find it, assuming the false-positive rate is reasonable and it isn't too computationally expensive. My recommendation would be to find the test cases in this patch that are not currently covered by the frontend patch, and ask that they be covered there (possibly even including the fixits).

There is a patch (D33537) for a check which is a superset of this: It does not only check for noexcept (and throw()) functions, but also for destructors, move constructors, move assignments, the main() function, swap() functions and also functions given as option. A more important difference is that we traverse the whole call-chain and check all the throw statements and try-catch blocks so indirectly throwing functions are also reported and no flase positives are caused by throw and catch in the same try block.

There is a patch (D33537) for a check which is a superset of this: [...] A more important difference is that we traverse the whole call-chain and check all the throw statements and try-catch blocks so indirectly throwing functions are also reported and no flase positives are caused by throw and catch in the same try block.

As a matter of fact this check also handles catching, so no false positive here.

But let's get to the point:
For me D33537 and D33333 seem like clearly better options and I think we should proceed with one or both of them instead of this one. However they don't handle (yet) everything that this check does. In particular they don't seem to handle noexcept decided during compilation (example: https://pastebin.com/1jZzRAbC). I've run into this case when I tried this check on HHVM and its dependencies.

Quoting @aaron.ballman

As for #3 [noexcept decided during compilation], I would consider that to be a false-positive as well. A computed noexcept clause is going to be very common, especially in generic code. I think it's probably more important to get this one right than #2 [throw and catch in the same function].

As for this check, I think the development will stop here, no point in duplicating the effort. Maybe the test cases will be of some use to you.

alexfh removed a reviewer: alexfh.Jun 2 2017, 10:38 AM
aaron.ballman resigned from this revision.Nov 26 2017, 6:28 AM
This revision is now accepted and ready to land.Nov 26 2017, 6:28 AM