From 9994581395555e994d2158f688bae60c969b1e3e Mon Sep 17 00:00:00 2001 From: Yan Zhang Date: Thu, 16 Nov 2017 01:28:29 +0000 Subject: [PATCH] add check to avoid throwing objc exception according to Google Objective-C guide Summary: This is a small check to avoid throwing objc exceptions. In specific it will detect the usage of @throw statement and throw warning. Reviewers: hokein, benhamilton Reviewed By: hokein, benhamilton Subscribers: cfe-commits, mgorny Differential Revision: https://reviews.llvm.org/D40058 llvm-svn: 318366 --- .../AvoidThrowingObjCExceptionCheck.cpp | 47 +++++++++++++++++++ .../google/AvoidThrowingObjCExceptionCheck.h | 39 +++++++++++++++ .../clang-tidy/google/CMakeLists.txt | 1 + .../clang-tidy/google/GoogleTidyModule.cpp | 3 ++ clang-tools-extra/docs/ReleaseNotes.rst | 5 ++ .../google-objc-avoid-throwing-exception.rst | 38 +++++++++++++++ .../docs/clang-tidy/checks/list.rst | 1 + .../google-objc-avoid-throwing-exception.m | 32 +++++++++++++ 8 files changed, 166 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst create mode 100644 clang-tools-extra/test/clang-tidy/google-objc-avoid-throwing-exception.m diff --git a/clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.cpp b/clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.cpp new file mode 100644 index 0000000000000..791651fb6d6e0 --- /dev/null +++ b/clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.cpp @@ -0,0 +1,47 @@ +//===--- AvoidThrowingObjCExceptionCheck.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 "AvoidThrowingObjCExceptionCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace google { +namespace objc { + +void AvoidThrowingObjCExceptionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(objcThrowStmt().bind("throwStmt"), this); + Finder->addMatcher( + objcMessageExpr(anyOf(hasSelector("raise:format:"), + hasSelector("raise:format:arguments:")), + hasReceiverType(asString("NSException"))) + .bind("raiseException"), + this); +} + +void AvoidThrowingObjCExceptionCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedStmt = + Result.Nodes.getNodeAs("throwStmt"); + const auto *MatchedExpr = + Result.Nodes.getNodeAs("raiseException"); + auto SourceLoc = MatchedStmt == nullptr ? MatchedExpr->getSelectorStartLoc() + : MatchedStmt->getThrowLoc(); + diag(SourceLoc, + "pass in NSError ** instead of throwing exception to indicate " + "Objective-C errors"); +} + +} // namespace objc +} // namespace google +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.h b/clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.h new file mode 100644 index 0000000000000..9498226d7e6e5 --- /dev/null +++ b/clang-tools-extra/clang-tidy/google/AvoidThrowingObjCExceptionCheck.h @@ -0,0 +1,39 @@ +//===--- AvoidThrowingObjCExceptionCheck.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_GOOGLE_OBJC_AVOID_THROWING_EXCEPTION_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_OBJC_AVOID_THROWING_EXCEPTION_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace google { +namespace objc { + +/// The check is to find usage of @throw invocation in Objective-C code. +/// We should avoid using @throw for Objective-C exceptions according to +/// the Google Objective-C Style Guide. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/google-objc-avoid-throwing-exception.html +class AvoidThrowingObjCExceptionCheck : public ClangTidyCheck { + public: + AvoidThrowingObjCExceptionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace objc +} // namespace google +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_OBJC_AVOID_THROWING_EXCEPTION_H diff --git a/clang-tools-extra/clang-tidy/google/CMakeLists.txt b/clang-tools-extra/clang-tidy/google/CMakeLists.txt index 9a91915424661..5f1eb2334e8c6 100644 --- a/clang-tools-extra/clang-tidy/google/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/google/CMakeLists.txt @@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyGoogleModule AvoidCStyleCastsCheck.cpp + AvoidThrowingObjCExceptionCheck.cpp DefaultArgumentsCheck.cpp ExplicitConstructorCheck.cpp ExplicitMakePairCheck.cpp diff --git a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp b/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp index 45d7de584354b..aa1802de0eeec 100644 --- a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp @@ -15,6 +15,7 @@ #include "../readability/NamespaceCommentCheck.h" #include "../readability/RedundantSmartptrGetCheck.h" #include "AvoidCStyleCastsCheck.h" +#include "AvoidThrowingObjCExceptionCheck.h" #include "DefaultArgumentsCheck.h" #include "ExplicitConstructorCheck.h" #include "ExplicitMakePairCheck.h" @@ -49,6 +50,8 @@ class GoogleModule : public ClangTidyModule { "google-explicit-constructor"); CheckFactories.registerCheck( "google-global-names-in-headers"); + CheckFactories.registerCheck( + "google-objc-avoid-throwing-exception"); CheckFactories.registerCheck( "google-objc-global-variable-declaration"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 1d70bffdab319..a3a95e53f2564 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -57,6 +57,11 @@ The improvements are... Improvements to clang-tidy -------------------------- +- New `google-avoid-throwing-objc-exception + `_ check + + Add new check to detect throwing exceptions in Objective-C code, which should be avoided. + - New `objc-property-declaration `_ check diff --git a/clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst b/clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst new file mode 100644 index 0000000000000..5710154693462 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-throwing-exception.rst @@ -0,0 +1,38 @@ +.. title:: clang-tidy - google-objc-avoid-throwing-exception + +google-objc-avoid-throwing-exception +==================================== + +This check finds finds uses of throwing exceptions usages in Objective-C files. +For the same reason as the Google C++ style guide, we prefer not throwing +exceptions from Objective-C code. + +The corresponding C++ style guide rule: +https://google.github.io/styleguide/cppguide.html#Exceptions + +Instead, prefer passing in ``NSError **`` and return ``BOOL`` to indicate success or failure. + +A counterexample: + +.. code-block:: objc + + - (void)readFile { + if ([self isError]) { + @throw [NSException exceptionWithName:...]; + } + } + +Instead, returning an error via ``NSError **`` is preferred: + +.. code-block:: objc + + - (BOOL)readFileWithError:(NSError **)error { + if ([self isError]) { + *error = [NSError errorWithDomain:...]; + return NO; + } + return YES; + } + +The corresponding style guide rule: +http://google.github.io/styleguide/objcguide.html#avoid-throwing-exceptions diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 1ad7166dab803..7c1bf074026d1 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -60,6 +60,7 @@ Clang-Tidy Checks google-default-arguments google-explicit-constructor google-global-names-in-headers + google-objc-avoid-throwing-exception google-objc-global-variable-declaration google-readability-braces-around-statements (redirects to readability-braces-around-statements) google-readability-casting diff --git a/clang-tools-extra/test/clang-tidy/google-objc-avoid-throwing-exception.m b/clang-tools-extra/test/clang-tidy/google-objc-avoid-throwing-exception.m new file mode 100644 index 0000000000000..7fa32e7a5aa59 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/google-objc-avoid-throwing-exception.m @@ -0,0 +1,32 @@ +// RUN: %check_clang_tidy %s google-objc-avoid-throwing-exception %t +@class NSString; + +@interface NSException + ++ (void)raise:(NSString *)name format:(NSString *)format; ++ (void)raise:(NSString *)name format:(NSString *)format arguments:(NSString *)args; // using NSString type since va_list cannot be recognized here + +@end + +@interface NotException + ++ (void)raise:(NSString *)name format:(NSString *)format; + +@end + +@implementation Foo +- (void)f { + NSString *foo = @"foo"; + @throw foo; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass in NSError ** instead of throwing exception to indicate Objective-C errors [google-objc-avoid-throwing-exception] +} + +- (void)f2 { + [NSException raise:@"TestException" format:@"Test"]; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: pass in NSError ** instead of throwing exception to indicate Objective-C errors [google-objc-avoid-throwing-exception] + [NSException raise:@"TestException" format:@"Test %@" arguments:@"bar"]; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: pass in NSError ** instead of throwing exception to indicate Objective-C errors [google-objc-avoid-throwing-exception] + [NotException raise:@"NotException" format:@"Test"]; +} +@end +