[clang-tidy] Enhance misc-suspicious-string-compare by matching string.compare

[etienneb]

Summary

This patch added the support for method "compare" on string-like classes.

The LLVM stringRef is supported. The checker assume that StringRef is returning -1, 0 or 1.
Which is not the case for other functions returning <0, 0 or >0.

[alexfh]

I'm somewhat reluctant to add LLVM-specific checks to misc-. I would prefer either to split the LLVM-specific part to a separate check in llvm/ (which might derive from this check or somehow reuse the logic) or make the check configurable enough so that these checks can be enabled for LLVM in the config file.

clang-tidy/misc/SuspiciousStringCompareCheck.cpp

119

This is inconsistent with the code above. I'd also use ofClass(hasName("::llvm::StringRef")), hasAnyName("compare", ...).

[etienneb]

address alexfh comments, add a macro FP.

[etienneb]

In D19577#413526, @alexfh wrote:

I'm somewhat reluctant to add LLVM-specific checks to misc-. I would prefer either to split the LLVM-specific part to a separate check in llvm/ (which might derive from this check or somehow reuse the logic) or make the check configurable enough so that these checks can be enabled for LLVM in the config file.

How should we solve adding other code base to checkers.
It will be common to have these code base: chromium, llvm, boost, stl.

Should we use the local/global to configure them?

[Eugene.Zelenko]

Please add check options descriptions to documentation.

[alexfh]

In D19577#413888, @etienneb wrote:

In D19577#413526, @alexfh wrote:

I'm somewhat reluctant to add LLVM-specific checks to misc-. I would prefer either to split the LLVM-specific part to a separate check in llvm/ (which might derive from this check or somehow reuse the logic) or make the check configurable enough so that these checks can be enabled for LLVM in the config file.

How should we solve adding other code base to checkers.
It will be common to have these code base: chromium, llvm, boost, stl.

Should we use the local/global to configure them?

(we might have discussed this offline, but I'm not sure now, so posting it here)

I don't think that checks should know about specifics of different code bases, since this doesn't scale well. One of alternative approaches should be used instead, depending on the amount and complexity of the project-specific parts:

1. Provide enough configuration options to support known and unknown code bases (within certain limits). If needed, register checks with a project-specific alias and corresponding option values. Alternatively, specify options in the project's .clang-tidy file. Example: google-readability-namespace-comments check.
2. Make the check extensible via inheritance. Example: llvm-header-guard check.
3. Pull out some utility functions/matchers/classes/... and make a separate check for each code base.

One more thing to consider is that it's usually better to keep the overlap in patterns detected by the different variants of the check minimal. E.g. a check detecting a certain problematic pattern applied to LLVM containers should not complain about the same pattern applied to STL containers to avoid duplicate warnings in case both checks are turned on (consider a code base that uses LLVM, boost and STL, and needs three variants of some check).

clang-tidy/misc/SuspiciousStringCompareCheck.cpp

 //===--- SuspiciousStringCompareCheck.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 "SuspiciousStringCompareCheck.h"
+#include "../utils/Matchers.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
-#include "../utils/Matchers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 static const char KnownStringCompareFunctions[] = "__builtin_memcmp;"
const auto ComparisonOperator =
       expr(anyOf(ComparisonUnaryOperator, ComparisonBinaryOperator));
 
   // Add the list of known string compare-like functions and add user-defined
   // functions.
   std::vector<std::string> FunctionNames;
   ParseFunctionNames(KnownStringCompareFunctions, &FunctionNames);
   ParseFunctionNames(StringCompareLikeFunctions, &FunctionNames);
 
-  // Match a call to a string compare functions.
+  // Match a std::string::compare call.
+  const auto StdStringCompareCallExpr =
+      cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(hasName("basic_string")),
+                                             hasName("compare"))
+                                   .bind("decl")))
+          .bind("call");
+
+  // Match llvm strings variants.
+  const auto LLVMStringCompareCallExpr =
+      cxxMemberCallExpr(
+          callee(cxxMethodDecl(
+                     ofClass(hasName("::llvm::StringRef")),
+                     hasAnyName("compare", "compare_lower", "compare_numeric"))
+                     .bind("decl")))
+          .bind("call");
+
+  // Match a call to a string compare functions (i.e. strcmp).
   const auto FunctionCompareDecl =
       functionDecl(hasAnyName(std::vector<StringRef>(FunctionNames.begin(),
                                                      FunctionNames.end())))
           .bind("decl");
   const auto DirectStringCompareCallExpr =
       callExpr(hasDeclaration(FunctionCompareDecl)).bind("call");
