[clang-tidy] Add new check readability-trailing-comma (llvm#173669)

clang-format has a couple of similar options:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#enumtrailingcomma
- add trailing commas for enum

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#inserttrailingcommas
- add trailing commas for C++
but generally they are marked with such warning:

> Warning
>
> Setting this option to any value other than Leave could lead to
incorrect code formatting due to clang-format’s lack of complete
semantic information. As such, extra care should be taken to review code
changes made by this option.

clang-tidy on the other hand has all semantic information, thus can
(hopefully) provide 0 false-positives.

Note that we have already overlapping checks in clang-format/clang-tidy
like:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#insertbraces vs
https://clang.llvm.org/extra/clang-tidy/checks/readability/braces-around-statements.html

Author: vbvictor

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

@@ -58,6 +58,7 @@ add_clang_library(clangTidyReadabilityModule STATIC
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
   SuspiciousCallArgumentCheck.cpp
+  TrailingCommaCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
   UppercaseLiteralSuffixCheck.cpp
   UseAnyOfAllOfCheck.cpp

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

@@ -60,6 +60,7 @@
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
+#include "TrailingCommaCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
@@ -177,6 +178,8 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-simplify-boolean-expr");
     CheckFactories.registerCheck<SuspiciousCallArgumentCheck>(
         "readability-suspicious-call-argument");
+    CheckFactories.registerCheck<TrailingCommaCheck>(
+        "readability-trailing-comma");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(

clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp

@@ -0,0 +1,179 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TrailingCommaCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy {
+
+template <>
+struct OptionEnumMapping<readability::TrailingCommaCheck::CommaPolicyKind> {
+  static llvm::ArrayRef<
+      std::pair<readability::TrailingCommaCheck::CommaPolicyKind, StringRef>>
+  getEnumMapping() {
+    static constexpr std::pair<readability::TrailingCommaCheck::CommaPolicyKind,
+                               StringRef>
+        Mapping[] = {
+            {readability::TrailingCommaCheck::CommaPolicyKind::Append,
+             "Append"},
+            {readability::TrailingCommaCheck::CommaPolicyKind::Remove,
+             "Remove"},
+            {readability::TrailingCommaCheck::CommaPolicyKind::Ignore,
+             "Ignore"},
+        };
+    return {Mapping};
+  }
+};
+
+} // namespace clang::tidy
+
+namespace clang::tidy::readability {
+
+static bool isSingleLine(SourceRange Range, const SourceManager &SM) {
+  return SM.getExpansionLineNumber(Range.getBegin()) ==
+         SM.getExpansionLineNumber(Range.getEnd());
+}
+
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isMacro,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(EnumDecl,
+                                                        InitListExpr)) {
+  return Node.getBeginLoc().isMacroID() || Node.getEndLoc().isMacroID();
+}
+
+AST_MATCHER(EnumDecl, isEmptyEnum) { return Node.enumerators().empty(); }
+
+AST_MATCHER(InitListExpr, isEmptyInitList) { return Node.getNumInits() == 0; }
+
+} // namespace
+
+TrailingCommaCheck::TrailingCommaCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      SingleLineCommaPolicy(
+          Options.get("SingleLineCommaPolicy", CommaPolicyKind::Remove)),
+      MultiLineCommaPolicy(
+          Options.get("MultiLineCommaPolicy", CommaPolicyKind::Append)) {
+  if (SingleLineCommaPolicy == CommaPolicyKind::Ignore &&
+      MultiLineCommaPolicy == CommaPolicyKind::Ignore)
+    configurationDiag("The check '%0' will not perform any analysis because "
+                      "'SingleLineCommaPolicy' and 'MultiLineCommaPolicy' are "
+                      "both set to 'Ignore'.")
+        << Name;
+}
+
+void TrailingCommaCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "SingleLineCommaPolicy", SingleLineCommaPolicy);
+  Options.store(Opts, "MultiLineCommaPolicy", MultiLineCommaPolicy);
+}
+
+void TrailingCommaCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      enumDecl(isDefinition(), unless(isEmptyEnum()), unless(isMacro()))
+          .bind("enum"),
+      this);
+
+  Finder->addMatcher(initListExpr(unless(isEmptyInitList()), unless(isMacro()))
+                         .bind("initlist"),
+                     this);
+}
+
+void TrailingCommaCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("enum"))
+    checkEnumDecl(Enum, Result);
+  else if (const auto *InitList =
+               Result.Nodes.getNodeAs<InitListExpr>("initlist"))
+    checkInitListExpr(InitList, Result);
+  else
+    llvm_unreachable("No matches found");
+}
+
+void TrailingCommaCheck::checkEnumDecl(const EnumDecl *Enum,
+                                       const MatchFinder::MatchResult &Result) {
+  const bool IsSingleLine = isSingleLine(
+      {Enum->getBeginLoc(), Enum->getEndLoc()}, *Result.SourceManager);
+  const CommaPolicyKind Policy =
+      IsSingleLine ? SingleLineCommaPolicy : MultiLineCommaPolicy;
+
+  if (Policy == CommaPolicyKind::Ignore)
+    return;
+
+  const std::optional<Token> LastTok =
+      Lexer::findPreviousToken(Enum->getBraceRange().getEnd(),
+                               *Result.SourceManager, getLangOpts(), false);
+  if (!LastTok)
+    return;
+
+  emitDiag(LastTok->getLocation(), LastTok, DiagKind::Enum, Result, Policy);
+}
+
+void TrailingCommaCheck::checkInitListExpr(
+    const InitListExpr *InitList, const MatchFinder::MatchResult &Result) {
+  // We need to use non-empty syntactic form for correct source locations.
+  if (const InitListExpr *SynInitInitList = InitList->getSyntacticForm();
+      SynInitInitList && SynInitInitList->getNumInits() > 0)
+    InitList = SynInitInitList;
+
+  const bool IsSingleLine = isSingleLine(
+      {InitList->getBeginLoc(), InitList->getEndLoc()}, *Result.SourceManager);
+  const CommaPolicyKind Policy =
+      IsSingleLine ? SingleLineCommaPolicy : MultiLineCommaPolicy;
+
+  if (Policy == CommaPolicyKind::Ignore)
+    return;
+
+  const Expr *LastInit = InitList->inits().back();
+  assert(LastInit);
+
+  // Skip pack expansions - they already have special syntax with '...'
+  if (isa<PackExpansionExpr>(LastInit))
+    return;
+
+  const std::optional<Token> NextTok =
+      utils::lexer::findNextTokenSkippingComments(
+          LastInit->getEndLoc(), *Result.SourceManager, getLangOpts());
+
+  // If the next token is neither a comma nor closing brace, there might be
+  // a macro (e.g., #define COMMA ,) that we can't safely analyze.
+  if (NextTok && !NextTok->isOneOf(tok::comma, tok::r_brace))
+    return;
+
+  emitDiag(LastInit->getEndLoc(), NextTok, DiagKind::InitList, Result, Policy);
+}
+
+void TrailingCommaCheck::emitDiag(
+    SourceLocation LastLoc, std::optional<Token> Token, DiagKind Kind,
+    const ast_matchers::MatchFinder::MatchResult &Result,
+    CommaPolicyKind Policy) {
+  if (LastLoc.isInvalid() || !Token)
+    return;
+
+  const bool HasTrailingComma = Token->is(tok::comma);
+  if (Policy == CommaPolicyKind::Append && !HasTrailingComma) {
+    const SourceLocation InsertLoc = Lexer::getLocForEndOfToken(
+        LastLoc, 0, *Result.SourceManager, getLangOpts());
+    diag(InsertLoc, "%select{initializer list|enum}0 should have "
+                    "a trailing comma")
+        << Kind << FixItHint::CreateInsertion(InsertLoc, ",");
+  } else if (Policy == CommaPolicyKind::Remove && HasTrailingComma) {
+    const SourceLocation CommaLoc = Token->getLocation();
+    if (CommaLoc.isInvalid())
+      return;
+    diag(CommaLoc, "%select{initializer list|enum}0 should not have "
+                   "a trailing comma")
+        << Kind << FixItHint::CreateRemoval(CommaLoc);
+  }
+}
+
+} // namespace clang::tidy::readability

clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.h

@@ -0,0 +1,55 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TRAILINGCOMMACHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TRAILINGCOMMACHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Checks for presence or absence of trailing commas in enum definitions
+/// and initializer lists.
+///
+/// For the user-facing documentation see:
+/// https://clang.llvm.org/extra/clang-tidy/checks/readability/trailing-comma.html
+class TrailingCommaCheck : public ClangTidyCheck {
+public:
+  TrailingCommaCheck(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;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus || LangOpts.C99;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+
+  enum class CommaPolicyKind { Append, Remove, Ignore };
+
+private:
+  const CommaPolicyKind SingleLineCommaPolicy;
+  const CommaPolicyKind MultiLineCommaPolicy;
+
+  void checkEnumDecl(const EnumDecl *Enum,
+                     const ast_matchers::MatchFinder::MatchResult &Result);
+  void checkInitListExpr(const InitListExpr *InitList,
+                         const ast_matchers::MatchFinder::MatchResult &Result);
+
+  // Values correspond to %select{initializer list|enum}0 indices
+  enum DiagKind { InitList = 0, Enum = 1 };
+  void emitDiag(SourceLocation LastLoc, std::optional<Token> Token,
+                DiagKind Kind,
+                const ast_matchers::MatchFinder::MatchResult &Result,
+                CommaPolicyKind Policy);
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_TRAILINGCOMMACHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -128,6 +128,12 @@ New checks
   Finds and removes redundant conversions from ``std::[w|u8|u16|u32]string_view`` to
   ``std::[...]string`` in call expressions expecting ``std::[...]string_view``.
 
+- New :doc:`readability-trailing-comma
+  <clang-tidy/checks/readability/trailing-comma>` check.
+
+  Checks for presence or absence of trailing commas in enum definitions and
+  initializer lists.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -429,6 +429,7 @@ Clang-Tidy Checks
    :doc:`readability-static-definition-in-anonymous-namespace <readability/static-definition-in-anonymous-namespace>`, "Yes"
    :doc:`readability-string-compare <readability/string-compare>`, "Yes"
    :doc:`readability-suspicious-call-argument <readability/suspicious-call-argument>`,
+   :doc:`readability-trailing-comma <readability/trailing-comma>`, "Yes"
    :doc:`readability-uniqueptr-delete-release <readability/uniqueptr-delete-release>`, "Yes"
    :doc:`readability-uppercase-literal-suffix <readability/uppercase-literal-suffix>`, "Yes"
    :doc:`readability-use-anyofallof <readability/use-anyofallof>`,

clang-tools-extra/docs/clang-tidy/checks/readability/trailing-comma.rst

@@ -0,0 +1,68 @@
+.. title:: clang-tidy - readability-trailing-comma
+
+readability-trailing-comma
+==========================
+
+Checks for presence or absence of trailing commas in enum definitions and
+initializer lists.
+
+The check supports separate policies for single-line and multi-line constructs,
+allowing different styles for each. By default, the check enforces trailing
+commas in multi-line constructs and removes them from single-line constructs.
+
+Trailing commas in multi-line constructs offer several benefits:
+
+- Adding or removing elements at the end only changes a single line, making
+  diffs smaller and easier to read.
+- Formatters may change code to a more desired style.
+- Code generators avoid the need for special handling of the last element.
+
+.. code-block:: c++
+
+  // Without trailing commas - adding "Yellow" requires modifying the "Blue" line
+  enum Color {
+    Red,
+    Green,
+    Blue
+  };
+
+  // With trailing commas - adding "Yellow" is a clean, single-line change
+  enum Color {
+    Red,
+    Green,
+    Blue,
+  };
+
+
+Limitations
+-----------
+
+The check currently doesn't analyze code inside macros.
+
+
+Options
+-------
+
+.. option:: SingleLineCommaPolicy
+
+  Controls whether to add, remove, or ignore trailing commas in single-line
+  enum definitions and initializer lists.
+  Valid values are:
+
+  - `Append`: Add trailing commas where missing.
+  - `Remove`: Remove trailing commas where present.
+  - `Ignore`: Do not check single-line constructs.
+
+  Default is `Remove`.
+
+.. option:: MultiLineCommaPolicy
+
+  Controls whether to add, remove, or ignore trailing commas in multi-line
+  enum definitions and initializer lists.
+  Valid values are:
+
+  - `Append`: Add trailing commas where missing.
+  - `Remove`: Remove trailing commas where present.
+  - `Ignore`: Do not check multi-line constructs.
+
+  Default is `Append`.