From 2f6a52816fc33cf24373ce2aaf7df08b95403f44 Mon Sep 17 00:00:00 2001 From: Stephane Moore Date: Sat, 21 Sep 2019 01:22:22 +0000 Subject: [PATCH] =?utf8?q?[clang-tidy]=20Add=20check=20for=20classes=20mis?= =?utf8?q?sing=20-hash=20=E2=9A=A0=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 llvm-svn: 372445 --- clang-tools-extra/clang-tidy/objc/CMakeLists.txt | 1 + .../clang-tidy/objc/MissingHashCheck.cpp | 62 ++++++++++++++++++++ .../clang-tidy/objc/MissingHashCheck.h | 35 +++++++++++ .../clang-tidy/objc/ObjCTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 + .../docs/clang-tidy/checks/objc-missing-hash.rst | 16 +++++ .../test/clang-tidy/objc-missing-hash.m | 68 ++++++++++++++++++++++ 8 files changed, 192 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/objc/MissingHashCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/objc/MissingHashCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/objc-missing-hash.rst create mode 100644 clang-tools-extra/test/clang-tidy/objc-missing-hash.m diff --git a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt index 4eeb148..0a12e4a 100644 --- a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt @@ -4,6 +4,7 @@ add_clang_library(clangTidyObjCModule AvoidNSErrorInitCheck.cpp AvoidSpinlockCheck.cpp ForbiddenSubclassingCheck.cpp + MissingHashCheck.cpp ObjCTidyModule.cpp PropertyDeclarationCheck.cpp SuperSelfCheck.cpp diff --git a/clang-tools-extra/clang-tidy/objc/MissingHashCheck.cpp b/clang-tools-extra/clang-tidy/objc/MissingHashCheck.cpp new file mode 100644 index 0000000..0da5571 --- /dev/null +++ b/clang-tools-extra/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, Base) { + const ObjCInterfaceDecl *InterfaceDecl = Node.getClassInterface(); + return Base.matches(*InterfaceDecl, Finder, Builder); +} + +AST_MATCHER_P(ObjCContainerDecl, hasInstanceMethod, + ast_matchers::internal::Matcher, 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("impl"); + diag(ID->getLocation(), "%0 implements -isEqual: without implementing -hash") + << ID; +} + +} // namespace objc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/objc/MissingHashCheck.h b/clang-tools-extra/clang-tidy/objc/MissingHashCheck.h new file mode 100644 index 0000000..4ac74d5 --- /dev/null +++ b/clang-tools-extra/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-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp b/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp index 636e2c0..c9b57d5 100644 --- a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp +++ b/clang-tools-extra/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 @@ public: "objc-avoid-spinlock"); CheckFactories.registerCheck( "objc-forbidden-subclassing"); + CheckFactories.registerCheck( + "objc-missing-hash"); CheckFactories.registerCheck( "objc-property-declaration"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index fc72a33..d16d8c1 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -91,6 +91,12 @@ Improvements to clang-tidy Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites them to use ``Register`` +- New :doc:`objc-missing-hash + ` check. + + Finds Objective-C implementations that implement ``-isEqual:`` without also + appropriately implementing ``-hash``. + - Improved :doc:`bugprone-posix-return ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index a906a5b..e81b62d 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -325,6 +325,7 @@ Clang-Tidy Checks 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/clang-tools-extra/docs/clang-tidy/checks/objc-missing-hash.rst b/clang-tools-extra/docs/clang-tidy/checks/objc-missing-hash.rst new file mode 100644 index 0000000..ea8f775 --- /dev/null +++ b/clang-tools-extra/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/clang-tools-extra/test/clang-tidy/objc-missing-hash.m b/clang-tools-extra/test/clang-tidy/objc-missing-hash.m new file mode 100644 index 0000000..b9cc9d0 --- /dev/null +++ b/clang-tools-extra/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 + -- 2.7.4