-  const auto MacroStringCompareCallExpr =
-      conditionalOperator(
-        anyOf(hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)),
-              hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr))));
+
+  // Match common macros used to wrapped runtime functions.
+  const auto CondMacroStringCompareCallExpr = conditionalOperator(anyOf(
+      hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)),
+  const auto CommaMacroStringCompareCallExpr = binaryOperator(
+      hasOperatorName(","),
+      hasRHS(ignoringParenImpCasts(DirectStringCompareCallExpr)));
+
   // The implicit cast is not present in C.
   const auto StringCompareCallExpr = ignoringParenImpCasts(
-        anyOf(DirectStringCompareCallExpr, MacroStringCompareCallExpr));
+      anyOf(DirectStringCompareCallExpr, CondMacroStringCompareCallExpr,
+            CommaMacroStringCompareCallExpr, StdStringCompareCallExpr,
+            LLVMStringCompareCallExpr));
 
   if (WarnOnImplicitComparison) {
     // Detect suspicious calls to string compare:
     //     'if (strcmp())'  ->  'if (strcmp() != 0)'
     Finder->addMatcher(
         stmt(anyOf(ifStmt(hasCondition(StringCompareCallExpr)),
                    whileStmt(hasCondition(StringCompareCallExpr)),
                    doStmt(hasCondition(StringCompareCallExpr)),
void SuspiciousStringCompareCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       implicitCastExpr(unless(hasType(isInteger())),
                        hasSourceExpression(StringCompareCallExpr))
           .bind("invalid-conversion"),
       this);
 
   // Detect suspicious operator with string compare function as operand.
   Finder->addMatcher(
-      binaryOperator(
-          unless(anyOf(matchers::isComparisonOperator(), hasOperatorName("&&"),
-                       hasOperatorName("||"), hasOperatorName("="))),
+      binaryOperator(unless(anyOf(matchers::isComparisonOperator(),
+                                  hasOperatorName("&&"), hasOperatorName("||"),
+                                  hasOperatorName("="), hasOperatorName(","))),
                      hasEitherOperand(StringCompareCallExpr))
           .bind("suspicious-operator"),
       this);
 
-  // Detect comparison to invalid constant: 'strcmp() == -1'.
+  // Detect comparison to invalid constant: 'strcmp() == -2'.
   const auto InvalidLiteral = ignoringParenImpCasts(
       anyOf(integerLiteral(unless(equals(0))),
             unaryOperator(hasOperatorName("-"),
                           has(integerLiteral(unless(equals(0))))),
             characterLiteral(), cxxBoolLiteral()));
 
+  // A subset of functions allow comparison to 1 and -1.
+  const auto ValidLiteral = ignoringParenImpCasts(anyOf(
+      integerLiteral(equals(1)),
+      unaryOperator(hasOperatorName("-"), has(integerLiteral(equals(1))))));
+
+  const auto ValidComparator =
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     hasEitherOperand(LLVMStringCompareCallExpr),
+                     hasEitherOperand(ValidLiteral));
+
   Finder->addMatcher(binaryOperator(matchers::isComparisonOperator(),
                                     hasEitherOperand(StringCompareCallExpr),
-                                    hasEitherOperand(InvalidLiteral))
+                                    hasEitherOperand(InvalidLiteral),
+                                    unless(ValidComparator))
                          .bind("invalid-comparison"),
                      this);
 }
 
 void SuspiciousStringCompareCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *Decl = Result.Nodes.getNodeAs<FunctionDecl>("decl");
   const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
void SuspiciousStringCompareCheck::check(
   }
 
   if (Result.Nodes.getNodeAs<Stmt>("invalid-comparison")) {
     diag(Call->getLocStart(),
          "function %0 is compared to a suspicious constant")
         << Decl;
   }
 
-  if (const auto* BinOp = Result.Nodes.getNodeAs<BinaryOperator>("suspicious-operator")) {
+  if (const auto *BinOp =
+          Result.Nodes.getNodeAs<BinaryOperator>("suspicious-operator")) {
     diag(Call->getLocStart(), "results of function %0 used by operator '%1'")
         << Decl << BinOp->getOpcodeStr();
   }
 
   if (Result.Nodes.getNodeAs<Stmt>("invalid-conversion")) {
     diag(Call->getLocStart(), "function %0 has suspicious implicit cast")
         << Decl;
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

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

 // RUN: %check_clang_tidy %s misc-suspicious-string-compare %t -- \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: misc-suspicious-string-compare.WarnOnImplicitComparison, value: 1}, \
 // RUN:   {key: misc-suspicious-string-compare.WarnOnLogicalNotComparison, value: 1}]}' \
 // RUN: --
 
 typedef __SIZE_TYPE__ size;
 
+namespace std {
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T, typename A>
+struct basic_string {
+  typedef basic_string<C, T, A> _Type;
+  basic_string();
+  basic_string(const C *p, const A &a = A());
+  int compare(const C* s) const;
+};
+
+typedef basic_string<char, std::char_traits<char>, std::allocator<char> > string;
+}
+
+namespace llvm {
+struct StringRef {
+  StringRef();
+  StringRef(const char*);
+  int compare(StringRef RHS);
+  int compare_lower(StringRef RHS);
+  int compare_numeric(StringRef RHS);
+};
+}
+
 struct locale_t {
   void* dummy;
 } locale;
 
 static const char A[] = "abc";
 static const unsigned char U[] = "abc";
 static const unsigned char V[] = "xyz";
 static const wchar_t W[] = L"abc";
int strncmp_macro(const char* a, const char* b) {
   // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' has suspicious implicit cast
 
   if (macro_strncmp(a, b, 4) + 0)
     return 0;
   // CHECK-MESSAGES: [[@LINE-2]]:7: warning: results of function 'strcmp' used by operator '+'
 
   return 1;
 }
+
+#define uprv_checkValidMemory(a, b) ((void)(a))
+#define uprv_strncmp(s1, s2, n) ( \
+    uprv_checkValidMemory(s1, 1), \
+    uprv_checkValidMemory(s2, 1), \
+    strncmp(s1, s2, n))
+
+int strncmp_macro_uprv(const char* a, const char* b) {
+  if (uprv_strncmp(a, b, 4))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' is called without explicitly comparing result
+
+  if (uprv_strncmp(a, b, 4) == 2)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' is compared to a suspicious constant
+
+  if (uprv_strncmp(a, b, 4) <= .0)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' has suspicious implicit cast
+
+  if (uprv_strncmp(a, b, 4) + 0)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: results of function 'strncmp' used by operator '+'
+
+  return 1;
+}
+
+int test_string_patterns() {
+  std::string str;
+  if (str.compare("a"))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+  // CHECK-FIXES: if (str.compare("a") != 0)
+
+  if (str.compare("a") == 0 ||
+      str.compare("b"))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+  // CHECK-FIXES: str.compare("b") != 0)
+
+  if (str.compare("a") == 1)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant
+
+  if (str.compare("a") == 2)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant
+}
+
+int test_llvm_patterns() {
+  llvm::StringRef str;
+  if (str.compare("a"))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+  // CHECK-FIXES: if (str.compare("a") != 0)
+
+  if (str.compare_lower("a"))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare_lower' is called without explicitly comparing result
+  // CHECK-FIXES: if (str.compare_lower("a") != 0)   
+
+  if (str.compare("a") == 0 ||
+      str.compare("b"))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result
+  // CHECK-FIXES: str.compare("b") != 0)
+ 
+  if (str.compare("a") == 2)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant
+
+  // The following calls are valid.
+  if (str.compare("a") == 1) return 0;
+  if (str.compare("a") != 1) return 0;
+  if (str.compare("a") == 0) return 0;
+  if (str.compare("a") != 0) return 0;
+  if (str.compare("a") == -1) return 0;
+  if (str.compare("a") != -1) return 0;
+}