This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] New checker for exceptions that are created but not thrown
ClosedPublic

Authored by Szelethus on Feb 9 2018, 4:03 AM.

Details

Summary

New checker called misc-throw-keyword-missing warns about cases where a temporary object's type is (likely) an exception but is not thrown. This is done by checking whether the type's name (or one of its baseclass' name) contains the substring "EXCEPTION", "Exception" or "exception".

void f(int i){
    if(i = 0)
        // Exception is created but not thrown.
        std::runtime_error("Wrong argument");
}

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Feb 9 2018, 4:03 AM
lebedev.ri added a subscriber: cfe-commits.
whisperity requested changes to this revision.Feb 9 2018, 5:27 AM
whisperity added inline comments.
clang-tidy/misc/ThrowKeywordMissingCheck.cpp
21 ↗(On Diff #133583)

Superfluous comment, this can be removed.

32 ↗(On Diff #133583)

matchesName says

Does not match typedefs of an underlying type with the given name.

Does your check find the following case?

typedef std::exception ERROR_BASE;
class my_error : public ERROR_BASE {
  /* Usual yadda. */
};

int main() {
  my_error();
}
46 ↗(On Diff #133583)

Either use passive for both subsentences or don't use passive at all. Maybe one of the following could be a better wording:

exception instantied and not thrown

exception is constructed but is not thrown

exception construction without a throw statement

test/clang-tidy/misc-throw-keyword-missing.cpp
23 ↗(On Diff #133583)

Please format the code. Why isn't there a space before {?

29 ↗(On Diff #133583)

Format here too.

This revision now requires changes to proceed.Feb 9 2018, 5:27 AM
Szelethus updated this revision to Diff 133597.Feb 9 2018, 6:05 AM

Changes made according to @whisperity's comments.

Szelethus marked 5 inline comments as done.Feb 9 2018, 6:06 AM
Szelethus added inline comments.
clang-tidy/misc/ThrowKeywordMissingCheck.cpp
32 ↗(On Diff #133583)

It does! I've added this case to the test file.

Szelethus updated this revision to Diff 133600.Feb 9 2018, 6:15 AM
Szelethus marked an inline comment as done.
whisperity resigned from this revision.Feb 9 2018, 6:21 AM

Works for me but I haven't any sayings in these. 😇

One concern I have is with RAII objects with "exception" in the name. You may already properly handle this, but I'd like to see a test case like:

struct ExceptionRAII {
  ExceptionRAII() {}
  ~ExceptionRAII() {}
};

void foo() {
  ExceptionRAII E; // Don't trigger on this
}
clang-tidy/misc/ThrowKeywordMissingCheck.cpp
45 ↗(On Diff #133600)

I'm not keen on the wording here ("bound" isn't a common phrase for exceptions). How about "suspicious exception object created but not thrown; did you mean 'throw %0'" and then pass in the text for the object construction?

docs/clang-tidy/checks/misc-throw-keyword-missing.rst
6 ↗(On Diff #133600)

about the potentially -> about a potentially

7 ↗(On Diff #133600)

or the same as -> or is the same as

14 ↗(On Diff #133600)

Can you format the code snippet?

Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/misc-throw-keyword-missing.rst
6 ↗(On Diff #133600)

Please remove This check and enclose throw in ``

Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.
Eugene.Zelenko added a subscriber: xazax.hun.
Eugene.Zelenko added inline comments.
clang-tidy/misc/ThrowKeywordMissingCheck.cpp
24 ↗(On Diff #133600)

Please don't use auto where type could not be easily deduced.

aaron.ballman added inline comments.Feb 9 2018, 10:21 AM
clang-tidy/misc/ThrowKeywordMissingCheck.cpp
24 ↗(On Diff #133600)

auto is appropriate here because these are horrible to try to spell out manually.

hintonda removed a subscriber: hintonda.Feb 9 2018, 2:49 PM
Szelethus updated this revision to Diff 133860.EditedFeb 12 2018, 7:43 AM

Fixed almost everything mentioned in comments. The warning message now also contains the object's type (more on that in an inline comment).

I also came up with this problem:

 RegularException funcReturningExceptioniTest(int i) {
   return RegularException();
 }
 
 void returnedValueTest() {
   funcReturningExceptioniTest(3); //Should this emit a warning?
}

I'm not sure whether it'd be a good idea to warn about these cases. Unused return values can be found by many other means, and I'm afraid the standard library is filled with these cases.

I've also added this code snippet to the test file.

Szelethus marked 7 inline comments as done.Feb 12 2018, 7:52 AM
Szelethus added inline comments.
clang-tidy/misc/ThrowKeywordMissingCheck.cpp
45 ↗(On Diff #133600)

I tried using the Lexer::getSourceText function, but it doesn't print the right parantheses (ex. RegularException( instead of RegularException()).

Also, are you sure it'd be a good idea to pass the entire statement there? In cases where it would be long, it could draw the programmer's attention away from the actual problem, the missing throw keyword.

I also came up with this problem:

 RegularException funcReturningExceptioniTest(int i) {
   return RegularException();
 }
 
 void returnedValueTest() {
   funcReturningExceptioniTest(3); //Should this emit a warning?
}

I'm not sure whether it'd be a good idea to warn about these cases. Unused return values can be found by many other means, and I'm afraid the standard library is filled with these cases.

I think it's fine to not warn on this. It can be caught by other means, but if those turn out to be insufficient for some reason, we can address them in a follow-up patch.

clang-tidy/misc/ThrowKeywordMissingCheck.cpp
45 ↗(On Diff #133600)

That's a reasonable point, I'm fine with the way things are in this patch now.

docs/ReleaseNotes.rst
70 ↗(On Diff #133860)

This wording is a bit unclear. How about:

Diagnoses when a temporary object that appears to be an exception is constructed but not thrown.

test/clang-tidy/misc-throw-keyword-missing.cpp
33 ↗(On Diff #133860)

Missing space after the comment introducer. This happens in other places as well -- you should run the patch through clang-format to be sure to catch all the formatting issues.

54 ↗(On Diff #133860)

Starting with this warning message and each subsequent message, you can drop most of the wording. e.g., :[[@LINE+1]]:5: warning: suspicious exception is sufficient.

132 ↗(On Diff #133860)

typo: Exceptioni

Szelethus updated this revision to Diff 134005.Feb 13 2018, 1:28 AM
Szelethus marked 7 inline comments as done.
aaron.ballman requested changes to this revision.Feb 13 2018, 9:58 AM

I apologize for not noticing this important detail earlier -- I think this check should live under bugprone rather than misc. Other than where it lives (and the related naming issues), I think this is looking good!

test/clang-tidy/misc-throw-keyword-missing.cpp
38 ↗(On Diff #134005)

Classname -> Class name

This revision now requires changes to proceed.Feb 13 2018, 9:58 AM
Szelethus updated this revision to Diff 134174.Feb 14 2018, 1:52 AM

Renamed the checker from misc-throw-keyword-missing to bugprone-throw-keyword-missing.

Just noticed that I can't mark your inline comment as done, since the file got renamed. The typo is also fixed (Classname -> Class name).

aaron.ballman accepted this revision.Feb 14 2018, 5:10 AM

LGTM, thank you!

This revision is now accepted and ready to land.Feb 14 2018, 5:10 AM
This revision was automatically updated to reflect the committed changes.