From 4dc2e97095ccfab470ac34a014854c5d4fff6298 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Fri, 6 Dec 2024 13:35:57 -0800 Subject: [PATCH] Handle trailing commas in for-loop updaters. Fix #1354. --- CHANGELOG.md | 1 + lib/src/front_end/delimited_list_builder.dart | 16 ++++++++-------- lib/src/front_end/piece_factory.dart | 15 ++++++++++++--- test/tall/expression/collection_for.stmt | 14 ++++++++++++++ test/tall/regression/1300/1354.stmt | 12 ++++++++++++ test/tall/statement/for_comment.stmt | 16 ++++++++++++++++ test/tall/statement/for_initializer.stmt | 17 ++++++++++++++++- 7 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 test/tall/regression/1300/1354.stmt diff --git a/CHANGELOG.md b/CHANGELOG.md index aec802fd..bb3d915e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## 3.0.1-wip * Ensure comment formatting is idempotent (#1606). +* Handle trailing commas in for-loop updaters (#1354). ## 3.0.0 diff --git a/lib/src/front_end/delimited_list_builder.dart b/lib/src/front_end/delimited_list_builder.dart index 639c0c05..54a45c58 100644 --- a/lib/src/front_end/delimited_list_builder.dart +++ b/lib/src/front_end/delimited_list_builder.dart @@ -138,18 +138,18 @@ final class DelimitedListBuilder { _commentsBeforeComma = CommentSequence.empty; } - /// Adds the contents of [lineBuilder] to this outer [DelimitedListBuilder]. + /// Adds the contents of [inner] to this outer [DelimitedListBuilder]. /// - /// This is used when preserving newlines inside a collection literal. The - /// [lineBuilder] will be used for the elements that should be packed onto a - /// single line, and this builder is for the rows that are each on their own - /// line. - void addLineBuilder(DelimitedListBuilder lineBuilder) { + /// This is used when a [DelimiterListBuilder] is building a piece that will + /// then become an element in a surrounding [DelimitedListBuilder]. It ensures + /// that any comments around a trailing comma after [inner] don't get lost and + /// are instead hoisted up to be captured by this builder. + void addInnerBuilder(DelimitedListBuilder inner) { // Add the elements of the line to this builder. - add(lineBuilder.build()); + add(inner.build()); // Make sure that any trailing comments on the line aren't lost. - _commentsBeforeComma = lineBuilder._commentsBeforeComma; + _commentsBeforeComma = inner._commentsBeforeComma; } /// Writes any comments appearing before [token] to the list. diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index 3d0f2752..39386255 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -430,7 +430,16 @@ mixin PieceFactory { // The update clauses. if (forParts.updaters.isNotEmpty) { partsList.addCommentsBefore(forParts.updaters.first.beginToken); - partsList.add(createCommaSeparated(forParts.updaters)); + + // Create a nested list builder for the updaters so that they can + // remain unsplit even while the clauses split. + var updaterBuilder = DelimitedListBuilder( + this, const ListStyle(commas: Commas.nonTrailing)); + forParts.updaters.forEach(updaterBuilder.visit); + + // Add the updater builder to the clause builder so that any comments + // around a trailing comma after the updaters don't get dropped. + partsList.addInnerBuilder(updaterBuilder); } partsList.rightBracket(rightParenthesis); @@ -1083,7 +1092,7 @@ mixin PieceFactory { elements[i - 1].endToken, element.beginToken)) { // This element begins a new line. Add the elements on the previous // line to the list builder and start a new line. - builder.addLineBuilder(lineBuilder); + builder.addInnerBuilder(lineBuilder); lineBuilder = DelimitedListBuilder(this, lineStyle); atLineStart = true; } @@ -1099,7 +1108,7 @@ mixin PieceFactory { } // Finish the last line if there is anything on it. - if (!atLineStart) builder.addLineBuilder(lineBuilder); + if (!atLineStart) builder.addInnerBuilder(lineBuilder); } /// Writes a [VariablePiece] for a named or wildcard variable pattern. diff --git a/test/tall/expression/collection_for.stmt b/test/tall/expression/collection_for.stmt index d62528a4..7d753d7f 100644 --- a/test/tall/expression/collection_for.stmt +++ b/test/tall/expression/collection_for.stmt @@ -170,4 +170,18 @@ var list = [ inc ) element, +]; +>>> Trailing comma in increments. +var list = [ + for ( + x = 1; + true; + x += 1, x += 2, + ) + element, +]; +<<< +var list = [ + for (x = 1; true; x += 1, x += 2) + element, ]; \ No newline at end of file diff --git a/test/tall/regression/1300/1354.stmt b/test/tall/regression/1300/1354.stmt new file mode 100644 index 00000000..4e1288cc --- /dev/null +++ b/test/tall/regression/1300/1354.stmt @@ -0,0 +1,12 @@ +>>> +void main() { + for (int i; i < 10; print("foo"), ++i, print("bar"),) { + break; + } +} +<<< +void main() { + for (int i; i < 10; print("foo"), ++i, print("bar")) { + break; + } +} \ No newline at end of file diff --git a/test/tall/statement/for_comment.stmt b/test/tall/statement/for_comment.stmt index 33f6f8c3..33a61af6 100644 --- a/test/tall/statement/for_comment.stmt +++ b/test/tall/statement/for_comment.stmt @@ -223,6 +223,22 @@ for ( ) { body; } +>>> Preserve comments around discarded increment trailing comma. +for ( + init; + cond; + incr /* c1 */ , /* c2 */ +) { + body; +} +<<< +for ( + init; + cond; + incr /* c1 */ /* c2 */ +) { + body; +} >>> Line comment before first `;` in fully empty clauses. for ( // comment ; ; ) { body; } diff --git a/test/tall/statement/for_initializer.stmt b/test/tall/statement/for_initializer.stmt index 4672f17a..17be2546 100644 --- a/test/tall/statement/for_initializer.stmt +++ b/test/tall/statement/for_initializer.stmt @@ -155,4 +155,19 @@ for ( second = 2, third = 3, fourth = 4 -) {} \ No newline at end of file +) {} +>>> Discard trailing comma in unsplit increments. +for (foo; bar; first = 1, second = 2,) {} +<<< +for (foo; bar; first = 1, second = 2) {} +>>> Discard trailing comma in split increments. +for (foo; bar; first = 1, second = 2, third = 3, fourth = 4,) {} +<<< +for ( + foo; + bar; + first = 1, + second = 2, + third = 3, + fourth = 4 +) {}