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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
