[clang-tidy] Add a check for [super self] in initializers 🔍

[stephanemoore]

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.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

62

Please don't use auto where type is not obvious.

clang-tools-extra/docs/ReleaseNotes.rst

110

Please enclose NSObject in ``. Probably same for -self if this is language construct.

clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst

7

Please synchronize with Release Notes.

[aaron.ballman]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

58

This matcher seems like it would be generally useful -- would you mind adding it to the AST matcher interface rather than local to this check? It doesn't have to be done as part of this patch (we can leave the matcher here for the moment).

111

Is there a %0 missing from the diagnostic for this method declaration you're passing in?

[stephanemoore]

Removed usage of auto with a nonobvious type in isSubclassOf matcher.
Added backticks around NSObject in docs.
Synchronized check description in release notes and check documentation.

[stephanemoore]

Fix diagnostic format string to actually use the message's method declaration.

[stephanemoore]

Enclose -self in backticks in release notes and check documentation.

[stephanemoore]

Enclose -self in backticks in check documentation (overlooked in previous attempt).

[stephanemoore]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

58

Definitely agreed. I will send out a followup for a new AST matcher.

111

Good catch! That's egg on my face 😅🥚

I think I was internally conflicted over specifically mentioning -[NSObject self] versus using the method declaration of the message expression and somehow got stuck halfway in-between 😓 I think it's better to mention the method declaration of the message. Let me know if you think that I should instead mention -[NSObject self].

clang-tools-extra/docs/ReleaseNotes.rst

110

Good catch. Done.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

113

Please use double backticks for language constructs. Same in other places.

[stephanemoore]

Use double backticks rather than single backticks for symbols in documentation.

[stephanemoore]

clang-tools-extra/docs/ReleaseNotes.rst

113

Sorry I misread your earlier comment. I have been editing too much Markdown recently 😅 I have updated to use double backticks.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

111

Whoops; I forgot to update the message in the test.

[stephanemoore]

Update tests to match updated diagnostic.

[aaron.ballman]

I think this basically LGTM, but I'd appreciate hearing from someone more well-versed in ObjC before landing this. My primary question is: are there situations where [super self] is sensible (if so, how should the user silence the diagnostic; will NOLINT suffice or be too annoying), and are there any other circumstances where the fix-it is dangerous that we should be concerned by?

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

This could be dangerous if the [super self] construct is in a macro, couldn't it? e.g.,

#define DERP self

[super DERP];

[jordan_rose]

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Diagnostic nitpick: the Objective-C term is "init method", not "initializer".

[stephanemoore]

In D59806#1447929, @jordan_rose wrote:

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Agreed.

The only relatively common usage of -[NSObject self] that I am aware of is through KVC, e.g., to construct a set of distinct objects using array operators:

NSArray *uniqueObjects = [array valueForKeyPath:@"@distinctUnionOfObjects.self"]

I don't recall observing -[NSObject self] used in other situations.

Diagnostic nitpick: the Objective-C term is "init method", not "initializer".

Good catch. Glancing through the project, I have the impression that there is some inconsistency in the terminology used by different diagnostics and documentation in the project. For example, the ARC documentation on method families and various tests use the term "init methods". In contrast, -Wobjc-designated-initializers uses the term "initializer". The general pattern seems to be that documentation/diagnostics related to ARC use the term "init method" and diagnostics/documentation related to designated initializers use the term "initializer". I had assumed that "initializer" would be more intuitive to readers as this is the term typically used in Apple developer documentation, e.g., Concepts in Objective-C Programming > Object Initialization.

I believe that "initializer" will be more intuitive because of Apple documentation but I can change to "init method" if that is preferred. Please let me know what your preference is.

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

Good point. Let me add some test cases and make sure this is handled properly.

[jordan_rose]

Ooh, I should have remembered "designated initializer". I guess it doesn't matter so much either way.

[stephanemoore]

Add test cases with [super self] expanded from macros.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

Added some test cases where [super self] is expanded from macros.

[rjmccall]

In D59806#1448537, @stephanemoore wrote:

In D59806#1447929, @jordan_rose wrote:

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Agreed.

The only relatively common usage of -[NSObject self] that I am aware of is through KVC, e.g., to construct a set of distinct objects using array operators:

It's used idiomatically in ARC and GC as a way of preventing an object from being deallocated before a certain point, since the compiler cannot eliminate the call to self. But that shouldn't affect this warning.

[aaron.ballman]

In D59806#1447929, @jordan_rose wrote:

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Thank you for the verification! And agreed about the macro thing -- I am only worried about trying to issue a fix-it in that case, not the diagnostic itself.

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

You missed the test case I was worried about -- where the macro is mixed into the expression. I don't think we want to try to add a fix-it in that case.

clang-tools-extra/test/clang-tidy/objc-super-self.m

42

Are you missing a CHECK-FIXES here?

Personally, I don't think we should try to generate a fixit for this case, so I would expect a CHECK-FIXES that ensures this doesn't get modified.

[stephanemoore]

Add a test case where a macro emits just self in the message expression.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

Added a test case though at the moment it generates a fixit.

Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?

#if FLAG
#define INVOCATION self
#else
#define INVOCATION init
#endif

- (instancetype)init {
  return [super INVOCATION];
}

clang-tools-extra/test/clang-tidy/objc-super-self.m

42

No fix is currently generated for this case which is why there is no CHECK-FIXES. I also agree that no fix should be generated for this case.

I must confess that I have yet to fully understand why the fix for this case is discarded (though I am grateful for the behavior). Let me dig around to try to better understand why no fixit is generated for this case and assess adding a condition for emitting the fixit.

[aaron.ballman]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?

Essentially, yes. Macros can get arbitrarily complex and so our rule of thumb is to not apply fix-its when the code being fixed is within a macro. Another example that can be tricky is adding another layer of macros:

#define FOO super
#define BAR self
#define BAZ FOO BAR

- (instancetype)init {
  return [BAZ];
}

clang-tools-extra/test/clang-tidy/objc-super-self.m

42

Our typical way to check that a fix is not applied is to use // CHECK-FIXES: <original code> to test that the fix was not applied.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

113

Thanks for the explanation! I'll get started making the changes 👍

[stephanemoore]

Update check to avoid emitting a fix if the expression is in a macro.

[stephanemoore]

Add CHECK-FIXES to verify code is preserved for scenarios that should not have fixes.

[stephanemoore]

Check if either the receiver or selector are in macro locations.

[stephanemoore]

Fix some formatting issues.

[stephanemoore]

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

58

Sent out https://reviews.llvm.org/D60543.

clang-tools-extra/test/clang-tidy/objc-super-self.m

42

Gotcha. Added CHECK-FIXES to verify original code is preserved.

[aaron.ballman]

LGTM! You can either land this now and refactor after the AST matcher lands, or you can wait until the AST matcher lands and land this patch after -- your call.

[stephanemoore]

In D59806#1469257, @aaron.ballman wrote:

LGTM! You can either land this now and refactor after the AST matcher lands, or you can wait until the AST matcher lands and land this patch after -- your call.

I will plan on landing this and then follow up after the new AST matcher lands. I want to make sure the design for the new AST matcher is solid and evaluate the implications of the different approaches. I plan on updating both [ForbiddenSubclassingCheck](https://github.com/llvm/llvm-project/blob/2946cd7/clang-tools-extra/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L55) and SuperSelfCheck once the new AST matcher lands 👍

clang-tools-extra/clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   AvoidNSErrorInitCheck.cpp
   AvoidSpinlockCheck.cpp
   ForbiddenSubclassingCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
+  SuperSelfCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidNSErrorInitCheck.h"
 #include "AvoidSpinlockCheck.h"
 #include "ForbiddenSubclassingCheck.h"
 #include "PropertyDeclarationCheck.h"
+#include "SuperSelfCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
     CheckFactories.registerCheck<AvoidSpinlockCheck>(
         "objc-avoid-spinlock");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
+    CheckFactories.registerCheck<SuperSelfCheck>(
+        "objc-super-self");
   }
 };
 
 // Register the ObjCTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ObjCModule> X(
     "objc-module",
     "Adds Objective-C lint checks.");
 
 } // namespace objc
 

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp

+//===--- 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");
+
+  diag(Message->getBeginLoc(), "suspicious invocation of %0 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

clang-tools-extra/docs/ReleaseNotes.rst

   Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
   in the Time domain instead of the numeric domain.
 
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   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
   <clang-tidy/checks/modernize-use-override>` was added.
 
 - The :doc:`bugprone-argument-comment
   <clang-tidy/checks/bugprone-argument-comment>` now supports
   `CommentBoolLiterals`, `CommentIntegerLiterals`, `CommentFloatLiterals`,
   `CommentUserDefiniedLiterals`, `CommentStringLiterals`,
   `CommentCharacterLiterals` & `CommentNullPtrs` options.

clang-tools-extra/docs/clang-tidy/checks/list.rst

    modernize-use-override
    modernize-use-transparent-functors
    modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    objc-avoid-nserror-init
    objc-avoid-spinlock
    objc-forbidden-subclassing
    objc-property-declaration
+   objc-super-self
    openmp-exception-escape
    openmp-use-default-none
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-algorithm
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    performance-move-const-arg
    performance-move-constructor-init

clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst

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

clang-tools-extra/test/clang-tidy/objc-super-self.m

+// 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
+