[clang-tidy] Add check: replace string::find(...) == 0 with absl::Sta…

…rtsWith

Patch by Niko Weh!

Reviewers: hokein

Subscribers: klimek, cfe-commits, ioeric, ilya-biryukov, ahedberg

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

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@327111 91177308-0d34-0410-b5e6-96231b3b80d8

clang-tidy/CMakeLists.txt

@@ -27,6 +27,7 @@ add_clang_library(clangTidy
   )
 
 add_subdirectory(android)
+add_subdirectory(abseil)
 add_subdirectory(boost)
 add_subdirectory(bugprone)
 add_subdirectory(cert)

clang-tidy/abseil/AbseilTidyModule.cpp

@@ -0,0 +1,38 @@
+//===------- AbseilTidyModule.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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "StringFindStartswithCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+class AbseilModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<StringFindStartswithCheck>(
+        "abseil-string-find-startswith");
+  }
+};
+
+// Register the AbseilModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<AbseilModule> X("abseil-module",
+                                                    "Add Abseil checks.");
+
+} // namespace abseil
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the AbseilModule.
+volatile int AbseilModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/CMakeLists.txt

@@ -0,0 +1,14 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyAbseilModule
+  AbseilTidyModule.cpp
+  StringFindStartswithCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tidy/abseil/StringFindStartswithCheck.cpp

@@ -0,0 +1,133 @@
+//===--- StringFindStartswithCheck.cc - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "StringFindStartswithCheck.h"
+
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+#include <cassert>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+StringFindStartswithCheck::StringFindStartswithCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StringLikeClasses(utils::options::parseStringList(
+          Options.get("StringLikeClasses", "::std::basic_string"))),
+      IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+          Options.getLocalOrGlobal("IncludeStyle", "llvm"))),
+      AbseilStringsMatchHeader(
+          Options.get("AbseilStringsMatchHeader", "absl/strings/match.h")) {}
+
+void StringFindStartswithCheck::registerMatchers(MatchFinder *Finder) {
+  auto ZeroLiteral = integerLiteral(equals(0));
+  auto StringClassMatcher = cxxRecordDecl(hasAnyName(SmallVector<StringRef, 4>(
+      StringLikeClasses.begin(), StringLikeClasses.end())));
+
+  auto StringFind = cxxMemberCallExpr(
+      // .find()-call on a string...
+      callee(cxxMethodDecl(hasName("find"), ofClass(StringClassMatcher))),
+      // ... with some search expression ...
+      hasArgument(0, expr().bind("needle")),
+      // ... and either "0" as second argument or the default argument (also 0).
+      anyOf(hasArgument(1, ZeroLiteral), hasArgument(1, cxxDefaultArgExpr())));
+
+  Finder->addMatcher(
+      // Match [=!]= with a zero on one side and a string.find on the other.
+      binaryOperator(
+          anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+          hasEitherOperand(ignoringParenImpCasts(ZeroLiteral)),
+          hasEitherOperand(ignoringParenImpCasts(StringFind.bind("findexpr"))))
+          .bind("expr"),
+      this);
+}
+
+void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) {
+  const ASTContext &Context = *Result.Context;
+  const SourceManager &Source = Context.getSourceManager();
+
+  // Extract matching (sub)expressions
+  const auto *ComparisonExpr = Result.Nodes.getNodeAs<BinaryOperator>("expr");
+  assert(ComparisonExpr != nullptr);
+  const auto *Needle = Result.Nodes.getNodeAs<Expr>("needle");
+  assert(Needle != nullptr);
+  const Expr *Haystack = Result.Nodes.getNodeAs<CXXMemberCallExpr>("findexpr")
+                             ->getImplicitObjectArgument();
+  assert(Haystack != nullptr);
+
+  if (ComparisonExpr->getLocStart().isMacroID())
+    return;
+
+  // Get the source code blocks (as characters) for both the string object
+  // and the search expression
+  const StringRef NeedleExprCode = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(Needle->getSourceRange()), Source,
+      Context.getLangOpts());
+  const StringRef HaystackExprCode = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(Haystack->getSourceRange()), Source,
+      Context.getLangOpts());
+
+  // Create the StartsWith string, negating if comparison was "!=".
+  bool Neg = ComparisonExpr->getOpcodeStr() == "!=";
+  StringRef StartswithStr;
+  if (Neg) {
+    StartswithStr = "!absl::StartsWith";
+  } else {
+    StartswithStr = "absl::StartsWith";
+  }
+
+  // Create the warning message and a FixIt hint replacing the original expr.
+  auto Diagnostic =
+      diag(ComparisonExpr->getLocStart(),
+           (StringRef("use ") + StartswithStr + " instead of find() " +
+            ComparisonExpr->getOpcodeStr() + " 0")
+               .str());
+
+  Diagnostic << FixItHint::CreateReplacement(
+      ComparisonExpr->getSourceRange(),
+      (StartswithStr + "(" + HaystackExprCode + ", " + NeedleExprCode + ")")
+          .str());
+
+  // Create a preprocessor #include FixIt hint (CreateIncludeInsertion checks
+  // whether this already exists).
+  auto IncludeHint = IncludeInserter->CreateIncludeInsertion(
+      Source.getFileID(ComparisonExpr->getLocStart()), AbseilStringsMatchHeader,
+      false);
+  if (IncludeHint) {
+    Diagnostic << *IncludeHint;
+  }
+}
+
+void StringFindStartswithCheck::registerPPCallbacks(
+    CompilerInstance &Compiler) {
+  IncludeInserter = llvm::make_unique<clang::tidy::utils::IncludeInserter>(
+      Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle);
+  Compiler.getPreprocessor().addPPCallbacks(
+      IncludeInserter->CreatePPCallbacks());
+}
+
+void StringFindStartswithCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "StringLikeClasses",
+                utils::options::serializeStringList(StringLikeClasses));
+  Options.store(Opts, "IncludeStyle",
+                utils::IncludeSorter::toString(IncludeStyle));
+  Options.store(Opts, "AbseilStringsMatchHeader", AbseilStringsMatchHeader);
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/StringFindStartswithCheck.h

