[clang-tidy] Reduce false-positive ratio in misc-suspicious-missing-comma check.

[etienneb]

Summary

This patch is adding detection of common string literal patterns
that should not trigger warnings.

• Add a limit on the number of concatenated token,
• Add support for parenthese sequence of tokens,
• Add detection of valid indentation.

As an example, this code will no longer trigger a warning:

const char* Array[] = {
  "first literal"
    "indented literal"
    "indented literal",
  "second literal",
  [...]

[etienneb]

Simplification and nits.

[alexfh]

Looks good!

[etienneb]

thanks alex, I'll give a day or two to people to jump in.
If no other comments, I'll land it.

clang-tidy/misc/SuspiciousMissingCommaCheck.h

public:
   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;
+  // Maximal number of concatenated tokens.
+  const unsigned MaxConcatenatedTokens;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_MISSING_COMMA_H

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 
+bool isConcatenatedLiteralsOnPurpose(ASTContext* Ctx,
+                                     const StringLiteral* Lit) {
+  // String literals surrounded by parentheses are assumed to be on purpose.
+  //    i.e.:  const char* Array[] = { ("a" "b" "c"), "d", [...] };
+  auto Parents = Ctx->getParents(*Lit);
+  if (Parents.size() == 1 && Parents[0].get<ParenExpr>() != nullptr)
+    return true;
+
+  // Appropriately indented string literals are assumed to be on purpose.
+  // The following frequent indentation is accepted:
+  //     const char* Array[] = {
+  //       "first literal"
+  //           "indented literal"
+  //           "indented literal",
+  //       "second literal",
+  //       [...]
+  //     };
+  const SourceManager& SM = Ctx->getSourceManager();
+  bool IndentedCorrectly = true;
+  SourceLocation FirstToken = Lit->getStrTokenLoc(0);
+  FileID BaseFID = SM.getFileID(FirstToken);
+  unsigned int BaseIndent = SM.getSpellingColumnNumber(FirstToken);
+  unsigned int BaseLine = SM.getSpellingLineNumber(FirstToken);
+  for (unsigned int TokNum = 1; TokNum < Lit->getNumConcatenated(); ++ TokNum) {
+    SourceLocation Token = Lit->getStrTokenLoc(TokNum);
+    FileID FID = SM.getFileID(Token);
+    unsigned int Indent = SM.getSpellingColumnNumber(Token);
+    unsigned int Line = SM.getSpellingLineNumber(Token);
+    if (FID != BaseFID || Line != BaseLine + TokNum || Indent <= BaseIndent) {
+      IndentedCorrectly = false;
+      break;
+    }
+  }
+  if (IndentedCorrectly)
+    return true;
+
+  // There is no pattern recognized by the checker, assume it's not on purpose.
+  return false;
+}
+
 AST_MATCHER(StringLiteral, isConcatenatedLiteral) {
-  return Node.getNumConcatenated() > 1;
+  return Node.getNumConcatenated() > 1 &&
+         !isConcatenatedLiteralsOnPurpose(&Finder->getASTContext(), &Node);
 }
 
 }  // namespace
 
 SuspiciousMissingCommaCheck::SuspiciousMissingCommaCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       SizeThreshold(Options.get("SizeThreshold", 5U)),
-      RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))) {}
+      RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))),
+      MaxConcatenatedTokens(Options.get("MaxConcatenatedTokens", 5U)) {}
 
 void SuspiciousMissingCommaCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "SizeThreshold", SizeThreshold);
   Options.store(Opts, "RatioThreshold", std::to_string(RatioThreshold));
+  Options.store(Opts, "MaxConcatenatedTokens", MaxConcatenatedTokens);
 }
 
 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");
+  const auto *ConcatenatedLiteral =
+      Result.Nodes.getNodeAs<StringLiteral>("str");
   assert(InitializerList && ConcatenatedLiteral);
   
+  // Do not warn on string literals with too many tokens concatenated.
+  if (ConcatenatedLiteral->getNumConcatenated() >= MaxConcatenatedTokens)
+    return;
+
+  // Do not warn on string literals if they are concatenated on purpose.
+  if (isConcatenatedLiteralsOnPurpose(Result.Context, ConcatenatedLiteral))
+    return;
+
   // 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();

test/clang-tidy/misc-suspicious-missing-comma.cpp

const char* Cartoons[] = {
   "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.
+// The following array should not trigger any warnings. There is more than 5
+// elements, but they are all concatenated string literals.
 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",
+
+  "GET /index.html-en HTTP/1.0\r\n"
+  "\r\n",
+
+  "GET /index.html-fr HTTP/1.0\r\n"
+  "\r\n",
+
+  "GET /index.html-es HTTP/1.0\r\n"
+  "\r\n",
 };
 
 // This array is too small to trigger a warning.
 const char* SmallArray[] = {
   "a" "b", "c"
 };
+
+// Parentheses should be enough to avoid warnings.
+const char* ParentheseArray[] = {
+  ("a" "b"), "c",
+  ("d"
+   "e"
+   "f"),
+  "g", "h", "i", "j", "k", "l"
+};
+
+// Indentation should be enough to avoid warnings.
+const char* CorrectlyIndentedArray[] = {
+  "This is a long message "
+      "which is spanning over multiple lines."
+      "And this should be fine.",
+  "a", "b", "c", "d", "e", "f",
+  "g", "h", "i", "j", "k", "l"
+};
+
+const char* IncorrectlyIndentedArray[] = {
+  "This is a long message "
+  "which is spanning over multiple lines."
+      "And this should be fine.",
+  "a", "b", "c", "d", "e", "f",
+  "g", "h", "i", "j", "k", "l"
+};
+// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: suspicious string literal, probably missing a comma [misc-suspicious-missing-comma]
+
+const char* ManyConcatenatedTokensArray[] = {
+  "Dummy line",
+  "Dummy line",
+  "a" "b" "c" "d" "e" "f",
+  "g" "h" "i" "j" "k" "l",
+  "Dummy line",
+  "Dummy line",
+  "Dummy line",
+  "Dummy line",
+};