[PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Aaron Ballman]

On Sat, Jul 18, 2015 at 11:44 AM, Tobias Langner
<randomcppprogrammer at gmail.com> wrote:
> randomcppprogrammer created this revision.
> randomcppprogrammer added reviewers: klimek, cfe-commits, jbcoe.
>
> This patch contains an implementation to check whether exceptions where thrown by value and caught by const reference. This is a guideline mentioned in different places, e.g. "C++ Coding Standards" by H. Sutter.

I happened to be working on this exact same problem while developing
rules for CERT's C++ secure coding guidelines, using a very similar
approach. Thank you for looking into this! If you'd like another
coding standard to compare this to:
https://www.securecoding.cert.org/confluence/display/cplusplus/ERR61-CPP.+Catch+exceptions+by+lvalue+reference
(we have a recommendation for throwing rvalues as well).

A few things I noticed:

This is not testing whether you are throwing by value, but are instead
testing whether you are throwing a pointer. Is this intentional? Also,
not all thrown pointers are dangerous. For instance: throw "This is a
cheap exception"; is very reasonable code, while throw new
BadIdea(12); is not.

As for catching, catching by const lvalue reference is not always the
correct recommendation. For instance, the catch block could be
modifying the exception object and rethrowing it, at which point you
cannot catch by const reference. Also, not all types really need to be
caught by reference, such as builtin types (think int) or trivial
types.

Some comments on the code below.

>
> Patch by Tobias Langner
>
> http://reviews.llvm.org/D11328
>
> Files:
>   clang-tidy/misc/CMakeLists.txt
>   clang-tidy/misc/MiscTidyModule.cpp
>   clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
>   clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
>   test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp

> Index: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp
> ===================================================================
> --- /dev/null
> +++ test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp
> @@ -0,0 +1,71 @@
> +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-throw-by-value-catch-by-reference %t
> +// REQUIRES: shell
> +
> +class logic_error {
> +public:
> +  logic_error(const char *message) {}
> +};
> +
> +int selector;

Don't need this.

