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

[mwyman]

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

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

29 ↗

(On Diff #197463)

Please use static instead anonymous namespace for functions. See LLVM Coding Guidelines. Same for other functions.

clang-tools-extra/docs/ReleaseNotes.rst

107 ↗

(On Diff #197463)

Please highlight +new with double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst

4 ↗

(On Diff #197463)

Length should be same as title.

6 ↗

(On Diff #197463)

Please synchronize first statement with Release Notes.

9 ↗

(On Diff #197463)

Please highlight +alloc and -init with double back-ticks.

19 ↗

(On Diff #197463)

Please highlight +alloc/-init with double back-ticks.

[mwyman]

Fixes per review comments.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

16 ↗

(On Diff #197660)

Unnecessary empty line.

29 ↗

(On Diff #197660)

Please elide braces.

34 ↗

(On Diff #197660)

Please elide braces.

46 ↗

(On Diff #197660)

Please elide braces.

94 ↗

(On Diff #197660)

You could return {}

115 ↗

(On Diff #197660)

Please elide braces.

clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst

27 ↗

(On Diff #197660)

I think you could format link in cert-env33-c.rst style.

[mwyman]

Updated per review comments.

[aaron.ballman]

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

73 ↗

(On Diff #197681)

We don't typically make local value types const (only pointers or references); here and elsewhere in the patch.

76–77 ↗

(On Diff #197681)

Should this be configurable, or will users not need to control the behavior here?

[mwyman]

Updated to address code review comments.

[mwyman]

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

76–77 ↗

(On Diff #197681)

For now I think there is only this handful of Foundation types commonly created only with the factory methods; I'm not sure it makes a huge amount of sense to open it for configuration at this time.

[gribozavr]

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

55 ↗

(On Diff #198495)

"Returns"

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h

19 ↗

(On Diff #198495)

It is unclear whether the check ensures that the code does not use +new, or whether it finds code that does not use +new, this description can be read both ways.

Try: This check finds Objective-C code that ...

clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst

6 ↗

(On Diff #198495)

"Finds calls to ..."

[aaron.ballman]

LGTM modulo the comments from @gribozavr.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

76–77 ↗

(On Diff #197681)

That seems reasonable to me -- we can add an option later if we find we need one.

[mwyman]

Update comments/doc based on feedback.

[stephanemoore]

Looks good to me once the comments from other reviewers have been addressed. I added a couple suggestions as well.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

112 ↗

(On Diff #198503)

If the message expression is within a macro expansion, maybe we should emit the diagnostic without the fixit?

clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m

1 ↗

(On Diff #198503)

Suggestion:
It might be good to add a test case for another root class that doesn't have -init, e.g., NSProxy.

[mwyman]

Update for review comments.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

112 ↗

(On Diff #198503)

I'm leery of emitting a warning in a case where the code may not originate from the code in question. Maybe if the macro is defined in the same source file, but I think perhaps that could be a follow-up.

[mwyman]

I don't have commit access, so if somebody could submit them that would be wonderful!

[stephanemoore]

I believe that all the feedback from @gribozavr has been addressed modulo one small nit. Once that nit has been addressed, I can land this.

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h

19 ↗

(On Diff #198702)

I believe @gribozavr recommended "This check".

[mwyman]

Update for comments

[stephanemoore]

clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp

112 ↗

(On Diff #198503)

I think that it's reasonable to surface the diagnostic. If the user determines that the diagnostic is not relevant in this situation then they can suppress it. That is, I believe it's preferable to surface issues and have the user determine relevance rather than suppress diagnostics without the knowledge of the user.

(with that said, I do not consider this a hard blocker and am comfortable deferring this for future consideration)

clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m

53 ↗

(On Diff #199338)

Shouldn't you declare +[ProxyFoo new] so that you can call it on line 67?

[mwyman]

Added +new declaration for ProxyFoo.

[mwyman]

Bah, previous changes not caught in Git commit; switching back and forth between Git/Mercurial makes for some mix-ups, I guess.

[stephanemoore]

clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp

54 ↗

(On Diff #199375)

It is a little odd that "google-objc-avoid-nsobject-new" warns on +new even for classes that are not derived from NSObject. In a sense, "nsobject-new" is being used as an identifier for any class method named "new". Interestingly, renaming to the more direct "google-objc-avoid-new-class-method" is even more misleading. I have yet to think of a name that I think would actually be better.

While I think the current name is potentially misleading, I think I am okay with this name for the following reasons:
(1) I believe this will only be misleading in marginal scenarios. I believe that it will be exceedingly rare for a class method named new to exist in a class hierarchy where the root class is not NSObject.
(2) We can always amend the check to restrict it to NSObject. In the unexpected circumstance where someone does provide a legitimate use case for a class method named new in a class hierarchy where the root class is not NSObject, we can simply amend the check to restrict its enforcement to class hierarchies where the root class is NSObject.

[mwyman]

Syncing code with HEAD

clang-tidy/google/AvoidNSObjectNewCheck.h

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

clang-tidy/google/AvoidNSObjectNewCheck.cpp

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

clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
+  AvoidNSObjectNewCheck.cpp
   AvoidThrowingObjCExceptionCheck.cpp
   AvoidUnderscoreInGoogletestNameCheck.cpp
   DefaultArgumentsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   FunctionNamingCheck.cpp
   GlobalNamesInHeadersCheck.cpp
   GlobalVariableDeclarationCheck.cpp

clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/BracesAroundStatementsCheck.h"
 #include "../readability/FunctionSizeCheck.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "AvoidCStyleCastsCheck.h"
+#include "AvoidNSObjectNewCheck.h"
 #include "AvoidThrowingObjCExceptionCheck.h"
 #include "AvoidUnderscoreInGoogletestNameCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
 #include "FunctionNamingCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "GlobalVariableDeclarationCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<build::UsingNamespaceDirectiveCheck>(
         "google-build-using-namespace");
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "google-default-arguments");
     CheckFactories.registerCheck<ExplicitConstructorCheck>(
         "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>(
         "google-objc-function-naming");
     CheckFactories.registerCheck<objc::GlobalVariableDeclarationCheck>(
         "google-objc-global-variable-declaration");
     CheckFactories.registerCheck<runtime::IntegerTypesCheck>(
         "google-runtime-int");

docs/ReleaseNotes.rst

 
 - 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:`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
   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

docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst

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

docs/clang-tidy/checks/list.rst

.. toctree::
    fuchsia-trailing-return
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    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
    google-readability-avoid-underscore-in-googletest-name
    google-readability-braces-around-statements (redirects to readability-braces-around-statements) <google-readability-braces-around-statements>
    google-readability-casting
    google-readability-function-size (redirects to readability-function-size) <google-readability-function-size>
    google-readability-namespace-comments (redirects to llvm-namespace-comment) <google-readability-namespace-comments>

test/clang-tidy/google-objc-avoid-nsobject-new.m

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