New check that warns about throw in noexcept function.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Please add documentation and mention this check in docs/clang-tidy/checks/list.rst and docs/ReleaseNotes.rst (in alphabetical order).
Do you know guys is it possible to get to noexcept source location, or we have to do by hand using lexer?
If it might be possible to get the location of noexcept(expression) using FunctionProtoType::getNoExceptExpr (though I haven't tried this), but there seems to be no information left in the AST about a simple noexcept. However, it should be easy to find noexcept using a lexer (unless, it's hidden in a macro).
Note `FunctionProtoType::getNoExceptExpr` is weird. If you have the same expr in multiple noexcepts, then it returns the same object for all of them, so it is useless for determining the location in code.
I ended up finding it all "by hand" with lexer (looking for matching parentheses).
We will see how much time it will take to make it pushable to master. IMHO this code is not complicated enough to make it wait for your patch, but it's cool that you took care of it :)
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
25 ↗ | (On Diff #59179) | you can use forFunction instead of hasAncestor(functionDecl( Also add test case void getFunction() noexcept { struct X { static void inner() { throw 42; } }; } |
130 ↗ | (On Diff #59179) | add new line after this line |
131 ↗ | (On Diff #59179) | add some new line |
134 ↗ | (On Diff #59179) | if one one the redecl won't be valid, then you can't change any of them. I guess you should accumulate them in |
135 ↗ | (On Diff #59179) | I think it would be better to tell about declaration only in one place (where is the definition) Also, I would use "note:" instead of warning here. (passing DiagnosticIDs::Level::Note as third argument) Also I think "no-throw function declared here" would be better |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
32 ↗ | (On Diff #59179) | function in llvm starts with lower case |
57 ↗ | (On Diff #59179) | same here |
71 ↗ | (On Diff #59179) | move it one line higher |
73 ↗ | (On Diff #59179) | Use full words. Consider changit to OpenParenCount |
91 ↗ | (On Diff #59179) | rename it to findNoexceptRange at mark it static. |
102 ↗ | (On Diff #59179) | remove extra braces |
108–110 ↗ | (On Diff #59179) | The comment is wrong. Shouldn't it be // We have noexcept that doesn't evaluate to true |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
25 ↗ | (On Diff #59179) | 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)? |
32 ↗ | (On Diff #59179) | 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. |
91 ↗ | (On Diff #59179) | This function will not work with dynamic exception specifications that also specify the function is nonthrowing, e.g., throw(). |
130 ↗ | (On Diff #59179) | How about wording the diagnostic: "'throw' expression in a function declared with a non-throwing exception specification" |
131 ↗ | (On Diff #59179) | const auto *, please (assuming Redecl is a pointer type). |
clang-tidy/misc/ThrowWithNoexceptCheck.h | ||
20 ↗ | (On Diff #59179) | What is the false positive rate with this check over a large codebase that uses exceptions? |
test/clang-tidy/misc-throw-with-noexcept.cpp | ||
70 ↗ | (On Diff #59179) | There are no tests for dynamic exception specifications. |
Thanks for review, guys. I fixed as many easy problems as I had time for today, will continue later.
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
25 ↗ | (On Diff #59179) | unless(hasAncestor(cxxTryStmt())) should do the work for almost all cases. cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow()).bind("func"))), + it should check that the throw is not in CXXCatchStmt (unless it is in another try block). |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
33 ↗ | (On Diff #59277) | I agree 100%. I see the code here as a temporary solution, before we have a proper AST functionality for doing that cleanly. |
I took advantage of new getExceptionSpecSourceRange (it wasn't available before) instead of getting exception specification manually with the lexer.
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
52–53 ↗ | (On Diff #84577) | just add DiagnosticIDs::Note as third argument |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
44 ↗ | (On Diff #84577) | Use line comments (//). |
53 ↗ | (On Diff #84577) | nit: no leading capitalization in diagnostic messages needed. |
test/clang-tidy/misc-throw-with-noexcept.cpp | ||
88 ↗ | (On Diff #84577) | 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. |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
54 ↗ | (On Diff #85882) | 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? |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
54 ↗ | (On Diff #85882) | I think that the best way would be to have warnings in order: warning function declared nothrow here have throw statement inside: Note: Fixit applied for every other declaration What do you think Alex? |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
54 ↗ | (On Diff #85882) | @Prazek 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. |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
54 ↗ | (On Diff #85882) | 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. |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
54 ↗ | (On Diff #85882) | 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). |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
54 ↗ | (On Diff #85882) | That's right, but does it make sense to show user all the places where the function was declared? |
clang-tidy/misc/ThrowWithNoexceptCheck.h | ||
20 ↗ | (On Diff #59179) | Do you know any good project to check it? |
clang-tidy/misc/ThrowWithNoexceptCheck.h | ||
---|---|---|
20 ↗ | (On Diff #59179) | 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. |
clang-tidy/misc/ThrowWithNoexceptCheck.h | ||
---|---|---|
20 ↗ | (On Diff #59179) | 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:
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? |
clang-tidy/misc/ThrowWithNoexceptCheck.h | ||
---|---|---|
20 ↗ | (On Diff #59179) | 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. |
clang-tidy/misc/ThrowWithNoexceptCheck.cpp | ||
---|---|---|
22 ↗ | (On Diff #97859) | If we handle throw() then it should be CPlusPlus |
28 ↗ | (On Diff #97859) | is this cland-formatted? |
51–53 ↗ | (On Diff #97859) | Here and in many other places - remove unnecessary braces (llvm coding style) |
117 ↗ | (On Diff #97859) | clang-format whole file |
docs/clang-tidy/checks/misc-throw-with-noexcept.rst | ||
9–10 ↗ | (On Diff #97859) | This is probably outdated. Also mention the other features that you developed here. |
docs/ReleaseNotes.rst | ||
---|---|---|
58 ↗ | (On Diff #99502) | 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.
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.
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.