[clang-tidy][objc] Finds and fixes improper usages of XCTAssertEquals and XCTAssertNotEquals.

Using XCTAssertEqual on NSString* objects is almost always  wrong.

Unfortunately, we have seen a lot of tests doing this and reyling on pointer equality for strings with the same values (which happens to work sometimes - depending on the linker, but this assumption is not guaranteed by the language)

These fixes would make tests less brittle.

Differential Revision: https://reviews.llvm.org/D114975

Author: oontvoo

clang-tools-extra/clang-tidy/objc/AssertEquals.cpp

@@ -0,0 +1,65 @@
+//===--- AssertEquals.cpp - 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "AssertEquals.h"
+
+#include <map>
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+// Mapping from `XCTAssert*Equal` to `XCTAssert*EqualObjects` name.
+static const std::map<std::string, std::string> &NameMap() {
+  static std::map<std::string, std::string> map{
+      {"XCTAssertEqual", "XCTAssertEqualObjects"},
+      {"XCTAssertNotEqual", "XCTAssertNotEqualObjects"},
+
+  };
+  return map;
+}
+
+void AssertEquals::registerMatchers(MatchFinder *finder) {
+  for (const auto &pair : NameMap()) {
+    finder->addMatcher(
+        binaryOperator(anyOf(hasOperatorName("!="), hasOperatorName("==")),
+                       isExpandedFromMacro(pair.first),
+                       anyOf(hasLHS(hasType(qualType(
+                                 hasCanonicalType(asString("NSString *"))))),
+                             hasRHS(hasType(qualType(
+                                 hasCanonicalType(asString("NSString *"))))))
+
+                           )
+            .bind(pair.first),
+        this);
+  }
+}
+
+void AssertEquals::check(const ast_matchers::MatchFinder::MatchResult &result) {
+  for (const auto &pair : NameMap()) {
+    if (const auto *root = result.Nodes.getNodeAs<BinaryOperator>(pair.first)) {
+      SourceManager *sm = result.SourceManager;
+      // The macros are nested two levels, so going up twice.
+      auto macro_callsite = sm->getImmediateMacroCallerLoc(
+          sm->getImmediateMacroCallerLoc(root->getBeginLoc()));
+      diag(macro_callsite, "use " + pair.second + " for comparing objects")
+          << FixItHint::CreateReplacement(
+                 clang::CharSourceRange::getCharRange(
+                     macro_callsite,
+                     macro_callsite.getLocWithOffset(pair.first.length())),
+                 pair.second);
+    }
+  }
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/objc/AssertEquals.h

@@ -0,0 +1,39 @@
+//===--- AssertEquals.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 THIRD_PARTY_LLVM_LLVM_PROJECT_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_OBJCASSERTEQUALS_H_
+#define THIRD_PARTY_LLVM_LLVM_PROJECT_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_OBJCASSERTEQUALS_H_
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Warn if XCTAssertEqual() or XCTAssertNotEqual() is used with at least one
+/// operands of type NSString*.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-assert-equals.html
+class AssertEquals final : public ClangTidyCheck {
+public:
+  AssertEquals(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.ObjC;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // THIRD_PARTY_LLVM_LLVM_PROJECT_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_OBJCASSERTEQUALS_H_

clang-tools-extra/clang-tidy/objc/CMakeLists.txt

@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
   )
 
 add_clang_library(clangTidyObjCModule
+  AssertEquals.cpp
   AvoidNSErrorInitCheck.cpp
   DeallocInCategoryCheck.cpp
   ForbiddenSubclassingCheck.cpp

clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp

@@ -9,6 +9,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AssertEquals.h"
 #include "AvoidNSErrorInitCheck.h"
 #include "DeallocInCategoryCheck.h"
 #include "ForbiddenSubclassingCheck.h"
@@ -28,6 +29,8 @@ class ObjCModule : public ClangTidyModule {
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
+    CheckFactories.registerCheck<AssertEquals>("objc-assert-equals");
+
     CheckFactories.registerCheck<DeallocInCategoryCheck>(
         "objc-dealloc-in-category");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -261,6 +261,7 @@ Clang-Tidy Checks
    `mpi-buffer-deref <mpi-buffer-deref.html>`_, "Yes"
    `mpi-type-mismatch <mpi-type-mismatch.html>`_, "Yes"
    `objc-avoid-nserror-init <objc-avoid-nserror-init.html>`_,
+   `objc-assert-equals <objc-assert-equals.html>`_, "Yes"
    `objc-dealloc-in-category <objc-dealloc-in-category.html>`_,
    `objc-forbidden-subclassing <objc-forbidden-subclassing.html>`_,
    `objc-missing-hash <objc-missing-hash.html>`_,

clang-tools-extra/docs/clang-tidy/checks/objc-assert-equals.rst

@@ -0,0 +1,11 @@
+.. title:: clang-tidy - objc-assert-equals
+
+objc-assert-equals
+==================
+
+Finds improper usages of `XCTAssertEqual` and `XCTAssertNotEqual` and replaces
+them with `XCTAssertEqualObjects` or `XCTAssertNotEqualObjects`.
+
+This makes tests less fragile, as many improperly rely on pointer equality for
+strings that have equal values.  This assumption is not guarantted by the
+language.

clang-tools-extra/test/clang-tidy/checkers/Inputs/objc-assert/XCTestAssertions.h

@@ -0,0 +1,30 @@
+#ifndef THIRD_PARTY_LLVM_LLVM_PROJECT_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_OBJC_ASSERT_XCTESTASSERTIONS_H_
+#define THIRD_PARTY_LLVM_LLVM_PROJECT_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_OBJC_ASSERT_XCTESTASSERTIONS_H_
+
+#define _XCTPrimitiveAssertEqual(test, expression1, expressionStr1, \
+                                 expression2, expressionStr2, ...)  \
+  ({                                                                \
+    __typeof__(expression1) expressionValue1 = (expression1);       \
+    __typeof__(expression2) expressionValue2 = (expression2);       \
+    if (expressionValue1 != expressionValue2) {                     \
+    }                                                               \
+  })
+
+#define _XCTPrimitiveAssertEqualObjects(test, expression1, expressionStr1, \
+                                        expression2, expressionStr2, ...)  \
+  ({                                                                       \
+    __typeof__(expression1) expressionValue1 = (expression1);              \
+    __typeof__(expression2) expressionValue2 = (expression2);              \
+    if (expressionValue1 != expressionValue2) {                            \
+    }                                                                      \
+  })
+
+#define XCTAssertEqual(expression1, expression2, ...)                     \
+  _XCTPrimitiveAssertEqual(nil, expression1, @ #expression1, expression2, \
+                           @ #expression2, __VA_ARGS__)
+
+#define XCTAssertEqualObjects(expression1, expression2, ...)        \
+  _XCTPrimitiveAssertEqualObjects(nil, expression1, @ #expression1, \
+                                  expression2, @ #expression2, __VA_ARGS__)
+
+#endif // THIRD_PARTY_LLVM_LLVM_PROJECT_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_OBJC_ASSERT_XCTESTASSERTIONS_H_

clang-tools-extra/test/clang-tidy/checkers/objc-assert-equals.m

@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s objc-assert-equals %t -- -- -I %S/Inputs/objc-assert
+#include "XCTestAssertions.h"
+// Can't reference NSString directly so we use this getStr() instead.
+__typeof(@"abc") getStr() {
+  return @"abc";
+}
+void foo() {
+  XCTAssertEqual(getStr(), @"abc");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use XCTAssertEqualObjects for comparing objects
+  // CHECK-FIXES: XCTAssertEqualObjects(getStr(), @"abc");
+  XCTAssertEqual(@"abc", @"abc");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use XCTAssertEqualObjects for comparing objects
+  // CHECK-FIXES: XCTAssertEqualObjects(@"abc", @"abc");
+  XCTAssertEqual(@"abc", getStr());
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use XCTAssertEqualObjects for comparing objects
+  // CHECK-FIXES: XCTAssertEqualObjects(@"abc", getStr());
+  XCTAssertEqual(getStr(), getStr());
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use XCTAssertEqualObjects for comparing objects
+  // CHECK-FIXES: XCTAssertEqualObjects(getStr(), getStr());
+  // Primitive types should be ok
+  XCTAssertEqual(123, 123);
+  XCTAssertEqual(123.0, 123.45);
+  // FIXME: This is the case where we don't diagnose properly.
+  // XCTAssertEqual(@"abc" != @"abc", @"xyz" != @"xyz")
+}