diff options
8 files changed, 175 insertions, 0 deletions
diff --git a/clang-tools-extra/clang-tidy/objc/AssertEquals.cpp b/clang-tools-extra/clang-tidy/objc/AssertEquals.cpp new file mode 100644 index 0000000..198e7f0 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/clang-tidy/objc/AssertEquals.h b/clang-tools-extra/clang-tidy/objc/AssertEquals.h new file mode 100644 index 0000000..94389fe --- /dev/null +++ b/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_ diff --git a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt b/clang-tools-extra/clang-tidy/objc/CMakeLists.txt index 9a553f5a..1129c6a 100644 --- a/clang-tools-extra/clang-tidy/objc/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp b/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp index 56ab0ab..3a5bf7d 100644 --- a/clang-tools-extra/clang-tidy/objc/ObjCTidyModule.cpp +++ b/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 @@ 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>( diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 3dbba16..1e6936f 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/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>`_, diff --git a/clang-tools-extra/docs/clang-tidy/checks/objc-assert-equals.rst b/clang-tools-extra/docs/clang-tidy/checks/objc-assert-equals.rst new file mode 100644 index 0000000..b7a3e76 --- /dev/null +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/objc-assert/XCTestAssertions.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/objc-assert/XCTestAssertions.h new file mode 100644 index 0000000..c2e3fba --- /dev/null +++ b/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_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/objc-assert-equals.m b/clang-tools-extra/test/clang-tidy/checkers/objc-assert-equals.m new file mode 100644 index 0000000..e79d083 --- /dev/null +++ b/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") +} |