From e010406e28074f954336106ac624181b24923a41 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Fri, 27 Oct 2017 07:41:36 +0000 Subject: [PATCH] [clang-tidy ObjC] [3/3] New check objc-forbidden-subclassing Summary: This is part 3 of 3 of a series of changes to improve Objective-C linting in clang-tidy. This adds a new clang-tidy check `objc-forbidden-subclassing` which ensures clients do not create subclasses of Objective-C classes which are not designed to be subclassed. (Note that for code under your control, you should use __attribute__((objc_subclassing_restricted)) instead -- this is intended for third-party APIs which cannot be modified.) By default, the following classes (which are publicly documented as not supporting subclassing) are forbidden from subclassing: ABNewPersonViewController ABPeoplePickerNavigationController ABPersonViewController ABUnknownPersonViewController NSHashTable NSMapTable NSPointerArray NSPointerFunctions NSTimer UIActionSheet UIAlertView UIImagePickerController UITextInputMode UIWebView Clients can set a CheckOption `objc-forbidden-subclassing.ClassNames` to a semicolon-separated list of class names, which overrides this list. Test Plan: `ninja check-clang-tools` Patch by Ben Hamilton! Reviewers: hokein, alexfh Reviewed By: hokein Subscribers: saidinwot, Wizard, srhines, mgorny, xazax.hun Differential Revision: https://reviews.llvm.org/D39142 llvm-svn: 316744 --- clang-tools-extra/clang-tidy/objc/CMakeLists.txt | 1 + .../clang-tidy/objc/ForbiddenSubclassingCheck.cpp | 118 +++++++++++++++++++++ .../clang-tidy/objc/ForbiddenSubclassingCheck.h | 42 ++++++++ .../clang-tidy/objc/ObjCTidyModule.cpp | 4 +- clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ clang-tools-extra/docs/clang-tidy/checks/list.rst | 9 +- .../checks/objc-forbidden-subclassing.rst | 27 +++++ .../clang-tidy/objc-forbidden-subclassing-custom.m | 39 +++++++ .../test/clang-tidy/objc-forbidden-subclassing.m | 21 ++++ .../unittests/clang-tidy/ObjCModuleTest.cpp | 30 +++++- 10 files changed, 291 insertions(+), 6 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/objc-forbidden-subclassing.rst create mode 100644 clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing-custom.m create mode 100644 clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing.m diff --git a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt index 85682b5..d445f6e 100644 --- a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt @@ -1,6 +1,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyObjCModule + ForbiddenSubclassingCheck.cpp ObjCTidyModule.cpp LINK_LIBS diff --git a/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp b/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp new file mode 100644 index 0000000..a8d79f5 --- /dev/null +++ b/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp @@ -0,0 +1,118 @@ +//===--- ForbiddenSubclassingCheck.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 "ForbiddenSubclassingCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/Hashing.h" +#include "llvm/ADT/SmallVector.h" +#include "../utils/OptionsUtils.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace objc { + +namespace { + +constexpr char DefaultForbiddenSuperClassNames[] = + "ABNewPersonViewController;" + "ABPeoplePickerNavigationController;" + "ABPersonViewController;" + "ABUnknownPersonViewController;" + "NSHashTable;" + "NSMapTable;" + "NSPointerArray;" + "NSPointerFunctions;" + "NSTimer;" + "UIActionSheet;" + "UIAlertView;" + "UIImagePickerController;" + "UITextInputMode;" + "UIWebView"; + +/// \brief Matches Objective-C classes that directly or indirectly +/// have a superclass matching \c Base. +/// +/// Note that a class is not considered to be a subclass of itself. +/// +/// Example matches Y, Z +/// (matcher = objcInterfaceDecl(hasName("X"))) +/// \code +/// @interface X +/// @end +/// @interface Y : X // directly derived +/// @end +/// @interface Z : Y // indirectly derived +/// @end +/// \endcode +AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOf, + ast_matchers::internal::Matcher, Base) { + for (const auto *SuperClass = Node.getSuperClass(); + SuperClass != nullptr; + SuperClass = SuperClass->getSuperClass()) { + if (Base.matches(*SuperClass, Finder, Builder)) { + return true; + } + } + return false; +} + +} // namespace + +ForbiddenSubclassingCheck::ForbiddenSubclassingCheck( + StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + ForbiddenSuperClassNames( + utils::options::parseStringList( + Options.get("ClassNames", DefaultForbiddenSuperClassNames))) { +} + +void ForbiddenSubclassingCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + objcInterfaceDecl( + isSubclassOf( + objcInterfaceDecl( + hasAnyName( + std::vector( + ForbiddenSuperClassNames.begin(), + ForbiddenSuperClassNames.end()))) + .bind("superclass"))) + .bind("subclass"), + this); +} + +void ForbiddenSubclassingCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *SubClass = Result.Nodes.getNodeAs( + "subclass"); + assert(SubClass != nullptr); + const auto *SuperClass = Result.Nodes.getNodeAs( + "superclass"); + assert(SuperClass != nullptr); + diag(SubClass->getLocation(), + "Objective-C interface %0 subclasses %1, which is not " + "intended to be subclassed") + << SubClass + << SuperClass; +} + +void ForbiddenSubclassingCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store( + Opts, + "ForbiddenSuperClassNames", + utils::options::serializeStringList(ForbiddenSuperClassNames)); +} + +} // namespace objc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.h b/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.h new file mode 100644 index 0000000..6c7e08b --- /dev/null +++ b/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.h @@ -0,0 +1,42 @@ +//===--- ForbiddenSubclassingCheck.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_OBJC_FORBIDDEN_SUBCLASSING_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_FORBIDDEN_SUBCLASSING_CHECK_H + +#include "../ClangTidy.h" +#include "llvm/ADT/StringRef.h" +#include +#include + +namespace clang { +namespace tidy { +namespace objc { + +/// Finds Objective-C classes which have a superclass which is +/// documented to not support subclassing. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/objc-forbidden-subclassing.html +class ForbiddenSubclassingCheck : public ClangTidyCheck { +public: + ForbiddenSubclassingCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Options) override; + +private: + const std::vector ForbiddenSuperClassNames; +}; + +} // namespace objc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_FORBIDDEN_SUBCLASSING_CHECK_H diff --git a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp b/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp index 46ff21f..51540739 100644 --- a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ForbiddenSubclassingCheck.h" using namespace clang::ast_matchers; @@ -20,7 +21,8 @@ namespace objc { class ObjCModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { - // TODO(D39142): Add checks here. + CheckFactories.registerCheck( + "objc-forbidden-subclassing"); } }; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 4dc3483..b36217a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -59,6 +59,12 @@ Improvements to clang-tidy - New module `objc` for Objective-C style checks. +- New `objc-forbidden-subclassing + `_ check + + Ensures Objective-C classes do not subclass any classes which are + not intended to be subclassed. + - Renamed checks to use correct term "implicit conversion" instead of "implicit cast" and modified messages and option names accordingly: diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index ca3d081..38ff9cb 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -7,9 +7,9 @@ Clang-Tidy Checks android-cloexec-accept android-cloexec-accept4 android-cloexec-creat + android-cloexec-dup android-cloexec-epoll-create android-cloexec-epoll-create1 - android-cloexec-dup android-cloexec-fopen android-cloexec-inotify-init android-cloexec-inotify-init1 @@ -38,7 +38,7 @@ Clang-Tidy Checks cert-msc30-c (redirects to cert-msc50-cpp) cert-msc50-cpp cert-oop11-cpp (redirects to misc-move-constructor-init) - cppcoreguidelines-c-copy-assignment-signature + cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) cppcoreguidelines-interfaces-global-init cppcoreguidelines-no-malloc cppcoreguidelines-owning-memory @@ -76,8 +76,8 @@ Clang-Tidy Checks hicpp-explicit-conversions (redirects to google-explicit-constructor) hicpp-function-size (redirects to readability-function-size) hicpp-invalid-access-moved (redirects to misc-use-after-move) - hicpp-move-const-arg (redirects to misc-move-const-arg) hicpp-member-init (redirects to cppcoreguidelines-pro-type-member-init) + hicpp-move-const-arg (redirects to misc-move-const-arg) hicpp-named-parameter (redirects to readability-named-parameter) hicpp-new-delete-operators (redirects to misc-new-delete-overloads) hicpp-no-array-decay (redirects to cppcoreguidelines-pro-bounds-array-to-pointer-decay) @@ -95,7 +95,7 @@ Clang-Tidy Checks hicpp-use-noexcept (redirects to modernize-use-noexcept) hicpp-use-nullptr (redirects to modernize-use-nullptr) hicpp-use-override (redirects to modernize-use-override) - hicpp-vararg (redirects to cppcoreguidelines-pro-type-varg) + hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) llvm-header-guard llvm-include-order llvm-namespace-comment @@ -180,6 +180,7 @@ Clang-Tidy Checks performance-type-promotion-in-math-fn performance-unnecessary-copy-initialization performance-unnecessary-value-param + objc-forbidden-subclassing readability-avoid-const-params-in-decls readability-braces-around-statements readability-container-size-empty diff --git a/clang-tools-extra/docs/clang-tidy/checks/objc-forbidden-subclassing.rst b/clang-tools-extra/docs/clang-tidy/checks/objc-forbidden-subclassing.rst new file mode 100644 index 0000000..629f8b6 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/objc-forbidden-subclassing.rst @@ -0,0 +1,27 @@ +.. title:: clang-tidy - objc-forbidden-subclassing + +objc-forbidden-subclassing +================================== + +Finds Objective-C classes which are subclasses of classes which are +not designed to be subclassed. + +By default, includes a list of Objective-C classes which +are publicly documented as not supporting subclassing. + +.. note:: + + Instead of using this check, for code under your control, you should add + ``__attribute__((objc_subclassing_restricted))`` before your ``@interface`` declarations + to ensure the compiler prevents others from subclassing your Objective-C classes. + See https://clang.llvm.org/docs/AttributeReference.html#objc-subclassing-restricted + +Options +------- + +.. option:: ForbiddenSuperClassNames + + Semicolon-separated list of names of Objective-C classes which + do not support subclassing. + + Defaults to ``ABNewPersonViewController;ABPeoplePickerNavigationController;ABPersonViewController;ABUnknownPersonViewController;NSHashTable;NSMapTable;NSPointerArray;NSPointerFunctions;NSTimer;UIActionSheet;UIAlertView;UIImagePickerController;UITextInputMode;UIWebView``. diff --git a/clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing-custom.m b/clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing-custom.m new file mode 100644 index 0000000..59e6a88 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing-custom.m @@ -0,0 +1,39 @@ +// RUN: %check_clang_tidy %s objc-forbidden-subclassing %t \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: objc-forbidden-subclassing.ClassNames, value: "Foo;Quux"}]}' \ +// RUN: -- + +@interface UIImagePickerController +@end + +// Make sure custom config options replace (not add to) the default list. +@interface Waldo : UIImagePickerController +// CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: Objective-C interface 'Waldo' subclasses 'UIImagePickerController', which is not intended to be subclassed [objc-forbidden-subclassing] +@end + +@interface Foo +@end + +@interface Bar : Foo +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Objective-C interface 'Bar' subclasses 'Foo', which is not intended to be subclassed [objc-forbidden-subclassing] +@end + +// Check subclasses of subclasses. +@interface Baz : Bar +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Objective-C interface 'Baz' subclasses 'Foo', which is not intended to be subclassed [objc-forbidden-subclassing] +@end + +@interface Quux +@end + +// Check that more than one forbidden superclass can be specified. +@interface Xyzzy : Quux +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Objective-C interface 'Xyzzy' subclasses 'Quux', which is not intended to be subclassed [objc-forbidden-subclassing] +@end + +@interface Plugh +@end + +@interface Corge : Plugh +// CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: Objective-C interface 'Corge' subclasses 'Plugh', which is not intended to be subclassed [objc-forbidden-subclassing] +@end diff --git a/clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing.m b/clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing.m new file mode 100644 index 0000000..3ad38bf --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/objc-forbidden-subclassing.m @@ -0,0 +1,21 @@ +// RUN: %check_clang_tidy %s objc-forbidden-subclassing %t + +@interface UIImagePickerController +@end + +@interface Foo : UIImagePickerController +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Objective-C interface 'Foo' subclasses 'UIImagePickerController', which is not intended to be subclassed [objc-forbidden-subclassing] +@end + +// Check subclasses of subclasses. +@interface Bar : Foo +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Objective-C interface 'Bar' subclasses 'UIImagePickerController', which is not intended to be subclassed [objc-forbidden-subclassing] +@end + +@interface Baz +@end + +// Make sure innocent subclasses aren't caught by the check. +@interface Blech : Baz +// CHECK-MESSAGES-NOT: :[[@LINE-1]]:12: warning: Objective-C interface 'Blech' subclasses 'Baz', which is not intended to be subclassed [objc-forbidden-subclassing] +@end diff --git a/clang-tools-extra/unittests/clang-tidy/ObjCModuleTest.cpp b/clang-tools-extra/unittests/clang-tidy/ObjCModuleTest.cpp index c50480a..92ae8e1 100644 --- a/clang-tools-extra/unittests/clang-tidy/ObjCModuleTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/ObjCModuleTest.cpp @@ -9,12 +9,40 @@ #include "ClangTidyTest.h" #include "gtest/gtest.h" +#include "objc/ForbiddenSubclassingCheck.h" + +using namespace clang::tidy::objc; namespace clang { namespace tidy { namespace test { -// TODO(D39142) Add unit tests for the ObjC module here once a check lands. +TEST(ObjCForbiddenSubclassing, AllowedSubclass) { + std::vector Errors; + runCheckOnCode( + "@interface Foo\n" + "@end\n" + "@interface Bar : Foo\n" + "@end\n", + &Errors, + "input.m"); + EXPECT_EQ(0ul, Errors.size()); +} + +TEST(ObjCForbiddenSubclassing, ForbiddenSubclass) { + std::vector Errors; + runCheckOnCode( + "@interface UIImagePickerController\n" + "@end\n" + "@interface Foo : UIImagePickerController\n" + "@end\n", + &Errors, + "input.m"); + EXPECT_EQ(1ul, Errors.size()); + EXPECT_EQ( + "Objective-C interface 'Foo' subclasses 'UIImagePickerController', which is not intended to be subclassed", + Errors[0].Message.Message); +} } // namespace test } // namespace tidy -- 2.7.4