[clang] a324fcf - clang-format: insert trailing commas into containers.
Martin Probst via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 29 04:26:18 PST 2020
Author: Martin Probst
Date: 2020-01-29T13:23:54+01:00
New Revision: a324fcf1ae62d065b957e66a9d2f5c18b6259d27
URL: https://github.com/llvm/llvm-project/commit/a324fcf1ae62d065b957e66a9d2f5c18b6259d27
DIFF: https://github.com/llvm/llvm-project/commit/a324fcf1ae62d065b957e66a9d2f5c18b6259d27.diff
LOG: clang-format: insert trailing commas into containers.
Summary:
This change adds an option to insert trailing commas into container
literals. For example, in JavaScript:
const x = [
a,
b,
^~~~~ inserted if missing.
]
This is implemented as a seperate post-processing pass after formatting
(because formatting might change whether the container literal does or
does not wrap). This keeps the code relatively simple and orthogonal,
though it has the notable drawback that the newly inserted comma is not
taken into account for formatting decisions (e.g. it might exceed the 80
char limit). To avoid exceeding the ColumnLimit, a comma is only
inserted if it fits into the limit.
Trailing comma insertion conceptually conflicts with argument
bin-packing: inserting a comma disables bin-packing, so we cannot do
both. clang-format rejects FormatStyle configurations that do both with
this change.
Reviewers: krasimir, MyDeveloperDay
Subscribers: cfe-commits
Tags: #clang
Added:
Modified:
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/FormatTestJS.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 09a0556ab4c6..3891029a6f3a 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -35,7 +35,12 @@ class DiagnosticConsumer;
namespace format {
-enum class ParseError { Success = 0, Error, Unsuitable };
+enum class ParseError {
+ Success = 0,
+ Error,
+ Unsuitable,
+ BinPackTrailingCommaConflict
+};
class ParseErrorCategory final : public std::error_category {
public:
const char *name() const noexcept override;
@@ -544,6 +549,20 @@ struct FormatStyle {
/// \endcode
bool BinPackArguments;
+ /// The style of inserting trailing commas into container literals.
+ enum TrailingCommaStyle {
+ /// Do not insert trailing commas.
+ TCS_None,
+ /// Insert trailing commas in container literals that were wrapped over
+ /// multiple lines. Note that this is conceptually incompatible with
+ /// bin-packing, because the trailing comma is used as an indicator
+ /// that a container should be formatted one-per-line (i.e. not bin-packed).
+ /// So inserting a trailing comma counteracts bin-packing.
+ TCS_Wrapped,
+ };
+
+ TrailingCommaStyle InsertTrailingCommas;
+
/// If ``false``, a function declaration's or function definition's
/// parameters will either all be on the same line or will have one line each.
/// \code
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index dd131a93362c..6f585ae915a5 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -157,6 +157,13 @@ template <> struct ScalarEnumerationTraits<FormatStyle::BinPackStyle> {
}
};
+template <> struct ScalarEnumerationTraits<FormatStyle::TrailingCommaStyle> {
+ static void enumeration(IO &IO, FormatStyle::TrailingCommaStyle &Value) {
+ IO.enumCase(Value, "None", FormatStyle::TCS_None);
+ IO.enumCase(Value, "Wrapped", FormatStyle::TCS_Wrapped);
+ }
+};
+
template <> struct ScalarEnumerationTraits<FormatStyle::BinaryOperatorStyle> {
static void enumeration(IO &IO, FormatStyle::BinaryOperatorStyle &Value) {
IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -486,6 +493,7 @@ template <> struct MappingTraits<FormatStyle> {
IO.mapOptional("IndentWidth", Style.IndentWidth);
IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+ IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
@@ -644,6 +652,8 @@ std::string ParseErrorCategory::message(int EV) const {
return "Invalid argument";
case ParseError::Unsuitable:
return "Unsuitable";
+ case ParseError::BinPackTrailingCommaConflict:
+ return "trailing comma insertion cannot be used with bin packing";
}
llvm_unreachable("unexpected parse error");
}
@@ -788,6 +798,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
LLVMStyle.IndentWrappedFunctionNames = false;
LLVMStyle.IndentWidth = 2;
+ LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
LLVMStyle.JavaScriptWrapImports = true;
LLVMStyle.TabWidth = 8;
@@ -946,6 +957,9 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
// taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
// commonly followed by overlong URLs.
GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+ // TODO: enable once decided, in particular re disabling bin packing.
+ // https://google.github.io/styleguide/jsguide.html#features-arrays-trailing-comma
+ // GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
GoogleStyle.MaxEmptyLinesToKeep = 3;
GoogleStyle.NamespaceIndentation = FormatStyle::NI_All;
GoogleStyle.SpacesInContainerLiterals = false;
@@ -1211,6 +1225,11 @@ std::error_code parseConfiguration(StringRef Text, FormatStyle *Style) {
StyleSet.Add(std::move(DefaultStyle));
}
*Style = *StyleSet.Get(Language);
+ if (Style->InsertTrailingCommas != FormatStyle::TCS_None &&
+ Style->BinPackArguments) {
+ // See comment on FormatStyle::TSC_Wrapped.
+ return make_error_code(ParseError::BinPackTrailingCommaConflict);
+ }
return make_error_code(ParseError::Success);
}
@@ -1466,6 +1485,75 @@ class Formatter : public TokenAnalyzer {
FormattingAttemptStatus *Status;
};
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:
+/// const x = [
+/// 1,
+/// ];
+/// TrailingCommaInserter runs after formatting. To avoid causing a required
+/// reformatting (and thus reflow), it never inserts a comma that'd exceed the
+/// ColumnLimit.
+///
+/// Because trailing commas disable binpacking of arrays, TrailingCommaInserter
+/// is conceptually incompatible with bin packing.
+class TrailingCommaInserter : public TokenAnalyzer {
+public:
+ TrailingCommaInserter(const Environment &Env, const FormatStyle &Style)
+ : TokenAnalyzer(Env, Style) {}
+
+ std::pair<tooling::Replacements, unsigned>
+ analyze(TokenAnnotator &Annotator,
+ SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+ FormatTokenLexer &Tokens) override {
+ AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
+ tooling::Replacements Result;
+ insertTrailingCommas(AnnotatedLines, Result);
+ return {Result, 0};
+ }
+
+private:
+ /// Inserts trailing commas in [] and {} initializers if they wrap over
+ /// multiple lines.
+ void insertTrailingCommas(SmallVectorImpl<AnnotatedLine *> &Lines,
+ tooling::Replacements &Result) {
+ for (AnnotatedLine *Line : Lines) {
+ insertTrailingCommas(Line->Children, Result);
+ if (!Line->Affected)
+ continue;
+ for (FormatToken *FormatTok = Line->First; FormatTok;
+ FormatTok = FormatTok->Next) {
+ if (FormatTok->NewlinesBefore == 0)
+ continue;
+ FormatToken *Matching = FormatTok->MatchingParen;
+ if (!Matching || !FormatTok->getPreviousNonComment())
+ continue;
+ if (!(FormatTok->is(tok::r_square) &&
+ Matching->is(TT_ArrayInitializerLSquare)) &&
+ !(FormatTok->is(tok::r_brace) && Matching->is(TT_DictLiteral)))
+ continue;
+ FormatToken *Prev = FormatTok->getPreviousNonComment();
+ if (Prev->is(tok::comma) || Prev->is(tok::semi))
+ continue;
+ // getEndLoc is not reliably set during re-lexing, use text length
+ // instead.
+ SourceLocation Start =
+ Prev->Tok.getLocation().getLocWithOffset(Prev->TokenText.size());
+ // If inserting a comma would push the code over the column limit, skip
+ // this location - it'd introduce an unstable formatting due to the
+ // required reflow.
+ unsigned ColumnNumber =
+ Env.getSourceManager().getSpellingColumnNumber(Start);
+ if (ColumnNumber > Style.ColumnLimit)
+ continue;
+ // Comma insertions cannot conflict with each other, and this pass has a
+ // clean set of Replacements, so the operation below cannot fail.
+ cantFail(Result.add(
+ tooling::Replacement(Env.getSourceManager(), Start, 0, ",")));
+ }
+ }
+ }
+};
+
// This class clean up the erroneous/redundant code around the given ranges in
// file.
class Cleaner : public TokenAnalyzer {
@@ -2435,6 +2523,12 @@ reformat(const FormatStyle &Style, StringRef Code,
return Formatter(Env, Expanded, Status).process();
});
+ if (Style.Language == FormatStyle::LK_JavaScript &&
+ Style.InsertTrailingCommas == FormatStyle::TCS_Wrapped)
+ Passes.emplace_back([&](const Environment &Env) {
+ return TrailingCommaInserter(Env, Expanded).process();
+ });
+
auto Env =
std::make_unique<Environment>(Code, FileName, Ranges, FirstStartColumn,
NextStartColumn, LastStartColumn);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index cf8bb2c52f3d..63f83f7ca23c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -13031,6 +13031,12 @@ TEST_F(FormatTest, ParsesConfigurationWithLanguages) {
"IndentWidth: 34",
&Style),
ParseError::Unsuitable);
+ FormatStyle BinPackedTCS = {};
+ BinPackedTCS.Language = FormatStyle::LK_JavaScript;
+ EXPECT_EQ(parseConfiguration("BinPackArguments: true\n"
+ "InsertTrailingCommas: Wrapped",
+ &BinPackedTCS),
+ ParseError::BinBackTrailingCommaConflict);
EXPECT_EQ(12u, Style.IndentWidth);
CHECK_PARSE("IndentWidth: 56", IndentWidth, 56u);
EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
diff --git a/clang/unittests/Format/FormatTestJS.cpp b/clang/unittests/Format/FormatTestJS.cpp
index 3c104b7aadbe..6efb8662f0f5 100644
--- a/clang/unittests/Format/FormatTestJS.cpp
+++ b/clang/unittests/Format/FormatTestJS.cpp
@@ -878,6 +878,45 @@ TEST_F(FormatTestJS, ColumnLayoutForArrayLiterals) {
"]);");
}
+TEST_F(FormatTestJS, TrailingCommaInsertion) {
+ FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+ Style.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
+ // Insert comma in wrapped array.
+ verifyFormat("const x = [\n"
+ " 1, //\n"
+ " 2,\n"
+ "];",
+ "const x = [\n"
+ " 1, //\n"
+ " 2];",
+ Style);
+ // Insert comma in newly wrapped array.
+ Style.ColumnLimit = 30;
+ verifyFormat("const x = [\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaa,\n"
+ "];",
+ "const x = [aaaaaaaaaaaaaaaaaaaaaaaaa];", Style);
+ // Do not insert trailing commas if they'd exceed the colum limit
+ verifyFormat("const x = [\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ "];",
+ "const x = [aaaaaaaaaaaaaaaaaaaaaaaaaaaa];", Style);
+ // Object literals.
+ verifyFormat("const x = {\n"
+ " a: aaaaaaaaaaaaaaaaa,\n"
+ "};",
+ "const x = {a: aaaaaaaaaaaaaaaaa};", Style);
+ verifyFormat("const x = {\n"
+ " a: aaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ "};",
+ "const x = {a: aaaaaaaaaaaaaaaaaaaaaaaaa};", Style);
+ // Object literal types.
+ verifyFormat("let x: {\n"
+ " a: aaaaaaaaaaaaaaaaaaaaa,\n"
+ "};",
+ "let x: {a: aaaaaaaaaaaaaaaaaaaaa};", Style);
+}
+
TEST_F(FormatTestJS, FunctionLiterals) {
FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
More information about the cfe-commits
mailing list