@@ -0,0 +1,48 @@
+//===--- StringFindStartswithCheck.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 LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRING_FIND_STARTSWITH_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRING_FIND_STARTSWITH_H_
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include <memory>
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+// Find string.find(...) == 0 comparisons and suggest replacing with StartsWith.
+// FIXME(niko): Add similar check for EndsWith
+// FIXME(niko): Add equivalent modernize checks for C++20's std::starts_With
+class StringFindStartswithCheck : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+  StringFindStartswithCheck(StringRef Name, ClangTidyContext *Context);
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  std::unique_ptr<clang::tidy::utils::IncludeInserter> IncludeInserter;
+  const std::vector<std::string> StringLikeClasses;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+  const std::string AbseilStringsMatchHeader;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_STRING_FIND_STARTSWITH_H_

clang-tidy/plugin/CMakeLists.txt

@@ -9,6 +9,7 @@ add_clang_library(clangTidyPlugin
   clangSema
   clangTidy
   clangTidyAndroidModule
+  clangTidyAbseilModule
   clangTidyBoostModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule

clang-tidy/tool/CMakeLists.txt

@@ -18,6 +18,7 @@ target_link_libraries(clang-tidy
   clangBasic
   clangTidy
   clangTidyAndroidModule
+  clangTidyAbseilModule
   clangTidyBoostModule
   clangTidyBugproneModule
   clangTidyCERTModule

clang-tidy/tool/ClangTidyMain.cpp

@@ -499,6 +499,11 @@ extern volatile int CERTModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination =
     CERTModuleAnchorSource;
 
+// This anchor is used to force the linker to link the AbseilModule.
+extern volatile int AbseilModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED AbseilModuleAnchorDestination =
+    AbseilModuleAnchorSource;
+
 // This anchor is used to force the linker to link the BoostModule.
 extern volatile int BoostModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination =

docs/ReleaseNotes.rst

@@ -77,6 +77,15 @@ Improvements to clang-tidy
 
   Warns if a class inherits from multiple classes that are not pure virtual.
 
+- New `abseil` module for checks related to the `Abseil <https://abseil.io>`_
+  library.
+
+- New `abseil-string-find-startswith
+  <http://clang.llvm.org/extra/clang-tidy/checks/abseil-string-find-startswith.html>`_ check
+
+  Checks whether a ``std::string::find()`` result is compared with 0, and
+  suggests replacing with ``absl::StartsWith()``.
+
 - New `fuchsia-statically-constructed-objects
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check
 

docs/clang-tidy/checks/abseil-string-find-startswith.rst

@@ -0,0 +1,41 @@
+.. title:: clang-tidy - abseil-string-find-startswith
+
+abseil-string-find-startswith
+=============================
+
+Checks whether a ``std::string::find()`` result is compared with 0, and
+suggests replacing with ``absl::StartsWith()``. This is both a readability and
+performance issue.
+
+.. code-block:: c++
+
+  string s = "...";
+  if (s.find("Hello World") == 0) { /* do something */ }
+
+becomes
+
+
+.. code-block:: c++
+
+  string s = "...";
+  if (absl::StartsWith(s, "Hello World")) { /* do something */ }
+
+
+Options
+-------
+
+.. option:: StringLikeClasses
+
+   Semicolon-separated list of names of string-like classes. By default only
+   ``std::basic_string`` is considered. The list of methods to considered is
+   fixed.
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+.. option:: AbseilStringsMatchHeader
+
+   The location of Abseil's ``strings/match.h``. Defaults to
+   ``absl/strings/match.h``.

docs/clang-tidy/checks/list.rst

@@ -4,6 +4,7 @@ Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat

test/clang-tidy/abseil-string-find-startswith.cpp

@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s abseil-string-find-startswith %t
+
+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 basic_string &);
+  basic_string(const C *, const A &a = A());
+  ~basic_string();
+  int find(basic_string<C> s, int pos = 0);
+  int find(const char *s, int pos = 0);
+};
+typedef basic_string<char> string;
+typedef basic_string<wchar_t> wstring;
+} // namespace std
+
+std::string foo(std::string);
+std::string bar();
+
+#define A_MACRO(x, y) ((x) == (y))
+
+void tests(std::string s) {
+  s.find("a") == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of find() == 0 [abseil-string-find-startswith]
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}}
+
+  s.find(s) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}}
+
+  s.find("aaa") != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}}
+
+  s.find(foo(foo(bar()))) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, foo(foo(bar())));{{$}}
+
+  if (s.find("....") == 0) { /* do something */ }
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}if (absl::StartsWith(s, "....")) { /* do something */ }{{$}}
+
+  0 != s.find("a");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith
+  // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "a");{{$}}
+
+  // expressions that don't trigger the check are here.
+  A_MACRO(s.find("a"), 0);
+  s.find("a", 1) == 0;
+  s.find("a", 1) == 1;
+  s.find("a") == 1;
+}