[clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[madsravn]

Summary

Adding a new check to clang-tidy. It checks if std::string.compare(std::string) is being used to check for equality or inequality instead of the intended purpose (which is sorting).

It is based on bug 28022 (https://llvm.org/bugs/show_bug.cgi?id=28022). I imagine that this check can be extended to contain a check for that as well.

[JDevlieghere]

You might also want to add a few tests cases, for instance the ones you used as examples in the documentation.

clang-tidy/misc/MiscStringCompareCheck.cpp

27 ↗

(On Diff #79552)

Do you need to bind to "str2" and "mem"?

[madsravn]

In D27210#607749, @JDevlieghere wrote:

You might also want to add a few tests cases, for instance the ones you used as examples in the documentation.

I will do that.

I added the two extra bindings for possible future use. I will remove them.

[malcolm.parsons]

I think you could add fixits for this.

clang-tidy/misc/MiscStringCompareCheck.cpp

48 ↗

(On Diff #79552)

Doesn't test RHS.

54 ↗

(On Diff #79552)

Doesn't test RHS.
Could be combined with the previous case.

clang-tidy/misc/MiscStringCompareCheck.h

24 ↗

(On Diff #79552)

Remove Misc.

Did you use add_new_check.py to add this check?

docs/clang-tidy/checks/misc-string-compare.rst

5

Too many =.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[Eugene.Zelenko]

docs/clang-tidy/checks/misc-string-compare.rst

9

Please enclose compare in ``. Probably will be good idea to add (). Same below.

11

Please enclose -1, 0 and 1 in `.

[madsravn]

Updated the patch to include changes suggested by comments by JDevlieghere. Other fixes will follow.

[malcolm.parsons]

Please run clang-format on all new files.

test/clang-tidy/misc-string-compare.cpp

4

clang-tidy tests don't #include system headers.
Declare the bits you need instead.
See test/clang-tidy/misc-string-constructor.cpp for an example.

10

Some other test ideas:

if (str1.compare("foo")) {}

return str1.compare(str2) == 0;

func(str1.compare(str2) != 0);

if (str2.empty() || str1.compare(str2) != 0) {}

[madsravn]

Updated per comments

[madsravn]

Diff reflecting changes suggested by comments.

[malcolm.parsons]

clang-tidy/misc/MiscStringCompareCheck.cpp

48 ↗

(On Diff #79552)

I mean: doesn't test whether RHS is zero.

[madsravn]

Added integerLiteral on both sides of the == operator.

[malcolm.parsons]

I don't know why you're restricting this check to only match within the condition of an if statement.

[madsravn]

Trimmed down the ast matcher a little.

[alexfh]

clang-tidy/misc/StringCompareCheck.cpp

30

Please add trailing periods here and elsewhere.

37–51

These two matchers can be merged to avoid repetition.

56–58

It looks like it's relatively straightforward to implement fixit hints. WDYT?

test/clang-tidy/misc-string-compare.cpp

30

Truncate all CHECK patterns after the first one after of strings;

[malcolm.parsons]

docs/clang-tidy/checks/misc-string-compare.rst

22

Please clang-format this code.

test/clang-tidy/misc-string-compare.cpp

16

Missing &s.
Please clang-format this file.

[madsravn]

Updated according to comments. Still missing fixit.

[madsravn]

Fixed broken tests.

[xazax.hun]

docs/clang-tidy/checks/misc-string-compare.rst

11

As far as I remember this is not true. The `compare` method can return any integer number, only the sign is defined. It is not guaranteed to return -1 or 1 in case of inequality.

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

26

This needs to check that it's one of the single parameter overloads of compare.

28

I don't think you need to check what the first argument is.

40

Is this clang-formatted?

test/clang-tidy/misc-string-compare.cpp

10

There's still no test for the single parameter c-string overload.

[madsravn]

Did as comments suggested: Fixed the description about compare returning -1, 0 or 1. Fixed the ast matcher to only find compare with one argument. Clang-formatted everything. Added a new test (str.compare("foo")) and wrote a FIXME for the fixit.

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

26

Add parameterCountIs(1).

53

Just fix the easy cases?

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

26

Actually, the argumentCountIs(1) below should be sufficient.

[alexfh]

clang-tidy/misc/StringCompareCheck.cpp

49

compare should be enclosed in single quotes ('compare'), since it refers to a method.

docs/clang-tidy/checks/misc-string-compare.rst

13

The documentation doesn't explain why is using compare for equality comparison should be avoided. Maybe that is recommended to avoid the risk of incorrect interpretation of the return value and simplify the code?

[malcolm.parsons]

docs/clang-tidy/checks/misc-string-compare.rst

13

Also, a string equality check can be faster than compare as it can exit early when the strings aren't the same length.

[madsravn]

Updated matcher.

Made fixits for some cases

Updated the documentation.

[malcolm.parsons]

Do the fixits work when the object is a pointer to a string?
pstr1->compare(str2) == 0
Does the warning work when the argument is a pointer to a string?
str1.compare(*pstr2) == 0
Does the warning work when the argument is the result of a function call?
Does the warning work when the object is the result of a function call?

[madsravn]

Updated the matcher to find suggested occurences.

[Prazek]

Good job.
I think it is resonable to warn in cases like:

if (str.compare(str2) == 1)

or even

if(str.compare(str2) == -1)

Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember corectly PVS studio found some bugs like this.

clang-tidy/misc/StringCompareCheck.cpp

26

Start with upper case

28

+1, just check if you are calling function with 1 argument.
you can still use hasArgument(0, expr().bind("str2")) to bind argument

[madsravn]

Small changes made by suggestions. strCompare is now with uppercase: StrCompare

Checking for str.compare(str) == {-1,1} as well.

[Prazek]

Do you need some help with implementing the other fixit, or you just need some extra time? It seems to be almost the same as your second fixit

clang-tidy/misc/StringCompareCheck.cpp

70–71

Take this warning to some static const global variable

72

match1 and match2 are in different matchers (passed to register matchers)?

If so put return here after diag to finish control flow for this case.

82

and use it here

clang-tidy/misc/StringCompareCheck.h

11–12

This should be much longer string. Do you know about ./add_new_check?

Please make one similar to other checks

37

DITTO

test/clang-tidy/misc-string-compare.cpp

36–41

Why this one doesn't have fixit hint?

71

no fixit?

[madsravn]

Updated according to comments.

I have decided to keep the fixit for match1 a FIXME.

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

26

misc-suspicious-string-compare warns about suspicious strcmp(); maybe it should handle string::compare() too.

clang-tidy/misc/StringCompareCheck.h

24

Change filename to misc-string-compare.html

docs/ReleaseNotes.rst

81

typo. ineqaulity -> inequality

[madsravn]

clang-tidy/misc/MiscStringCompareCheck.h

24 ↗

(On Diff #79552)

No, but the files I was looking at had the same naming convention. Maybe something has changed in that regards recently?

I will fix it.

clang-tidy/misc/StringCompareCheck.cpp

26

Do you suggest that I move this check to misc-suspicious-string-compare? Or should we just remove it from here?

docs/clang-tidy/checks/misc-string-compare.rst

11

This is true. I checked it - it is just some implementations which tend to use -1, 0 and 1. However, the specification says negative, 0 and positive. I will correct it. Thanks

test/clang-tidy/misc-string-compare.cpp

10

None of those fit the ast match.

I think it's fine as it is now. If the matcher will be expanded to check for some of those cases, I think more test cases are needed.

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

26

Remove from here and add to misc-suspicious-string-compare in another patch.

[madsravn]

Reviews based on comments. Removed check for suspicious string compare.

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

26

This message is not used.

38

Do you really care what the callee expression is?
Use isArrow() on the MemberExpr to check if it's a pointer.

68

All variables should start with a capital letter.

[madsravn]

clang-tidy/misc/StringCompareCheck.cpp

38

How else would I get str1? Using the below snippet, I only get str1.compare and str1->compare instead of str1.
Given a MemberExpr (str1.compare) is there an easy way to extract str1?

const auto StrCompare = cxxMemberCallExpr(
      callee(cxxMethodDecl(hasName("compare"),
                           ofClass(classTemplateSpecializationDecl(
                               hasName("::std::basic_string"))))),
      hasArgument(0, expr().bind("str2")), argumentCountIs(1),
      callee(memberExpr().bind("str1")))

[malcolm.parsons]

clang-tidy/misc/StringCompareCheck.cpp

38

getBase()

[madsravn]

Changes based on comments.
Shortened the ast matcher.

[malcolm.parsons]

LGTM!

[alexfh]

A late comment here: the check seems to suit better readability module instead of misc. Could you move it there?

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MultipleStatementMacroCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   RedundantExpressionCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp
+  StringCompareCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
+#include "StringCompareCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
         "misc-redundant-expression");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "misc-sizeof-expression");
     CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
+    CheckFactories.registerCheck<StringCompareCheck>("misc-string-compare");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "misc-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "misc-string-integer-assignment");
     CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
         "misc-string-literal-with-embedded-nul");
     CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
         "misc-suspicious-missing-comma");

clang-tidy/misc/StringCompareCheck.h

+//===--- StringCompareCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef STRING_COMPARE_CHECK_H
+#define STRING_COMPARE_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check flags all calls compare when used to check for string
+/// equality or inequality.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html
+class StringCompareCheck : public ClangTidyCheck {
+public:
+  StringCompareCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // STRING_COMPARE_CHECK_H

clang-tidy/misc/StringCompareCheck.cpp

+//===--- MiscStringCompare.cpp - clang-tidy--------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "StringCompareCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void StringCompareCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  const auto strCompare = cxxMemberCallExpr(
+      callee(cxxMethodDecl(hasName("compare"),
+                           ofClass(classTemplateSpecializationDecl(
+                               hasName("::std::basic_string"))))),
+      hasArgument(0, declRefExpr()), callee(memberExpr()));
+
+  // First and second case: cast str.compare(str) to boolean.
+  Finder->addMatcher(
+      implicitCastExpr(
+                 hasImplicitDestinationType(booleanType()), has(strCompare))
+          .bind("match"),
+      this);
+
+  // Third and fourth case: str.compare(str) == 0 and str.compare(str) != 0.
+  Finder->addMatcher(
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                                         hasEitherOperand(strCompare),
+                                         hasEitherOperand(integerLiteral(equals(0))))
+          .bind("match"),
+      this);
+}
+
+void StringCompareCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Matched = Result.Nodes.getNodeAs<Stmt>("match"))
+    diag(Matched->getLocStart(),
+         "do not use compare to test equality of strings; "
+         "use the string equality operator instead");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 - New `misc-move-forwarding-reference
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check
 
   Warns when ``std::move`` is applied to a forwarding reference instead of
   ``std::forward``.
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare.html>`_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 - New `misc-use-after-move
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html>`_ check
 
   Warns if an object is used after it has been moved, without an intervening
   reinitialization.
 
 - `modernize-make-unique
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html>`_

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-multiple-statement-macro
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-redundant-expression
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert
+   misc-string-compare
    misc-string-constructor
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-suspicious-string-compare
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference

docs/clang-tidy/checks/misc-string-compare.rst

+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===================
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (!str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (str1.compare(str2) == 0) {
+  }
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2) != 0) {
+  }
+
+  // use str1 == str2 instead.
+  if (0 == str1.compare(str2)) {
+  }
+
+  // use str1 != str2 instead.
+  if (0 != str1.compare(str2)) {
+  }
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.

test/clang-tidy/misc-string-compare.cpp

+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string<char> &str);
+  bool empty();
+};
+bool operator==(const basic_string<char> &lhs, const basic_string<char> &rhs);
+bool operator!=(const basic_string<char> &lhs, const basic_string<char> &rhs);
+typedef basic_string<char> string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use compare to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use compare to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}