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

[Tobias Langner]

Hi,

I modified the code. During this, additional questions popped up that I’d like to resolve before I put up a new patch. Please check my comments below.

> On 20 Jul 2015, at 17:17, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> 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?

Yes. It’s easier to test and since you can only throw by pointer or by value, it’s in my opinion equivalent.

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

this can be handled by allowing StringLiterals to be thrown, but no other pointers. My problem with this is that I don’t know how to recognise
this at the catch statement without removing the check for pointers. The only idea that I have right now is to check whether the pointed to type is ‘const char’ or ‘const wchar_t’ or one of the new ones for unicode. But it feels a little bit brittle.

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

I added the check for builtin & trivial types - that was a good idea. Regarding catching as reference (opposed to const reference), I could think of 2 different options:
- require nolint - which I think is viable. It marks in source code that someone had thought about this potential problem.
- having an option for the test (something like enforce const reference).
Both have their benefits & drawbacks. If a project requires at most call sites that the exception is modified, the second one is better - but I don’t know how to test different option given the current test infrastructure. If modifying an exception is rare, I think the first option is better as it forces to comment this rare situation.

I would not like to remove the test for const-ness on catch because it can be a real performance hog that some might not be aware about.
Any advice which way to go? 
> 
> 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.
Done
> 
>> +typedef logic_error *logic_ptr;
>> +typedef logic_ptr logic_double_typedef;
>> +
>> +void testThrowFunc() {
>> +  switch (selector) {
> 
> Or the switch.
Done
> 
>> +  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?

for me structs are close to pods (and some static analysis tools like QAC++ actually have rules that enforce that). This is clearly not a pod so I think class is appropriate (unless that’s against the coding guidelines of the clang project).

> 
>> +  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.
done
> 
>> +      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.
done
> 
>> +    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