mwyman updated this revision to Diff 198702.
mwyman marked 2 inline comments as done.
mwyman added a comment.
Update for review comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61350/new/
https://reviews.llvm.org/D61350
Files:
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h
clang-tools-extra/clang-tidy/google/CMakeLists.txt
clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
Index: clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
@@ -0,0 +1,79 @@
+// 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
+@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
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
@@ -133,6 +133,7 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
===================================================================
--- /dev/null
+++ 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>`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
in the Time domain instead of the numeric domain.
+- 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:`google-readability-avoid-underscore-in-googletest-name
<clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
check.
Index: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
+++ 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 @@
"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>(
Index: clang-tools-extra/clang-tidy/google/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/google/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/google/CMakeLists.txt
@@ -2,6 +2,7 @@
add_clang_library(clangTidyGoogleModule
AvoidCStyleCastsCheck.cpp
+ AvoidNSObjectNewCheck.cpp
AvoidThrowingObjCExceptionCheck.cpp
AvoidUnderscoreInGoogletestNameCheck.cpp
DefaultArgumentsCheck.cpp
Index: clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h
===================================================================
--- /dev/null
+++ 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 {
+
+/// The 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
Index: clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
===================================================================
--- /dev/null
+++ 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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
