[clang-tidy] Add check for classes missing -hash ⚠️
Summary:
Apple documentation states that:
"If two objects are equal, they must have the same hash value. This last
point is particularly important if you define isEqual: in a subclass and
intend to put instances of that subclass into a collection. Make sure
you also define hash in your subclass."
https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418795-isequal?language=objc
In many or all versions of libobjc, -[NSObject isEqual:] is a pointer
equality check and -[NSObject hash] returns the messaged object's
pointer. A relatively common form of developer error is for a developer to
override -isEqual: in a subclass without overriding -hash to ensure that
hashes are equal for objects that are equal.
It is assumed that an override of -isEqual: is a strong signal for
changing the object's equality operator to something other than pointer
equality which implies that a missing override of -hash could result in
distinct objects being equal but having distinct hashes because they are
independent instances. This added check flags classes that override
-isEqual: but inherit NSObject's implementation of -hash to warn of the
potential for unexpected behavior.
The proper implementation of -hash is the responsibility of the
developer and the check will only verify that the developer made an
effort to properly implement -hash. Developers can set up unit tests
to verify that their implementation of -hash is appropriate.
Test Notes:
Ran check-clang-tools.
Reviewers: aaron.ballman, benhamilton
Reviewed By: aaron.ballman
Subscribers: Eugene.Zelenko, mgorny, xazax.hun, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D67737
git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@372445 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/clang-tidy/objc/CMakeLists.txt b/clang-tidy/objc/CMakeLists.txt
index 4eeb148..0a12e4a 100644
--- a/clang-tidy/objc/CMakeLists.txt
+++ b/clang-tidy/objc/CMakeLists.txt
@@ -4,6 +4,7 @@
AvoidNSErrorInitCheck.cpp
AvoidSpinlockCheck.cpp
ForbiddenSubclassingCheck.cpp
+ MissingHashCheck.cpp
ObjCTidyModule.cpp
PropertyDeclarationCheck.cpp
SuperSelfCheck.cpp
diff --git a/clang-tidy/objc/MissingHashCheck.cpp b/clang-tidy/objc/MissingHashCheck.cpp
new file mode 100644
index 0000000..0da5571
--- /dev/null
+++ b/clang-tidy/objc/MissingHashCheck.cpp
@@ -0,0 +1,62 @@
+//===--- MissingHashCheck.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 "MissingHashCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+namespace {
+
+AST_MATCHER_P(ObjCImplementationDecl, hasInterface,
+ ast_matchers::internal::Matcher<ObjCInterfaceDecl>, Base) {
+ const ObjCInterfaceDecl *InterfaceDecl = Node.getClassInterface();
+ return Base.matches(*InterfaceDecl, Finder, Builder);
+}
+
+AST_MATCHER_P(ObjCContainerDecl, hasInstanceMethod,
+ ast_matchers::internal::Matcher<ObjCMethodDecl>, Base) {
+ // Check each instance method against the provided matcher.
+ for (const auto *I : Node.instance_methods()) {
+ if (Base.matches(*I, Finder, Builder))
+ return true;
+ }
+ return false;
+}
+
+} // namespace
+
+void MissingHashCheck::registerMatchers(MatchFinder *Finder) {
+ // This check should only be applied to Objective-C sources.
+ if (!getLangOpts().ObjC)
+ return;
+
+ Finder->addMatcher(
+ objcMethodDecl(
+ hasName("isEqual:"), isInstanceMethod(),
+ hasDeclContext(objcImplementationDecl(
+ hasInterface(isDirectlyDerivedFrom("NSObject")),
+ unless(hasInstanceMethod(hasName("hash"))))
+ .bind("impl"))),
+ this);
+}
+
+void MissingHashCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *ID = Result.Nodes.getNodeAs<ObjCImplementationDecl>("impl");
+ diag(ID->getLocation(), "%0 implements -isEqual: without implementing -hash")
+ << ID;
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tidy/objc/MissingHashCheck.h b/clang-tidy/objc/MissingHashCheck.h
new file mode 100644
index 0000000..4ac74d5
--- /dev/null
+++ b/clang-tidy/objc/MissingHashCheck.h
@@ -0,0 +1,35 @@
+//===--- MissingHashCheck.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_OBJC_MISSINGHASHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_MISSINGHASHCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C implementations that implement -isEqual: without also
+/// appropriately implementing -hash.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-missing-hash.html
+class MissingHashCheck : public ClangTidyCheck {
+public:
+ MissingHashCheck(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_MISSINGHASHCHECK_H
diff --git a/clang-tidy/objc/ObjCTidyModule.cpp b/clang-tidy/objc/ObjCTidyModule.cpp
index 636e2c0..c9b57d5 100644
--- a/clang-tidy/objc/ObjCTidyModule.cpp
+++ b/clang-tidy/objc/ObjCTidyModule.cpp
@@ -12,6 +12,7 @@
#include "AvoidNSErrorInitCheck.h"
#include "AvoidSpinlockCheck.h"
#include "ForbiddenSubclassingCheck.h"
+#include "MissingHashCheck.h"
#include "PropertyDeclarationCheck.h"
#include "SuperSelfCheck.h"
@@ -30,6 +31,8 @@
"objc-avoid-spinlock");
CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
"objc-forbidden-subclassing");
+ CheckFactories.registerCheck<MissingHashCheck>(
+ "objc-missing-hash");
CheckFactories.registerCheck<PropertyDeclarationCheck>(
"objc-property-declaration");
CheckFactories.registerCheck<SuperSelfCheck>(
diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index fc72a33..d16d8c1 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -91,6 +91,12 @@
Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
them to use ``Register``
+- New :doc:`objc-missing-hash
+ <clang-tidy/checks/objc-missing-hash>` check.
+
+ Finds Objective-C implementations that implement ``-isEqual:`` without also
+ appropriately implementing ``-hash``.
+
- Improved :doc:`bugprone-posix-return
<clang-tidy/checks/bugprone-posix-return>` check.
diff --git a/docs/clang-tidy/checks/list.rst b/docs/clang-tidy/checks/list.rst
index a906a5b..e81b62d 100644
--- a/docs/clang-tidy/checks/list.rst
+++ b/docs/clang-tidy/checks/list.rst
@@ -325,6 +325,7 @@
objc-avoid-nserror-init
objc-avoid-spinlock
objc-forbidden-subclassing
+ objc-missing-hash
objc-property-declaration
objc-super-self
openmp-exception-escape
diff --git a/docs/clang-tidy/checks/objc-missing-hash.rst b/docs/clang-tidy/checks/objc-missing-hash.rst
new file mode 100644
index 0000000..ea8f775
--- /dev/null
+++ b/docs/clang-tidy/checks/objc-missing-hash.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - objc-missing-hash
+
+objc-missing-hash
+=================
+
+Finds Objective-C implementations that implement ``-isEqual:`` without also
+appropriately implementing ``-hash``.
+
+Apple documentation highlights that objects that are equal must have the same
+hash value:
+https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418795-isequal?language=objc
+
+Note that the check only verifies the presence of ``-hash`` in scenarios where
+its omission could result in unexpected behavior. The verification of the
+implementation of ``-hash`` is the responsibility of the developer, e.g.,
+through the addition of unit tests to verify the implementation.
diff --git a/test/clang-tidy/objc-missing-hash.m b/test/clang-tidy/objc-missing-hash.m
new file mode 100644
index 0000000..b9cc9d0
--- /dev/null
+++ b/test/clang-tidy/objc-missing-hash.m
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s objc-missing-hash %t
+
+typedef _Bool BOOL;
+#define YES 1
+#define NO 0
+typedef unsigned int NSUInteger;
+typedef void *id;
+
+@interface NSObject
+- (NSUInteger)hash;
+- (BOOL)isEqual:(id)object;
+@end
+
+@interface MissingHash : NSObject
+@end
+
+@implementation MissingHash
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 'MissingHash' implements -isEqual: without implementing -hash [objc-missing-hash]
+
+- (BOOL)isEqual:(id)object {
+ return YES;
+}
+
+@end
+
+@interface HasHash : NSObject
+@end
+
+@implementation HasHash
+
+- (NSUInteger)hash {
+ return 0;
+}
+
+- (BOOL)isEqual:(id)object {
+ return YES;
+}
+
+@end
+
+@interface NSArray : NSObject
+@end
+
+@interface MayHaveInheritedHash : NSArray
+@end
+
+@implementation MayHaveInheritedHash
+
+- (BOOL)isEqual:(id)object {
+ return YES;
+}
+
+@end
+
+@interface AnotherRootClass
+@end
+
+@interface NotDerivedFromNSObject : AnotherRootClass
+@end
+
+@implementation NotDerivedFromNSObject
+
+- (BOOL)isEqual:(id)object {
+ return NO;
+}
+
+@end
+
