[clang-tidy] Add a check for [super self] in initializers 🔍
Summary:
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`.
Subscribers: mgorny, xazax.hun, jdoerfert, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D59806
git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@358620 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/clang-tidy/objc/CMakeLists.txt b/clang-tidy/objc/CMakeLists.txt
index 4063bba..4eeb148 100644
--- a/clang-tidy/objc/CMakeLists.txt
+++ b/clang-tidy/objc/CMakeLists.txt
@@ -6,6 +6,7 @@
ForbiddenSubclassingCheck.cpp
ObjCTidyModule.cpp
PropertyDeclarationCheck.cpp
+ SuperSelfCheck.cpp
LINK_LIBS
clangAST
diff --git a/clang-tidy/objc/ObjCTidyModule.cpp b/clang-tidy/objc/ObjCTidyModule.cpp
index 5686c9f..636e2c0 100644
--- a/clang-tidy/objc/ObjCTidyModule.cpp
+++ b/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");
}
};
diff --git a/clang-tidy/objc/SuperSelfCheck.cpp b/clang-tidy/objc/SuperSelfCheck.cpp
new file mode 100644
index 0000000..7aafd66
--- /dev/null
+++ b/clang-tidy/objc/SuperSelfCheck.cpp
@@ -0,0 +1,127 @@
+//===--- 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 ObjCInterfaceDecl *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");
+
+ auto Diag = diag(Message->getExprLoc(), "suspicious invocation of %0 in "
+ "initializer; did you mean to "
+ "invoke a superclass initializer?")
+ << Message->getMethodDecl();
+
+ SourceLocation ReceiverLoc = Message->getReceiverRange().getBegin();
+ if (ReceiverLoc.isMacroID() || ReceiverLoc.isInvalid())
+ return;
+
+ SourceLocation SelectorLoc = Message->getSelectorStartLoc();
+ if (SelectorLoc.isMacroID() || SelectorLoc.isInvalid())
+ return;
+
+ Diag << FixItHint::CreateReplacement(Message->getSourceRange(),
+ StringRef("[super init]"));
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tidy/objc/SuperSelfCheck.h b/clang-tidy/objc/SuperSelfCheck.h
new file mode 100644
index 0000000..ed5d1cd
--- /dev/null
+++ b/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
diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index bf7e069..b62833d 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -108,6 +108,12 @@
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
diff --git a/docs/clang-tidy/checks/list.rst b/docs/clang-tidy/checks/list.rst
index 1b11a1a..201cceb 100644
--- a/docs/clang-tidy/checks/list.rst
+++ b/docs/clang-tidy/checks/list.rst
@@ -227,6 +227,7 @@
objc-avoid-spinlock
objc-forbidden-subclassing
objc-property-declaration
+ objc-super-self
openmp-exception-escape
openmp-use-default-none
performance-faster-string-find
diff --git a/docs/clang-tidy/checks/objc-super-self.rst b/docs/clang-tidy/checks/objc-super-self.rst
new file mode 100644
index 0000000..ed0bb29
--- /dev/null
+++ b/docs/clang-tidy/checks/objc-super-self.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - objc-super-self
+
+objc-super-self
+===============
+
+Finds invocations of ``-self`` on super instances in initializers of subclasses
+of ``NSObject`` and recommends calling a superclass initializer instead.
+
+Invoking ``-self`` on super instances in initializers 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.
+
diff --git a/test/clang-tidy/objc-super-self.m b/test/clang-tidy/objc-super-self.m
new file mode 100644
index 0000000..9653cd2
--- /dev/null
+++ b/test/clang-tidy/objc-super-self.m
@@ -0,0 +1,86 @@
+// 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 '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 'self' in initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+// CHECK-FIXES: self = [super init];
+ if (self) {
+ // ...
+ }
+ return self;
+}
+
+#define INITIALIZE() [super self]
+
+- (instancetype)initWithObject:(NSObject *)objc a:(int)a {
+ return INITIALIZE();
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious invocation of 'self' in initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+// CHECK-FIXES: return INITIALIZE();
+}
+
+#define INITIALIZER_IMPL() return [super self]
+
+- (instancetype)initWithObject:(NSObject *)objc b:(int)b {
+ INITIALIZER_IMPL();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: suspicious invocation of 'self' in initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+// CHECK-FIXES: INITIALIZER_IMPL();
+}
+
+#define INITIALIZER_METHOD self
+
+- (instancetype)initWithObject:(NSObject *)objc c:(int)c {
+ return [super INITIALIZER_METHOD];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious invocation of 'self' in initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+// CHECK-FIXES: return [super INITIALIZER_METHOD];
+}
+
+#define RECEIVER super
+
+- (instancetype)initWithObject:(NSObject *)objc d:(int)d {
+ return [RECEIVER self];
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious invocation of 'self' in initializer; did you mean to invoke a superclass initializer? [objc-super-self]
+// CHECK-FIXES: return [RECEIVER 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
+