> +typedef logic_error *logic_ptr;
> +typedef logic_ptr logic_double_typedef;
> +
> +void testThrowFunc() {
> +  switch (selector) {

Or the switch.

> +  case 0:
> +    throw new logic_error("by pointer");
> +  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: avoid throwing pointer
> +  // [misc-throw-by-value-catch-by-reference]
> +  case 1: {
> +    logic_ptr tmp = new logic_error("by pointer");
> +    throw tmp;
> +    // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: avoid throwing pointer
> +    // [misc-throw-by-value-catch-by-reference]
> +  }
> +  case 2:
> +    throw logic_error("by value");
> +  }
> +}
> +
> +void catchByPointer() {
> +  try {
> +    testThrowFunc();
> +  } catch (logic_error *e) {
> +    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference
> +    // [misc-throw-by-value-catch-by-reference]
> +  }
> +}
> +
> +void catchByValue() {
> +  try {
> +    testThrowFunc();
> +  } catch (logic_error e) {
> +    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference
> +    // [misc-throw-by-value-catch-by-reference]
> +  }
> +}
> +
> +void catchByReference() {
> +  try {
> +    testThrowFunc();
> +  } catch (logic_error &e) {
> +    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference to
> +    // avoid unnecessary copies [misc-throw-by-value-catch-by-reference]
> +  }
> +}
> +
> +void catchByConstReference() {
> +  try {
> +    testThrowFunc();
> +  } catch (const logic_error &e) {
> +  }
> +}
> +
> +void catchTypedef() {
> +  try {
> +    testThrowFunc();
> +  } catch (logic_ptr e) {
> +    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference
> +    // [misc-throw-by-value-catch-by-reference]
> +  }
> +}
> Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
> ===================================================================
> --- /dev/null
> +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
> @@ -0,0 +1,32 @@
> +//===--- ThrowByValueCatchByReferenceCheck.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_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
> +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
> +
> +#include "../ClangTidy.h"
> +
> +namespace clang {
> +namespace tidy {
> +
> +///\brief checks for locations that do not throw by value
> +// or catch by reference.
> +class ThrowByValueCatchByReferenceCheck : public ClangTidyCheck {
> +public:

Since everything's public, perhaps just use a struct instead?

> +  ThrowByValueCatchByReferenceCheck(StringRef Name, ClangTidyContext *Context)
> +      : ClangTidyCheck(Name, Context) {}
> +  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
> +  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
> +};
> +
> +} // namespace tidy
> +} // namespace clang
> +
> +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
> +
> Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
> ===================================================================
> --- /dev/null
> +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
> @@ -0,0 +1,57 @@
> +//===--- ThrowByValueCatchByReferenceCheck.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 "ThrowByValueCatchByReferenceCheck.h"
> +#include "clang/AST/ASTContext.h"
> +#include "clang/ASTMatchers/ASTMatchFinder.h"
> +
> +using namespace clang::ast_matchers;
> +
> +namespace clang {
> +namespace tidy {
> +
> +void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) {
> +  Finder->addMatcher(throwExpr().bind("throw"), this);
> +  Finder->addMatcher(catchStmt().bind("catch"), this);
> +}
> +
> +void ThrowByValueCatchByReferenceCheck::check(
> +    const MatchFinder::MatchResult &Result) {
> +  const auto *throwExpr = Result.Nodes.getNodeAs<CXXThrowExpr>("throw");
> +  if (throwExpr != nullptr) {
> +    auto *subExpr = throwExpr->getSubExpr();
> +    auto qualType = subExpr->getType();
> +    auto *type = qualType.getTypePtr();
> +    if (type->isPointerType() == true) { // throwing a pointer

Should drop the ==; also, elide the braces to match the coding conventions.

> +      diag(subExpr->getLocStart(), "avoid throwing pointer");
> +    }
> +  }
> +  const auto *catchStmt = Result.Nodes.getNodeAs<CXXCatchStmt>("catch");
> +  if (catchStmt != nullptr) {
> +    auto caughtType = catchStmt->getCaughtType();
> +    auto *type = caughtType.getTypePtr();
> +    auto *varDecl = catchStmt->getExceptionDecl();

Not all catch clauses have a type, such as catch (...). Should guard
against type being null, and include a test case for this. That also
goes for varDecl, so you may want to make the relationship more clear
that if one is null, they both are null.

> +    if (type->isPointerType()) {
> +      diag(varDecl->getLocStart(),
> +           "catch by const reference");
> +    } else if (!type->isReferenceType()) {
> +      diag(varDecl->getLocStart(), "catch by const reference");
> +    } else {
> +      auto* reference = type->castAs<const ReferenceType>();
> +      assert(reference!=nullptr);
> +      if (reference->getPointeeType().isConstQualified() == false) {
> +        diag(varDecl->getLocStart(),
> +             "catch by const reference to avoid unnecessary copies");
> +      }
> +    }
> +  }
> +}
> +
> +} // namespace tidy
> +} // namespace clang
> Index: clang-tidy/misc/MiscTidyModule.cpp
> ===================================================================
> --- clang-tidy/misc/MiscTidyModule.cpp
> +++ clang-tidy/misc/MiscTidyModule.cpp
> @@ -21,6 +21,7 @@
>  #include "NoexceptMoveConstructorCheck.h"
>  #include "StaticAssertCheck.h"
>  #include "SwappedArgumentsCheck.h"
> +#include "ThrowByValueCatchByReferenceCheck.h"
>  #include "UndelegatedConstructor.h"
>  #include "UniqueptrResetReleaseCheck.h"
>  #include "UnusedRAIICheck.h"
> @@ -54,6 +55,8 @@
>          "misc-static-assert");
>      CheckFactories.registerCheck<SwappedArgumentsCheck>(
>          "misc-swapped-arguments");
> +    CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
> +        "misc-throw-by-value-catch-by-reference");
>      CheckFactories.registerCheck<UndelegatedConstructorCheck>(
>          "misc-undelegated-constructor");
>      CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
> Index: clang-tidy/misc/CMakeLists.txt
> ===================================================================
> --- clang-tidy/misc/CMakeLists.txt
> +++ clang-tidy/misc/CMakeLists.txt
> @@ -13,6 +13,7 @@
>    NoexceptMoveConstructorCheck.cpp
>    StaticAssertCheck.cpp
>    SwappedArgumentsCheck.cpp
> +  ThrowByValueCatchByReferenceCheck.cpp
>    UndelegatedConstructor.cpp
>    UnusedRAIICheck.cpp
>    UniqueptrResetReleaseCheck.cpp
>

~Aaron