diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -35,7 +35,12 @@ namespace format { -enum class ParseError { Success = 0, Error, Unsuitable }; +enum class ParseError { + Success = 0, + Error, + Unsuitable, + BinBackTrailingCommaConflict +}; class ParseErrorCategory final : public std::error_category { public: const char *name() const noexcept override; @@ -544,6 +549,20 @@ /// \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 --- a/clang/lib/Format/Format.cpp +++ b/clang/lib/Format/Format.cpp @@ -157,6 +157,13 @@ } }; +template <> struct ScalarEnumerationTraits { + 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 { static void enumeration(IO &IO, FormatStyle::BinaryOperatorStyle &Value) { IO.enumCase(Value, "All", FormatStyle::BOS_All); @@ -486,6 +493,7 @@ 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 @@ return "Invalid argument"; case ParseError::Unsuitable: return "Unsuitable"; + case ParseError::BinBackTrailingCommaConflict: + return "trailing comma insertion cannot be used with bin packing"; } llvm_unreachable("unexpected parse error"); } @@ -788,6 +798,7 @@ 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 @@ // 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 @@ 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::BinBackTrailingCommaConflict); + } return make_error_code(ParseError::Success); } @@ -1466,6 +1485,75 @@ 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 + analyze(TokenAnnotator &Annotator, + SmallVectorImpl &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 &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 @@ 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(Code, FileName, Ranges, FirstStartColumn, NextStartColumn, LastStartColumn); diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -13031,6 +13031,12 @@ "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 --- a/clang/unittests/Format/FormatTestJS.cpp +++ b/clang/unittests/Format/FormatTestJS.cpp @@ -878,6 +878,45 @@ "]);"); } +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;