stephanemoore created this revision. Herald added subscribers: cfe-commits, jdoerfert, xazax.hun, mgorny. Herald added a project: clang. stephanemoore edited the summary of this revision.
This check aims to address a relatively common benign error where Objective-C subclass initializers call -self on their superclass instead of invoking a superclass initializer, typically -init. The error is typically benign because libobjc recognizes that improper initializer chaining is commonยน. One theory for the frequency of this error might be that -init and -self have the same return type which could potentially cause inappropriate autocompletion to -self instead of -init. The equal selector lengths and triviality of common initializer code probably contribute to errors like this slipping through code review undetected. This check aims to flag errors of this form in the interests of correctness and reduce incidence of initialization failing to chain to -[NSObject init]. [1] "In practice, it will be hard to rely on this function. Many classes do not properly chain -init calls." >From _objc_rootInit in >https://opensource.apple.com/source/objc4/objc4-750.1/runtime/NSObject.mm.auto.html. Test Notes: Verified via `make check-clang-tools`. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D59806 Files: clang-tools-extra/clang-tidy/objc/CMakeLists.txt clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp clang-tools-extra/clang-tidy/objc/SuperSelfCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst clang-tools-extra/test/clang-tidy/objc-super-self.m
Index: clang-tools-extra/test/clang-tidy/objc-super-self.m =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/objc-super-self.m @@ -0,0 +1,54 @@ +// RUN: %check_clang_tidy %s objc-super-self %t + +@interface NSObject +- (instancetype)init; +- (instancetype)self; +@end + +@interface NSObjectDerivedClass : NSObject +@end + +@implementation NSObjectDerivedClass + +- (instancetype)init { + return [super self]; +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious invocation of -[NSObject self] in initializer; did you mean to invoke a superclass initializer? [objc-super-self] +// CHECK-FIXES: return [super init]; +} + +- (instancetype)initWithObject:(NSObject *)obj { + self = [super self]; +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious invocation of -[NSObject self] in initializer; did you mean to invoke a superclass initializer? [objc-super-self] +// CHECK-FIXES: self = [super init]; + if (self) { + // ... + } + return self; +} + +- (instancetype)foo { + return [super self]; +} + +- (instancetype)bar { + return [self self]; +} + +@end + +@interface RootClass +- (instancetype)init; +- (instancetype)self; +@end + +@interface NotNSObjectDerivedClass : RootClass +@end + +@implementation NotNSObjectDerivedClass + +- (instancetype)init { + return [super self]; +} + +@end + Index: clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst @@ -0,0 +1,14 @@ +.. title:: clang-tidy - objc-super-self + +objc-super-self +=============== + +Finds invocations of -self on super instances (`[super self]`) in initializers +of subclasses of NSObject and recommends invoking a superclass initializer +instead. + +`[super self]` is a common programmer error when the programmer's original +intent is to call a superclass initializer. Failing to call a superclass +initializer breaks initializer chaining and can result in invalid object +initialization. + Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -227,6 +227,7 @@ objc-avoid-spinlock objc-forbidden-subclassing objc-property-declaration + objc-super-self performance-faster-string-find performance-for-range-copy performance-implicit-conversion-in-loop Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,11 @@ Checks whether there are underscores in googletest test and test case names in test macros, which is prohibited by the Googletest FAQ. +- New :doc:`objc-super-self <clang-tidy/checks/objc-super-self>` check. + + Finds invocations of -self on super instances in initializers of subclasses + of NSObject and recommends calling a superclass initializer instead. + - New alias :doc:`cppcoreguidelines-explicit-virtual-functions <clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>` to :doc:`modernize-use-override Index: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/objc/SuperSelfCheck.h @@ -0,0 +1,36 @@ +//===--- SuperSelfCheck.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_SUPERSELFCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_SUPERSELFCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace objc { + +/// Finds invocations of -self on super instances in initializers of subclasses +/// of NSObject and recommends calling a superclass initializer instead. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/objc-super-self.html +class SuperSelfCheck : public ClangTidyCheck { +public: + SuperSelfCheck(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 tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_SUPERSELFCHECK_H Index: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp @@ -0,0 +1,117 @@ +//===--- SuperSelfCheck.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 "SuperSelfCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace objc { + +namespace { + +/// \brief Matches Objective-C methods in the initializer family. +/// +/// Example matches -init and -initWithInt:. +/// (matcher = objcMethodDecl(isInitializer())) +/// \code +/// @interface Foo +/// - (instancetype)init; +/// - (instancetype)initWithInt:(int)i; +/// + (instancetype)init; +/// - (void)bar; +/// @end +/// \endcode +AST_MATCHER(ObjCMethodDecl, isInitializer) { + return Node.getMethodFamily() == OMF_init; +} + +/// \brief Matches Objective-C implementations of classes that directly or +/// indirectly have a superclass matching \c InterfaceDecl. +/// +/// Note that a class is not considered to be a subclass of itself. +/// +/// Example matches implementation declarations for Y and Z. +/// (matcher = objcInterfaceDecl(isSubclassOf(hasName("X")))) +/// \code +/// @interface X +/// @end +/// @interface Y : X +/// @end +/// @implementation Y // directly derived +/// @end +/// @interface Z : Y +/// @end +/// @implementation Z // indirectly derived +/// @end +/// \endcode +AST_MATCHER_P(ObjCImplementationDecl, isSubclassOf, + ast_matchers::internal::Matcher<ObjCInterfaceDecl>, + InterfaceDecl) { + // Check if any of the superclasses of the class match. + for (const auto *SuperClass = Node.getClassInterface()->getSuperClass(); + SuperClass != nullptr; SuperClass = SuperClass->getSuperClass()) { + if (InterfaceDecl.matches(*SuperClass, Finder, Builder)) + return true; + } + + // No matches found. + return false; +} + +/// \brief Matches Objective-C message expressions where the receiver is the +/// super instance. +/// +/// Example matches the invocations of -banana and -orange. +/// (matcher = objcMessageExpr(isMessagingSuperInstance())) +/// \code +/// - (void)banana { +/// [self apple] +/// [super banana]; +/// [super orange]; +/// } +/// \endcode +AST_MATCHER(ObjCMessageExpr, isMessagingSuperInstance) { + return Node.getReceiverKind() == ObjCMessageExpr::SuperInstance; +} + +} // namespace + +void SuperSelfCheck::registerMatchers(MatchFinder *Finder) { + // This check should only be applied to Objective-C sources. + if (!getLangOpts().ObjC) + return; + + Finder->addMatcher( + objcMessageExpr( + hasSelector("self"), isMessagingSuperInstance(), + hasAncestor(objcMethodDecl(isInitializer(), + hasDeclContext(objcImplementationDecl( + isSubclassOf(hasName("NSObject"))))))) + .bind("message"), + this); +} + +void SuperSelfCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Message = Result.Nodes.getNodeAs<ObjCMessageExpr>("message"); + + diag(Message->getBeginLoc(), "suspicious invocation of -[NSObject self] in " + "initializer; did you mean to " + "invoke a superclass initializer?") + << Message->getMethodDecl() + << FixItHint::CreateReplacement(Message->getSourceRange(), + StringRef("[super init]")); +} + +} // namespace objc +} // namespace tidy +} // namespace clang Index: clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp +++ clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp @@ -13,6 +13,7 @@ #include "AvoidSpinlockCheck.h" #include "ForbiddenSubclassingCheck.h" #include "PropertyDeclarationCheck.h" +#include "SuperSelfCheck.h" using namespace clang::ast_matchers; @@ -31,6 +32,8 @@ "objc-forbidden-subclassing"); CheckFactories.registerCheck<PropertyDeclarationCheck>( "objc-property-declaration"); + CheckFactories.registerCheck<SuperSelfCheck>( + "objc-super-self"); } }; Index: clang-tools-extra/clang-tidy/objc/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/objc/CMakeLists.txt +++ clang-tools-extra/clang-tidy/objc/CMakeLists.txt @@ -6,6 +6,7 @@ ForbiddenSubclassingCheck.cpp ObjCTidyModule.cpp PropertyDeclarationCheck.cpp + SuperSelfCheck.cpp LINK_LIBS clangAST
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits