[clang-tidy] New check calling out uses of +new in Objective-C code

gribozavr · gribozavr · commit 1520dafa20ed · 2019-05-23T12:01:26.000Z
Summary: Google's Objective-C style guide forbids calling or overriding +new to instantiate objects. This check warns on violations. Style guide reference: https://google.github.io/styleguide/objcguide.html#do-not-use-new Patch by Michael Wyman. Reviewers: benhamilton, aaron.ballman, JonasToth, gribozavr, ilya-biryukov, stephanemoore, mwyman Reviewed By: aaron.ballman, gribozavr, stephanemoore, mwyman Subscribers: stephanemoore, xazax.hun, Eugene.Zelenko, mgorny, cfe-commits Tags: #clang, #clang-tools-extra Differential Revision: https://reviews.llvm.org/D61350 llvm-svn: 361487
diff --git a/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp b/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
@@ -0,0 +1,130 @@
+//===--- AvoidNSObjectNewCheck.cpp - clang-tidy ---------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidNSObjectNewCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <map>
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace objc {
+
+static bool isMessageExpressionInsideMacro(const ObjCMessageExpr *Expr) {
+  SourceLocation ReceiverLocation = Expr->getReceiverRange().getBegin();
+  if (ReceiverLocation.isMacroID())
+    return true;
+
+  SourceLocation SelectorLocation = Expr->getSelectorStartLoc();
+  if (SelectorLocation.isMacroID())
+    return true;
+
+  return false;
+}
+
+// Walk up the class hierarchy looking for an -init method, returning true
+// if one is found and has not been marked unavailable.
+static bool isInitMethodAvailable(const ObjCInterfaceDecl *ClassDecl) {
+  while (ClassDecl != nullptr) {
+    for (const auto *MethodDecl : ClassDecl->instance_methods()) {
+      if (MethodDecl->getSelector().getAsString() == "init")
+        return !MethodDecl->isUnavailable();
+    }
+    ClassDecl = ClassDecl->getSuperClass();
+  }
+
+  // No -init method found in the class hierarchy. This should occur only rarely
+  // in Objective-C code, and only really applies to classes not derived from
+  // NSObject.
+  return false;
+}
+
+// Returns the string for the Objective-C message receiver. Keeps any generics
+// included in the receiver class type, which are stripped if the class type is
+// used. While the generics arguments will not make any difference to the
+// returned code at this time, the style guide allows them and they should be
+// left in any fix-it hint.
+static StringRef getReceiverString(SourceRange ReceiverRange,
+                                   const SourceManager &SM,
+                                   const LangOptions &LangOpts) {
+  CharSourceRange CharRange = Lexer::makeFileCharRange(
+      CharSourceRange::getTokenRange(ReceiverRange), SM, LangOpts);
+  return Lexer::getSourceText(CharRange, SM, LangOpts);
+}
+
+static FixItHint getCallFixItHint(const ObjCMessageExpr *Expr,
+                                  const SourceManager &SM,
+                                  const LangOptions &LangOpts) {
+  // Check whether the messaged class has a known factory method to use instead
+  // of -init.
+  StringRef Receiver =
+      getReceiverString(Expr->getReceiverRange(), SM, LangOpts);
+  // Some classes should use standard factory methods instead of alloc/init.
+  std::map<StringRef, StringRef> ClassToFactoryMethodMap = {{"NSDate", "date"},
+                                                            {"NSNull", "null"}};
+  auto FoundClassFactory = ClassToFactoryMethodMap.find(Receiver);
+  if (FoundClassFactory != ClassToFactoryMethodMap.end()) {
+    StringRef ClassName = FoundClassFactory->first;
+    StringRef FactorySelector = FoundClassFactory->second;
+    std::string NewCall =
+        llvm::formatv("[{0} {1}]", ClassName, FactorySelector);
+    return FixItHint::CreateReplacement(Expr->getSourceRange(), NewCall);
+  }
+
+  if (isInitMethodAvailable(Expr->getReceiverInterface())) {
+    std::string NewCall = llvm::formatv("[[{0} alloc] init]", Receiver);
+    return FixItHint::CreateReplacement(Expr->getSourceRange(), NewCall);
+  }
+
+  return {}; // No known replacement available.
+}
+
+void AvoidNSObjectNewCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().ObjC)
+    return;
+
+  // Add two matchers, to catch calls to +new and implementations of +new.
+  Finder->addMatcher(
+      objcMessageExpr(isClassMessage(), hasSelector("new")).bind("new_call"),
+      this);
+  Finder->addMatcher(
+      objcMethodDecl(isClassMethod(), isDefinition(), hasName("new"))
+          .bind("new_override"),
+      this);
+}
+
+void AvoidNSObjectNewCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *CallExpr =
+          Result.Nodes.getNodeAs<ObjCMessageExpr>("new_call")) {
+    // Don't warn if the call expression originates from a macro expansion.
+    if (isMessageExpressionInsideMacro(CallExpr))
+      return;
+
+    diag(CallExpr->getExprLoc(), "do not create objects with +new")
+        << getCallFixItHint(CallExpr, *Result.SourceManager,
+                            Result.Context->getLangOpts());
+  }
+
+  if (const auto *DeclExpr =
+          Result.Nodes.getNodeAs<ObjCMethodDecl>("new_override")) {
+    diag(DeclExpr->getBeginLoc(), "classes should not override +new");
+  }
+}
+
+} // namespace objc
+} // namespace google
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h b/clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h
@@ -0,0 +1,38 @@
+//===--- AvoidNSObjectNewCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDNSOBJECTNEWCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_AVOIDNSOBJECTNEWCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace objc {
+
+/// This check finds Objective-C code that uses +new to create object instances,
+/// or overrides +new in classes. Both are forbidden by Google's Objective-C
+/// style guide.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/google-avoid-nsobject-new.html
+class AvoidNSObjectNewCheck : public ClangTidyCheck {
+public:
+  AvoidNSObjectNewCheck(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_AVOIDNSOBJECTNEWCHECK_H
diff --git 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
+  AvoidNSObjectNewCheck.cpp
   AvoidThrowingObjCExceptionCheck.cpp
   AvoidUnderscoreInGoogletestNameCheck.cpp
   DefaultArgumentsCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp b/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
@@ -13,6 +13,7 @@
 #include "../readability/FunctionSizeCheck.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "AvoidCStyleCastsCheck.h"
+#include "AvoidNSObjectNewCheck.h"
 #include "AvoidThrowingObjCExceptionCheck.h"
 #include "AvoidUnderscoreInGoogletestNameCheck.h"
 #include "DefaultArgumentsCheck.h"
@@ -49,6 +50,8 @@ class GoogleModule : public ClangTidyModule {
         "google-explicit-constructor");
     CheckFactories.registerCheck<readability::GlobalNamesInHeadersCheck>(
         "google-global-names-in-headers");
+    CheckFactories.registerCheck<objc::AvoidNSObjectNewCheck>(
+        "google-objc-avoid-nsobject-new");
     CheckFactories.registerCheck<objc::AvoidThrowingObjCExceptionCheck>(
         "google-objc-avoid-throwing-exception");
     CheckFactories.registerCheck<objc::FunctionNamingCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -122,6 +122,12 @@ Improvements to clang-tidy
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
+- New :doc:`google-objc-avoid-nsobject-new
+  <clang-tidy/checks/google-objc-avoid-nsobject-new>` check.
+
+  Checks for calls to ``+new`` or overrides of it, which are prohibited by the
+  Google Objective-C style guide.
+
 - New :doc:`objc-super-self <clang-tidy/checks/objc-super-self>` check.
 
   Finds invocations of ``-self`` on super instances in initializers of
diff --git a/clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst b/clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - google-objc-avoid-nsobject-new
+
+google-objc-avoid-nsobject-new
+==============================
+
+Finds calls to ``+new`` or overrides of it, which are prohibited by the
+Google Objective-C style guide.
+
+The Google Objective-C style guide forbids calling ``+new`` or overriding it in
+class implementations, preferring ``+alloc`` and ``-init`` methods to
+instantiate objects.
+
+An example:
+
+.. code-block:: objc
+
+  NSDate *now = [NSDate new];
+  Foo *bar = [Foo new];
+
+Instead, code should use ``+alloc``/``-init`` or class factory methods.
+
+.. code-block:: objc
+
+  NSDate *now = [NSDate date];
+  Foo *bar = [[Foo alloc] init];
+
+This check corresponds to the Google Objective-C Style Guide rule
+`Do Not Use +new
+<https://google.github.io/styleguide/objcguide.html#do-not-use-new>`_.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -135,6 +135,7 @@ Clang-Tidy Checks
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
+   google-objc-avoid-nsobject-new
    google-objc-avoid-throwing-exception
    google-objc-function-naming
    google-objc-global-variable-declaration
diff --git a/clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m b/clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s google-objc-avoid-nsobject-new %t
+
+@interface NSObject
++ (instancetype)new;
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+@interface NSProxy  // Root class with no -init method.
+@end
+
+// NSDate provides a specific factory method.
+@interface NSDate : NSObject
++ (instancetype)date;
+@end
+
+// For testing behavior with Objective-C Generics.
+@interface NSMutableDictionary<__covariant KeyType, __covariant ObjectType> : NSObject
+@end
+
+@class NSString;
+
+#define ALLOCATE_OBJECT(_Type) [_Type new]
+
+void CheckSpecificInitRecommendations(void) {
+  NSObject *object = [NSObject new];
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+  // CHECK-FIXES: [NSObject alloc] init];
+
+  NSDate *correctDate = [NSDate date];
+  NSDate *incorrectDate = [NSDate new];
+  // CHECK-MESSAGES: [[@LINE-1]]:27: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+  // CHECK-FIXES: [NSDate date];
+
+  NSObject *macroCreated = ALLOCATE_OBJECT(NSObject);  // Shouldn't warn on macros.
+
+  NSMutableDictionary *dict = [NSMutableDictionary<NSString *, NSString *> new];
+  // CHECK-MESSAGES: [[@LINE-1]]:31: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+  // CHECK-FIXES: [NSMutableDictionary<NSString *, NSString *> alloc] init];
+}
+
+@interface Foo : NSObject
++ (instancetype)new; // Declare again to suppress warning.
+- (instancetype)initWithInt:(int)anInt;
+- (instancetype)init __attribute__((unavailable));
+
+- (id)new;
+@end
+
+@interface Baz : Foo // Check unavailable -init through inheritance.
+@end
+
+@interface ProxyFoo : NSProxy
++ (instancetype)new;
+@end
+
+void CallNewWhenInitUnavailable(void) {
+  Foo *foo = [Foo new];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+
+  Baz *baz = [Baz new];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+
+  // Instance method -new calls may be weird, but are not strictly forbidden.
+  Foo *bar = [[Foo alloc] initWithInt:4];
+  [bar new];
+
+  ProxyFoo *proxy = [ProxyFoo new];
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+}
+
+@interface HasNewOverride : NSObject
+@end
+
+@implementation HasNewOverride
++ (instancetype)new {
+  return [[self alloc] init];
+}
+// CHECK-MESSAGES: [[@LINE-3]]:1: warning: classes should not override +new [google-objc-avoid-nsobject-new]
+@end
