Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't add a trailing comma in lists that shouldn't have one. #1640

Merged
merged 2 commits into from
Feb 1, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 3.0.2-wip

* Don't add a trailing comma in lists that don't allow it, even when there is
a trailing comment (#1639).

## 3.0.1

* Handle trailing commas in for-loop updaters (#1354).
Expand Down
17 changes: 15 additions & 2 deletions lib/src/front_end/delimited_list_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,21 @@ final class DelimitedListBuilder {
});
}

var piece =
ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
// Find the index of the last non-comment element. The list may have
// trailing elements that are just comments. In that case, those are
// not considered the last element when it comes to deciding whether to add
// a trailing comma on the last element.
var lastNonCommentElement = -1;
for (var i = _elements.length - 1; i >= 0; i--) {
if (!_elements[i].isComment) {
lastNonCommentElement = i;
break;
}
}

var piece = ListPiece(
_leftBracket, _elements, _blanksAfter, _rightBracket, _style,
lastNonCommentElement: lastNonCommentElement);
if (_mustSplit || forceSplit) piece.pin(State.split);
return piece;
}
Expand Down
27 changes: 21 additions & 6 deletions lib/src/piece/list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ final class ListPiece extends Piece {
/// The details of how this particular list should be formatted.
final ListStyle _style;

/// The index of the last element in [_elements] that has content and isn't
/// a comment, or `-1` if all elements are comments.
final int _lastNonCommentElement;

/// Whether any element in this argument list can be block formatted.
bool get hasBlockElement =>
_elements.any((element) => element.allowNewlinesWhenUnsplit);
Expand All @@ -64,8 +68,10 @@ final class ListPiece extends Piece {
/// [_elements] must not be empty. (If there are no elements, just concatenate
/// the brackets directly.)
ListPiece(
this._before, this._elements, this._blanksAfter, this._after, this._style)
: assert(_elements.isNotEmpty) {
this._before, this._elements, this._blanksAfter, this._after, this._style,
{required int lastNonCommentElement})
: assert(_elements.isNotEmpty),
_lastNonCommentElement = lastNonCommentElement {
// For most elements, we know whether or not it will have a comma based
// only on the comma style and its position in the list, so pin those here.
for (var i = 0; i < _elements.length; i++) {
Expand All @@ -80,13 +86,13 @@ final class ListPiece extends Piece {
// Always has a comma after every element except the last. The last
// will be constrained to have one or not depending on whether the
// list splits. See applyConstraints().
if (i < _elements.length - 1) {
if (i < _lastNonCommentElement) {
element.pin(ListElementPiece._appendComma);
}

case Commas.nonTrailing:
// Never a trailing comma after the last element.
element.pin(i < _elements.length - 1
element.pin(i < _lastNonCommentElement
? ListElementPiece._appendComma
: State.unsplit);

Expand All @@ -103,8 +109,8 @@ final class ListPiece extends Piece {
@override
void applyConstraints(State state, Constrain constrain) {
// Give the last element a trailing comma only if the list is split.
if (_style.commas == Commas.trailing) {
constrain(_elements.last,
if (_style.commas == Commas.trailing && _lastNonCommentElement != -1) {
constrain(_elements[_lastNonCommentElement],
state == State.split ? ListElementPiece._appendComma : State.unsplit);
}
}
Expand Down Expand Up @@ -319,6 +325,15 @@ final class ListElementPiece extends Piece {
_hangingComments.add(comment);
}

/// Whether this piece is a comment-only element, like the second element in:
///
/// [
/// real,
/// // Comment.
/// anotherReal,
/// ]
bool get isComment => _content == null;

void addComment(Piece comment, {bool beforeDelimiter = false}) {
_hangingComments.add(comment);
if (beforeDelimiter) _commentsBeforeDelimiter++;
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: dart_style
# Note: See tool/grind.dart for how to bump the version.
version: 3.0.1
version: 3.0.2-wip
description: >-
Opinionated, automatic Dart source code formatter.
Provides an API and a CLI tool.
Expand Down
33 changes: 33 additions & 0 deletions test/tall/regression/1600/1639.unit
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
>>>
class C<T
// 1
// 2
// 3
> {}
<<<
class C<
T
// 1
// 2
// 3
> {}
>>> (indent 2)
void Function<
Y extends A<FcovCyclicCoBound<Function(Function(FutureOr<Object>))>>
// ^
// [cfe] Type argument 'dynamic Function(dynamic Function(FutureOr<Object>))' doesn't conform to the bound 'dynamic Function(X)' of the type variable 'X' on 'FcovCyclicCoBound'.
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// [analyzer] COMPILE_TIME_ERROR.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS
>()
x264;
void Function<Y extends A<CFcov<Object>>>() x265;
<<<
void Function<
Y extends A<FcovCyclicCoBound<Function(Function(FutureOr<Object>))>>
// ^
// [cfe] Type argument 'dynamic Function(dynamic Function(FutureOr<Object>))' doesn't conform to the bound 'dynamic Function(X)' of the type variable 'X' on 'FcovCyclicCoBound'.
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// [analyzer] COMPILE_TIME_ERROR.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS
>()
x264;
void Function<Y extends A<CFcov<Object>>>() x265;
12 changes: 12 additions & 0 deletions test/tall/type/function_comment.stmt
Original file line number Diff line number Diff line change
Expand Up @@ -443,4 +443,16 @@ Function(
int c, [
direction,
])
x;
>>> After type parameter on its own line.
Function<
Y
// Comment.
>()
x;
<<<
Function<
Y
// Comment.
>()
x;