[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

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.

[Szelethus]

Changes made according to @whisperity's comments.

[Szelethus]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

32 ↗

(On Diff #133583)

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

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]

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

6 ↗

(On Diff #133600)

Please remove This check and enclose throw in ``

[Eugene.Zelenko]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

24 ↗

(On Diff #133600)

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

[aaron.ballman]

clang-tidy/misc/ThrowKeywordMissingCheck.cpp

24 ↗

(On Diff #133600)

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

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.

[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

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

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

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

(On Diff #134005)

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/bugprone/BugproneTidyModule.cpp

 #include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "StringConstructorCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
+#include "ThrowKeywordMissingCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
+    CheckFactories.registerCheck<ThrowKeywordMissingCheck>(
+        "bugprone-throw-keyword-missing");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");
   }
 };

clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IntegerDivisionCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   StringConstructorCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
+  ThrowKeywordMissingCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/bugprone/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_BUGPRONE_THROWKEYWORDMISSINGCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_THROWKEYWORDMISSINGCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// 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/bugprone-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 bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_THROWKEYWORDMISSINGCHECK_H

clang-tidy/bugprone/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 bugprone {
+
+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 bugprone
+} // 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 `bugprone-throw-keyword-missing
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-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/bugprone-throw-keyword-missing.rst

+.. title:: clang-tidy - bugprone-throw-keyword-missing
+
+bugprone-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");
+    }
+  }
+
+

docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-forward-declaration-namespace
    bugprone-inaccurate-erase
    bugprone-integer-division
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-move-forwarding-reference
    bugprone-multiple-statement-macro
    bugprone-string-constructor
    bugprone-suspicious-memset-usage
+   bugprone-throw-keyword-missing
    bugprone-undefined-memory-manipulation
    bugprone-use-after-move
    bugprone-virtual-near-miss
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp

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

+// RUN: %check_clang_tidy %s bugprone-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 {};
+
+// Class name 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 {{.*}}'? [bugprone-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;
+}