[clang-tidy] Add a new checker to detect missing comma in initializer list.

Summary:
This checker is able to detect missing comma in
an array of string literals.

```
  const char* A[] = {
    "abc",
    "def"   // missing comma (no compiler warnings)
    "ghi",
  };
```

The ratio of false-positive is reduced by restricting the
size of the array considered and the ratio of missing
comma.

To validate the quantity of false positive, the checker
was tried over LLVM and chromium code and detected these
cases:

[[ http://reviews.llvm.org/D18454 | http://reviews.llvm.org/D18454 ]]
[[https://codereview.chromium.org/1807753002/ | https://codereview.chromium.org/1807753002/]]
[[https://codereview.chromium.org/1826193002/ | https://codereview.chromium.org/1826193002/]]
[[https://codereview.chromium.org/1805713002/ | https://codereview.chromium.org/1805713002/]]

Reviewers: alexfh

Subscribers: LegalizeAdulthood, szdominik, xazax.hun, cfe-commits

Differential Revision: http://reviews.llvm.org/D18457

llvm-svn: 265033

diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 1c20095..fdfa26e 100644 (file)
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -23,6 +23,7 @@ add_clang_library(clangTidyMiscModule
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
+  SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp

diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index 2da30b7..7dc8a21 100644 (file)
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -31,6 +31,7 @@
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
+#include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
@@ -88,6 +89,8 @@ public:
         "misc-static-assert");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "misc-string-integer-assignment");
+    CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
+        "misc-suspicious-missing-comma");
     CheckFactories.registerCheck<SuspiciousSemicolonCheck>(
         "misc-suspicious-semicolon");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(

diff --git a/clang-tools-extra/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp b/clang-tools-extra/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
new file mode 100644 (file)
index 0000000..a3a479c
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
@@ -0,0 +1,80 @@
+//===--- SuspiciousMissingCommaCheck.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 "SuspiciousMissingCommaCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(StringLiteral, isConcatenatedLiteral) {
+  return Node.getNumConcatenated() > 1;
+}
+
+}  // namespace
+
+SuspiciousMissingCommaCheck::SuspiciousMissingCommaCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      SizeThreshold(Options.get("SizeThreshold", 5U)),
+      RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))) {}
+
+void SuspiciousMissingCommaCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "SizeThreshold", SizeThreshold);
+  Options.store(Opts, "RatioThreshold", std::to_string(RatioThreshold));
+}
+
+void SuspiciousMissingCommaCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConcatenatedStringLiteral =
+      stringLiteral(isConcatenatedLiteral()).bind("str");
+
+  const auto StringsInitializerList =
+      initListExpr(hasType(constantArrayType()),
+                   has(expr(ignoringImpCasts(ConcatenatedStringLiteral))));
+
+  Finder->addMatcher(StringsInitializerList.bind("list"), this);
+}
+
+void SuspiciousMissingCommaCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *InitializerList = Result.Nodes.getNodeAs<InitListExpr>("list");
+  const auto *ConcatenatedLiteral = Result.Nodes.getNodeAs<Expr>("str");
+  assert(InitializerList && ConcatenatedLiteral);
+
+  // Skip small arrays as they often generate false-positive.
+  unsigned int Size = InitializerList->getNumInits();
+  if (Size < SizeThreshold) return;
+
+  // Count the number of occurence of concatenated string literal.
+  unsigned int Count = 0;
+  for (unsigned int i = 0; i < Size; ++i) {
+    const Expr *Child = InitializerList->getInit(i)->IgnoreImpCasts();
+    if (const auto *Literal = dyn_cast<StringLiteral>(Child)) {
+      if (Literal->getNumConcatenated() > 1) ++Count;
+    }
+  }
+
+  // Warn only when concatenation is not common in this initializer list.
+  // The current threshold is set to less than 1/5 of the string literals.
+  if (double(Count) / Size > RatioThreshold) return;
+
+  diag(ConcatenatedLiteral->getLocStart(),
+       "suspicious string literal, probably missing a comma");
+}
+
+}  // namespace misc
+}  // namespace tidy
+}  // namespace clang

diff --git a/clang-tools-extra/clang-tidy/misc/SuspiciousMissingCommaCheck.h b/clang-tools-extra/clang-tidy/misc/SuspiciousMissingCommaCheck.h
new file mode 100644 (file)
index 0000000..209d7fb
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/SuspiciousMissingCommaCheck.h
@@ -0,0 +1,41 @@
+//===--- SuspiciousMissingCommaCheck.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_MISC_SUSPICIOUS_MISSING_COMMA_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_MISSING_COMMA_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check finds string literals which are probably concatenated accidentally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-suspicious-missing-comma.html
+class SuspiciousMissingCommaCheck : public ClangTidyCheck {
+public:
+  SuspiciousMissingCommaCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;      
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  // Minimal size of a string literals array to be considered by the checker.
+  const unsigned SizeThreshold;
+  // Maximal threshold ratio of suspicious string literals to be considered.
+  const double RatioThreshold;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_MISSING_COMMA_H

diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 912b04a..c80ea50 100644 (file)
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -66,6 +66,7 @@ Clang-Tidy Checks
    misc-sizeof-container
    misc-static-assert
    misc-string-integer-assignment
+   misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference

diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-suspicious-missing-comma.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
new file mode 100644 (file)
index 0000000..e46566a
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - misc-suspicious-missing-comma
+
+misc-suspicious-missing-comma
+=============================
+
+String literals placed side-by-side are concatenated at translation phase 6
+(after the preprocessor). This feature is used to represent long string
+literal on multiple lines.
+
+For instance, these declarations are equivalent:
+  const char* A[] = "This is a test";
+  const char* B[] = "This" " is a "
+                    "test";
+
+A common mistake done by programmers is to forget a comma between two string
+literals in an array initializer list.
+
+  const char* Test[] = {
+    "line 1",
+    "line 2"     // Missing comma!
+    "line 3",
+    "line 4",
+    "line 5"
+  };
+
+The array contains the string "line 2line3" at offset 1 (i.e. Test[1]). Clang
+won't generate warnings at compile time.
+
+This checker may warn incorrectly on cases like:
+
+  const char* SupportedFormat[] = {
+    "Error %s",
+    "Code " PRIu64,   // May warn here.
+    "Warning %s",
+  };

diff --git a/clang-tools-extra/test/clang-tidy/misc-suspicious-missing-comma.cpp b/clang-tools-extra/test/clang-tidy/misc-suspicious-missing-comma.cpp
new file mode 100644 (file)
index 0000000..7b64fab
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/misc-suspicious-missing-comma.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s misc-suspicious-missing-comma %t
+
+const char* Cartoons[] = {
+  "Bugs Bunny",
+  "Homer Simpson",
+  "Mickey Mouse",
+  "Bart Simpson",
+  "Charlie Brown"  // There is a missing comma here.
+  "Fred Flintstone",
+  "Popeye",
+};
+// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: suspicious string literal, probably missing a comma [misc-suspicious-missing-comma]
+
+const wchar_t* Colors[] = {
+  L"Red", L"Yellow", L"Blue", L"Green", L"Purple", L"Rose", L"White", L"Black"
+};
+
+// The following array should not trigger any warnings.
+const char* HttpCommands[] = {
+  "GET / HTTP/1.0\r\n"
+  "\r\n",
+
+  "GET /index.html HTTP/1.0\r\n"
+  "\r\n",
+
+  "GET /favicon.ico HTTP/1.0\r\n"
+  "header: dummy"
+  "\r\n",
+};
+
+// This array is too small to trigger a warning.
+const char* SmallArray[] = {
+  "a" "b", "c"
+};