This is an archive of the discontinued LLVM Phabricator instance.

[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.

Diff Detail

Event Timeline

sbarzowski updated this revision to Diff 54010.Apr 17 2016, 1:25 PM
sbarzowski retitled this revision from to [clang-tidy] misc-throw-with-noexcept.
sbarzowski updated this object.
sbarzowski added a reviewer: Prazek.
sbarzowski set the repository for this revision to rL LLVM.

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?

alexfh edited edge metadata.Apr 21 2016, 7:54 AM

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).

alexfh requested changes to this revision.Apr 22 2016, 8:53 AM
alexfh edited edge metadata.

Please fix formatting, btw.

This revision now requires changes to proceed.Apr 22 2016, 8:53 AM
sbarzowski updated this revision to Diff 59179.Jun 1 2016, 1:10 AM
sbarzowski updated this object.
sbarzowski edited edge metadata.
sbarzowski removed rL LLVM as the repository for this revision.
sbarzowski edited edge metadata.
sbarzowski set the repository for this revision to rL LLVM.

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).

aaron.ballman added a subscriber: aaron.ballman.

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).

I think you may want to take a dependency on: http://reviews.llvm.org/D20428

Prazek added a comment.Jun 1 2016, 4:29 AM

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).

I think you may want to take a dependency on: http://reviews.llvm.org/D20428

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 :)

Prazek added inline comments.Jun 1 2016, 5:38 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
26

you can use forFunction instead of hasAncestor(functionDecl(
cxxThrowExpr(stmt(forFunction(isNoThrow() or cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow())))
you can even try to remove stmt.

Also add test case

void getFunction() noexcept {
 struct X { 
  static void inner()
  {
      throw 42;
  }
 }; 
}
131

add new line after this line

132

add some new line

135

if one one the redecl won't be valid, then you can't change any of them. I guess you should accumulate them in
llvm::SmallVector<SourseRange, 5> whilie checking if they are valid. If all of them will be valid then perform fix on all of them.

136

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)
You can also see how to do it here:
http://reviews.llvm.org/D18821

Also I think "no-throw function declared here" would be better

Prazek added inline comments.Jun 1 2016, 5:59 AM
clang-tidy/misc/ThrowWithNoexceptCheck.cpp
33

function in llvm starts with lower case

58

same here

72

move it one line higher

74

Use full words. Consider changit to OpenParenCount

92

rename it to findNoexceptRange at mark it static.

103

remove extra braces

109–111

The comment is wrong. Shouldn't it be // We have noexcept that doesn't evaluate to true

aaron.ballman requested changes to this revision.Jun 1 2016, 6:41 AM
aaron.ballman edited edge metadata.
aaron.ballman added inline comments.Jun 1 2016, 6:41 AM
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
53

nit: remove the FIXME

54

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
54

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
54

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
54

@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
54

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
54

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
54

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
60

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
shafik added a subscriber: shafik.Sep 21 2023, 9:21 AM

Is this PR still relevant or can we close it?

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2023, 9:21 AM