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

[etienneb]

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
https://codereview.chromium.org/1807753002/
https://codereview.chromium.org/1826193002/
https://codereview.chromium.org/1805713002/

[etienneb]

update comment.

[szdominik]

Hi!

I'm working on the same (or almost the same) checker as you, so maybe you'll find helpful my code.
I think your approach (or heuristics) is better than mine, but there are other options in this problem.
e.g.

• more precise location (where exactly is the missing comma)
• FixItHint for that
• think about what if the explicit array size is given (see below in my test)

Please, ask me if you have any question about my checker.

misc-wrong-string-array-initialization.cpp1 KBDownload

(test file)

WrongStringArrayInitializationCheck.h1 KBDownload

WrongStringArrayInitializationCheck.cpp3 KBDownload

[alexfh]

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

43

An early return would be better here.

46

Should this threshold be an option (or at least a named constant)?

53

const auto *Literal = ...

61

Should this threshold be an option (or at least a named constant)?

64

No capitalization and trailing periods, please.

Please also clang-format this file.

[etienneb]

address alexfh@ comments.

[etienneb]

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

43

moved to an assert.
It should never be false.

[alexfh]

Looks good with a few comments. Further functionality improvements can be done in a follow up, I think.

Please update docs/ReleaseNotes.rst.

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

73

I'd prefer this to be double(Count) / Size > RatioThreshold and RatioThreshold to be a double.

76

We need to add a recommendation on how to silence this warning (use parentheses, for example). Fine for a follow up.

[xazax.hun]

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

76

A possible way to silence this warning would be to implement a heuristic that is available in szdominik's implementation, i.e.: when the size of the array is explicitly given by the user and it matches the length of the initializer list do not warn.

[alexfh]

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

76

The check shouldn't complain in this case, but I wouldn't recommend this as a fix, since it makes the code more strict in an inconvenient way.

[LegalizeAdulthood]

Can you summarize this check in docs/ReleaseNotes.rst in the clang-tidy section, please?

[etienneb]

address alexfh@ comments.

[etienneb]

nits

[etienneb]

ready to land.

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

76

I'll provide the recommendation when it will be implemented. Next patch.

76

in case of a fixed array size, the algorithm could be more aggressive.
Feel free to push a patch for this.

I don't believe it's quite common to set the size for string literals. If you have example for it, please share it.

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
+  SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
+#include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "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>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");

clang-tidy/misc/SuspiciousMissingCommaCheck.h

+//===--- 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

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp

+//===--- 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

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    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
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii

docs/clang-tidy/checks/misc-suspicious-missing-comma.rst

+.. 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",
+  };

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

+// 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"
+};