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

[oontvoo]

Summary

Using XCTAssertEqual on NSString* objects are 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.

[oontvoo]

updated diff

[oontvoo]

doc

[oontvoo]

Thanks!

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

+//===--- 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:
+  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/AssertEquals.cpp

+//===--- 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 "llvm/ADT/DenseMap.h"
+
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+// Mapping from `XCTAssert*Equal` to `XCTAssert*EqualObjects` name.
+static const llvm::DenseMap<std::string, std::string> &NameMap() {
+  static llvm::DenseMap<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
 No newline at end of file

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

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyObjCModule
+  AssertEquals.cpp
   AvoidNSErrorInitCheck.cpp
   DeallocInCategoryCheck.cpp
   ForbiddenSubclassingCheck.cpp
   MissingHashCheck.cpp
   NSInvocationArgumentLifetimeCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
   SuperSelfCheck.cpp

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

 //===--- ObjCTidyModule.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 "AssertEquals.h"
 #include "AvoidNSErrorInitCheck.h"
 #include "DeallocInCategoryCheck.h"
 #include "ForbiddenSubclassingCheck.h"
 #include "MissingHashCheck.h"
 #include "NSInvocationArgumentLifetimeCheck.h"
 #include "PropertyDeclarationCheck.h"
 #include "SuperSelfCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   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>(
         "objc-forbidden-subclassing");
     CheckFactories.registerCheck<MissingHashCheck>(
         "objc-missing-hash");
     CheckFactories.registerCheck<NSInvocationArgumentLifetimeCheck>(
         "objc-nsinvocation-argument-lifetime");

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

.. csv-table::
    `bugprone-signal-handler <bugprone-signal-handler.html>`_,
    `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone-sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone-sizeof-expression.html>`_,
    `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,
    `bugprone-string-constructor <bugprone-string-constructor.html>`_, "Yes"
    `bugprone-string-integer-assignment <bugprone-string-integer-assignment.html>`_, "Yes"
    `bugprone-string-literal-with-embedded-nul <bugprone-string-literal-with-embedded-nul.html>`_,
-   `bugprone-stringview-nullptr <bugprone-stringview-nullptr.html>`_, "Yes"
+   `bugprone-stringview-nullptr <bugprone-stringview-nullptr.html>`_, "Yes"   
    `bugprone-suspicious-enum-usage <bugprone-suspicious-enum-usage.html>`_,
    `bugprone-suspicious-include <bugprone-suspicious-include.html>`_,
    `bugprone-suspicious-memory-comparison <bugprone-suspicious-memory-comparison.html>`_,
    `bugprone-suspicious-memset-usage <bugprone-suspicious-memset-usage.html>`_, "Yes"
    `bugprone-suspicious-missing-comma <bugprone-suspicious-missing-comma.html>`_,
    `bugprone-suspicious-semicolon <bugprone-suspicious-semicolon.html>`_, "Yes"
    `bugprone-suspicious-string-compare <bugprone-suspicious-string-compare.html>`_, "Yes"
    `bugprone-swapped-arguments <bugprone-swapped-arguments.html>`_, "Yes"
.. csv-table::
    `modernize-use-override <modernize-use-override.html>`_, "Yes"
    `modernize-use-trailing-return-type <modernize-use-trailing-return-type.html>`_, "Yes"
    `modernize-use-transparent-functors <modernize-use-transparent-functors.html>`_, "Yes"
    `modernize-use-uncaught-exceptions <modernize-use-uncaught-exceptions.html>`_, "Yes"
    `modernize-use-using <modernize-use-using.html>`_, "Yes"
    `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>`_,
    `objc-nsinvocation-argument-lifetime <objc-nsinvocation-argument-lifetime.html>`_, "Yes"
    `objc-property-declaration <objc-property-declaration.html>`_, "Yes"
    `objc-super-self <objc-super-self.html>`_, "Yes"
    `openmp-exception-escape <openmp-exception-escape.html>`_,
    `openmp-use-default-none <openmp-use-default-none.html>`_,

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

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

+// RUN: %check_clang_tidy %s custom-objc-assert-equals %t -- -- -I %S/input/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")
+}