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

[Szelethus]

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");
}

[whisperity]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

22

Superfluous comment, this can be removed.

33

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();
}

47

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

24

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

30

Format here too.

[Szelethus]

Changes made according to @whisperity's comments.

[Szelethus]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

33

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

[whisperity]

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

[aaron.ballman]

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

46

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

7

about the potentially -> about a potentially

8

or the same as -> or is the same as

15

Can you format the code snippet?

[Eugene.Zelenko]

docs/clang-tidy/checks/misc-throw-keyword-missing.rst

7

Please remove This check and enclose throw in ``

[Eugene.Zelenko]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

25

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

[aaron.ballman]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

25

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

[Szelethus]

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]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

46

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.

[aaron.ballman]

In D43120#1005100, @Szelethus wrote:

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

46

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

docs/ReleaseNotes.rst

70

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

34

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.

55

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.

133

typo: Exceptioni

[aaron.ballman]

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

Classname -> Class name

[Szelethus]

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

[Szelethus]

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]

LGTM, thank you!

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ForwardingReferenceOverloadCheck.cpp
   LambdaFunctionNameCheck.cpp
   MisplacedConstCheck.cpp
+  ThrowKeywordMissingCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   DefinitionsInHeadersCheck.cpp
   IncorrectRoundings.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp
   NewDeleteOverloadsCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
+#include "ThrowKeywordMissingCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "misc-forwarding-reference-overload");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "misc-lambda-function-name");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
+    CheckFactories.registerCheck<ThrowKeywordMissingCheck>(
+        "misc-throw-keyword-missing");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<IncorrectRoundings>(
         "misc-incorrect-roundings");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");

clang-tidy/misc/ThrowKeywordMissingCheck.h

+//===--- ThrowKeywordMissingCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROWKEYWORDMISSINGCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROWKEYWORDMISSINGCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Emits a warning about temporary objects whose type is (or is derived from) a
+/// class that has 'EXCEPTION', 'Exception' or 'exception' in its name.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-throw-keyword-missing.html
+class ThrowKeywordMissingCheck : public ClangTidyCheck {
+public:
+  ThrowKeywordMissingCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROWKEYWORDMISSINGCHECK_H

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

+//===--- ThrowKeywordMissingCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ThrowKeywordMissingCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void ThrowKeywordMissingCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  auto CtorInitializerList =
+      cxxConstructorDecl(hasAnyConstructorInitializer(anything()));
+
+  Finder->addMatcher(
+      expr(anyOf(cxxFunctionalCastExpr(), cxxBindTemporaryExpr(),
+                 cxxTemporaryObjectExpr()),
+           hasType(cxxRecordDecl(
+               isSameOrDerivedFrom(matchesName("[Ee]xception|EXCEPTION")))),
+           unless(anyOf(hasAncestor(stmt(
+                            anyOf(cxxThrowExpr(), callExpr(), returnStmt()))),
+                        hasAncestor(varDecl()),
+                        allOf(hasAncestor(CtorInitializerList),
+                              unless(hasAncestor(cxxCatchStmt()))))))
+          .bind("temporary-exception-not-thrown"),
+      this);
+}
+
+void ThrowKeywordMissingCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *TemporaryExpr =
+      Result.Nodes.getNodeAs<Expr>("temporary-exception-not-thrown");
+
+  diag(TemporaryExpr->getLocStart(), "suspicious exception object created but "
+                                     "not thrown; did you mean 'throw %0'?")
+      << TemporaryExpr->getType().getBaseTypeIdentifier()->getName();
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 
 - New `fuchsia-statically-constructed-objects
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check
 
   Warns if global, non-trivial objects with static storage are constructed, unless the 
   object is statically initialized with a ``constexpr`` constructor or has no 
   explicit constructor.
 
+- New `misc-throw-keyword-missing
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-throw-keyword-missing.html>`_ check
+
+  Diagnoses when a temporary object that appears to be an exception is constructed but not thrown.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-enum-usage
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-suspicious-string-compare
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
+   misc-throw-keyword-missing
    misc-unconventional-assign-operator
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    misc-unused-using-decls
    modernize-avoid-bind

docs/clang-tidy/checks/misc-throw-keyword-missing.rst

+.. title:: clang-tidy - misc-throw-keyword-missing
+
+misc-throw-keyword-missing
+==========================
+
+Warns about a potentially missing ``throw`` keyword. If a temporary object is created, but the
+object's type derives from (or is the same as) a class that has 'EXCEPTION', 'Exception' or
+'exception' in its name, we can assume that the programmer's intention was to throw that object.
+
+Example:
+
+.. code-block:: c++
+  void f(int i) {
+    if (i < 0) {
+      // Exception is created but is not thrown.
+      std::runtime_error("Unexpected argument");
+    }
+  }
+
+

test/clang-tidy/misc-throw-keyword-missing.cpp

+// RUN: %check_clang_tidy %s misc-throw-keyword-missing %t
+
+namespace std {
+
+// std::string declaration (taken from test/clang-tidy/readability-redundant-string-cstr-msvc.cpp).
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>
+struct basic_string {
+  basic_string();
+  basic_string(const basic_string &);
+  // MSVC headers define two constructors instead of using optional arguments.
+  basic_string(const C *);
+  basic_string(const C *, const A &);
+  ~basic_string();
+};
+typedef basic_string<char> string;
+typedef basic_string<wchar_t> wstring;
+
+// std::exception and std::runtime_error declaration.
+struct exception {
+  exception();
+  exception(const exception &other);
+  virtual ~exception();
+};
+
+struct runtime_error : public exception {
+  explicit runtime_error(const std::string &what_arg);
+};
+
+} // namespace std
+
+// The usage of this class should never emit a warning.
+struct RegularClass {};
+
+// Classname contains the substring "exception", in certain cases using this class should emit a warning.
+struct RegularException {
+  RegularException() {}
+
+  // Constructors with a single argument are treated differently (cxxFunctionalCastExpr).
+  RegularException(int) {}
+};
+
+// --------------
+
+void stdExceptionNotTrownTest(int i) {
+  if (i < 0)
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious exception object created but not thrown; did you mean 'throw {{.*}}'? [misc-throw-keyword-missing]
+    std::exception();
+
+  if (i > 0)
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious exception
+    std::runtime_error("Unexpected argument");
+}
+
+void stdExceptionThrownTest(int i) {
+  if (i < 0)
+    throw std::exception();
+
+  if (i > 0)
+    throw std::runtime_error("Unexpected argument");
+}
+
+void regularClassNotThrownTest(int i) {
+  if (i < 0)
+    RegularClass();
+}
+
+void regularClassThrownTest(int i) {
+  if (i < 0)
+    throw RegularClass();
+}
+
+void nameContainsExceptionNotThrownTest(int i) {
+  if (i < 0)
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious exception
+    RegularException();
+
+  if (i > 0)
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious exception
+    RegularException(5);
+}
+
+void nameContainsExceptionThrownTest(int i) {
+  if (i < 0)
+    throw RegularException();
+
+  if (i > 0)
+    throw RegularException(5);
+}
+
+template <class Exception>
+void f(int i, Exception excToBeThrown) {}
+
+void funcCallWithTempExcTest() {
+  f(5, RegularException());
+}
+
+// Global variable initilization test.
+RegularException exc = RegularException();
+RegularException *excptr = new RegularException();
+
+void localVariableInitTest() {
+  RegularException exc = RegularException();
+  RegularException *excptr = new RegularException();
+}
+
+class CtorInitializerListTest {
+  RegularException exc;
+
+  CtorInitializerListTest() : exc(RegularException()) {}
+
+  CtorInitializerListTest(int) try : exc(RegularException()) {
+    // Constructor body
+  } catch (...) {
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: suspicious exception
+    RegularException();
+  }
+
+  CtorInitializerListTest(float);
+};
+
+CtorInitializerListTest::CtorInitializerListTest(float) try : exc(RegularException()) {
+  // Constructor body
+} catch (...) {
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: suspicious exception
+  RegularException();
+}
+
+RegularException funcReturningExceptionTest(int i) {
+  return RegularException();
+}
+
+void returnedValueTest() {
+  funcReturningExceptionTest(3);
+}
+
+struct ClassBracedInitListTest {
+  ClassBracedInitListTest(RegularException exc) {}
+};
+
+void foo(RegularException, ClassBracedInitListTest) {}
+
+void bracedInitListTest() {
+  RegularException exc{};
+  ClassBracedInitListTest test = {RegularException()};
+  foo({}, {RegularException()});
+}
+
+typedef std::exception ERROR_BASE;
+class RegularError : public ERROR_BASE {};
+
+void typedefTest() {
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: suspicious exception
+  RegularError();
+}
+
+struct ExceptionRAII {
+  ExceptionRAII() {}
+  ~ExceptionRAII() {}
+};
+
+void exceptionRAIITest() {
+  ExceptionRAII E;
+}