Skip to content

Commit

Permalink
Tweak block argument heuristics (#1548)
Browse files Browse the repository at this point in the history 
Change the block format heuristics.

This produces output that better matches what users expected in the associated issues. I think it also looks better in the affected regression tests. And I ran this on a large corpus and looking at the diffs, these rules seem to be much better across the board.

The new rules are basically:

* A block formatted argument list can't end up with named arguments on multiple lines.

* Only one trailing argument is allowed after the block argument.

The existing rules are also kept:

* Prefer a function block argument over a collection one.

* Only allow a single block argument.

* Allow the first argument to be adjacent strings with a later function block argument (to handle stuff like test() and group()).

Fix #1527.
Fix #1528.
Fix #1543.
  • Loading branch information
@munificent
munificent authored Aug 28, 2024
1 parent 494a1d7 commit 05d08d9
Show file tree
Hide file tree
Showing 22 changed files with 604 additions and 267 deletions.
10 changes: 10 additions & 0 deletions lib/src/ast_extensions.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ import 'package:analyzer/dart/ast/token.dart';
import 'piece/list.dart';

extension AstNodeExtensions on AstNode {
/// When this node is in an argument list, what kind of block formatting
/// category it belongs to.
BlockFormat get blockFormatType => switch (this) {
AdjacentStrings(indentStrings: true) =>
BlockFormat.indentedAdjacentStrings,
AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
Expression(:var blockFormatType) => blockFormatType,
_ => BlockFormat.none,
};

/// The first token at the beginning of this AST node, not including any
/// tokens for leading doc comments.
///
Expand Down
203 changes: 122 additions & 81 deletions lib/src/front_end/delimited_list_builder.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ class DelimitedListBuilder {
});
}

if (_style.allowBlockElement) _setBlockElementFormatting();

var piece =
ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
if (_mustSplit || forceSplit) piece.pin(State.split);
Expand Down Expand Up @@ -134,8 +132,8 @@ class DelimitedListBuilder {
/// [addCommentsBefore()] for the first token in the [piece].
///
/// Assumes there is no comma after this piece.
void add(Piece piece, [BlockFormat format = BlockFormat.none]) {
_elements.add(ListElementPiece(_leadingComments, piece, format));
void add(Piece piece) {
_elements.add(ListElementPiece(_leadingComments, piece));
_leadingComments.clear();
_commentsBeforeComma = CommentSequence.empty;
}
Expand All @@ -152,25 +150,33 @@ class DelimitedListBuilder {
// Handle comments between the preceding element and this one.
addCommentsBefore(element.firstNonCommentToken);

// See if it's an expression that supports block formatting.
var format = switch (element) {
AdjacentStrings(indentStrings: true) =>
BlockFormat.indentedAdjacentStrings,
AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
Expression() => element.blockFormatType,
DartPattern() when element.canBlockSplit => BlockFormat.collection,
_ => BlockFormat.none,
};

// Traverse the element itself.
add(_visitor.nodePiece(element), format);
add(_visitor.nodePiece(element));

var nextToken = element.endToken.next!;
if (nextToken.lexeme == ',') {
_commentsBeforeComma = _visitor.comments.takeCommentsBefore(nextToken);
}
}

/// Visits a list of [elements].
///
/// If [allowBlockArgument] is `true`, then allows one element to receive
/// block formatting if appropriate, as in:
///
/// function(argument, [
/// block,
/// like,
/// ], argument);
void visitAll(List<AstNode> elements, {bool allowBlockArgument = false}) {
for (var i = 0; i < elements.length; i++) {
var element = elements[i];
visit(element);
}

if (allowBlockArgument) _setBlockArgument(elements);
}

/// Inserts an inner left delimiter between two elements.
///
/// This is used for parameter lists when there are both mandatory and
Expand Down Expand Up @@ -398,6 +404,14 @@ class DelimitedListBuilder {
);
}

/// Given an argument list, determines which if any of the arguments should
/// get special block-like formatting as in the list literal in:
///
/// function(argument, [
/// block,
/// like,
/// ], argument);
///
/// Looks at the [BlockFormat] types of all of the elements to determine if
/// one of them should be block formatted.
///
Expand Down Expand Up @@ -426,85 +440,112 @@ class DelimitedListBuilder {
///
/// Stores the result of this calculation by setting flags on the
/// [ListElement]s.
void _setBlockElementFormatting() {
// TODO(tall): These heuristics will probably need some iteration.
var functions = <int>[];
var collections = <int>[];
var adjacentStrings = <int>[];

for (var i = 0; i < _elements.length; i++) {
switch (_elements[i].blockFormat) {
case BlockFormat.function:
functions.add(i);
case BlockFormat.collection:
collections.add(i);
case BlockFormat.invocation:
// We don't allow function calls as block elements partially for style
// and partially for performance. It often doesn't look great to let
// nested function calls pack arbitrarily deeply as block arguments:
//
// ScaffoldMessenger.of(context).showSnackBar(SnackBar(
// content: Text(
// localizations.demoSnackbarsAction,
// )));
//
// This is better when expanded like:
//
// ScaffoldMessenger.of(context).showSnackBar(
// SnackBar(
// content: Text(
// localizations.demoSnackbarsAction,
// ),
// ),
// );
//
// Also, when invocations can be block arguments, which themselves
// may contain block arguments, it's easy to run into combinatorial
// performance in the solver as it tries to determine which of the
// nested calls should and shouldn't be block formatted.
break;
case BlockFormat.indentedAdjacentStrings:
case BlockFormat.unindentedAdjacentStrings:
adjacentStrings.add(i);
case BlockFormat.none:
break; // Not a block element.
void _setBlockArgument(List<AstNode> arguments) {
var candidateIndex = _candidateBlockArgument(arguments);
if (candidateIndex == -1) return;

// Only allow up to one trailing argument after the block argument. This
// handles the common `tags` and `timeout` named arguments in `test()` and
// `group()` while still mostly having the block argument be at the end of
// the argument list.
if (candidateIndex < arguments.length - 2) return;

// If there are multiple named arguments, they should never end up on
// separate lines (unless the whole argument list fully splits). Otherwise,
// it's too easy for an argument name to get buried in the middle of a line.
// So we look for named arguments before, on, and after the candidate
// argument. If more than one of those sections of arguments has a named
// argument, then we don't allow the block argument.
var namedSections = 0;
bool hasNamedArgument(int from, int to) {
for (var i = from; i < to; i++) {
if (arguments[i] is NamedExpression) return true;
}

return false;
}

switch ((functions, collections, adjacentStrings)) {
// Only allow block formatting in an argument list containing adjacent
// strings when:
//
// 1. The block argument is a function expression.
// 2. It is the second argument, following an adjacent strings expression.
// 3. There are no other adjacent strings in the argument list.
//
// This matches the `test()` and `group()` and other similar APIs where
// you have a message string followed by a block-like function expression
// but little else.
// TODO(tall): We may want to iterate on these heuristics. For now,
// starting with something very narrowly targeted.
case ([1], _, [0]):
if (hasNamedArgument(0, candidateIndex)) namedSections++;
if (hasNamedArgument(candidateIndex, candidateIndex + 1)) namedSections++;
if (hasNamedArgument(candidateIndex + 1, arguments.length)) namedSections++;

if (namedSections > 1) return;

// Edge case: If the first argument is adjacent strings and the second
// argument is a function literal, with optionally a third non-block
// argument, then treat the function as the block argument.
//
// This matches the `test()` and `group()` and other similar APIs where
// you have a message string followed by a block-like function expression
// but little else, as in:
//
// test('Some long test description '
// 'that splits into multiple lines.', () {
// expect(1 + 2, 3);
// });
if (candidateIndex == 1 &&
arguments[0] is! NamedExpression &&
arguments[1] is! NamedExpression) {
if ((arguments[0].blockFormatType, arguments[1].blockFormatType)
case (
BlockFormat.unindentedAdjacentStrings ||
BlockFormat.indentedAdjacentStrings,
BlockFormat.function
)) {
// The adjacent strings.
_elements[0].allowNewlinesWhenUnsplit = true;
if (_elements[0].blockFormat == BlockFormat.unindentedAdjacentStrings) {
if (arguments[0].blockFormatType ==
BlockFormat.unindentedAdjacentStrings) {
_elements[0].indentWhenBlockFormatted = true;
}

// The block-formattable function.
_elements[1].allowNewlinesWhenUnsplit = true;
return;
}
}

// If we get here, we have a block argument.
_elements[candidateIndex].allowNewlinesWhenUnsplit = true;
}

/// If an argument in [arguments] is a candidate to be block formatted,
/// returns its index.
///
/// Otherwise, returns `-1`.
int _candidateBlockArgument(List<AstNode> arguments) {
var functionIndex = -1;
var collectionIndex = -1;
// var stringIndex = -1;

for (var i = 0; i < arguments.length; i++) {
// See if it's an expression that supports block formatting.
switch (arguments[i].blockFormatType) {
case BlockFormat.function:
if (functionIndex >= 0) {
functionIndex = -2;
} else {
functionIndex = i;
}

// A function expression takes precedence over other block arguments.
case ([var element], _, _):
_elements[element].allowNewlinesWhenUnsplit = true;
case BlockFormat.collection:
if (collectionIndex >= 0) {
collectionIndex = -2;
} else {
collectionIndex = i;
}

// A single collection literal can be block formatted even if there are
// other arguments.
case ([], [var element], _):
_elements[element].allowNewlinesWhenUnsplit = true;
case BlockFormat.invocation:
case BlockFormat.indentedAdjacentStrings:
case BlockFormat.unindentedAdjacentStrings:
case BlockFormat.none:
break; // Normal argument.
}
}

// If we get here, there are no block element, or it's ambiguous as to
// which one should be it so none are.
if (functionIndex >= 0) return functionIndex;
if (collectionIndex >= 0) return collectionIndex;

return -1;
}
}
7 changes: 4 additions & 3 deletions lib/src/front_end/piece_factory.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ mixin PieceFactory {
leftBracket: leftBracket,
elements,
rightBracket: rightBracket,
style: const ListStyle(allowBlockElement: true));
allowBlockArgument: true);
}

/// Writes a bracket-delimited block or declaration body.
Expand Down Expand Up @@ -972,7 +972,8 @@ mixin PieceFactory {
{required Token leftBracket,
required Token rightBracket,
ListStyle style = const ListStyle(),
bool preserveNewlines = false}) {
bool preserveNewlines = false,
bool allowBlockArgument = false}) {
// If the list is completely empty, write the brackets directly inline so
// that we create fewer pieces.
if (!elements.canSplit(rightBracket)) {
Expand All @@ -988,7 +989,7 @@ mixin PieceFactory {
if (preserveNewlines && elements.containsLineComments(rightBracket)) {
_preserveNewlinesInCollection(elements, builder);
} else {
elements.forEach(builder.visit);
builder.visitAll(elements, allowBlockArgument: allowBlockArgument);
}

builder.rightBracket(rightBracket);
Expand Down
31 changes: 8 additions & 23 deletions lib/src/piece/list.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,6 @@ final class ListElementPiece extends Piece {

final Piece? _content;

/// What kind of block formatting can be applied to this element.
final BlockFormat blockFormat;

/// Whether newlines are allowed in this element when this list is unsplit.
///
/// This is generally only true for a single "block" element, as in:
Expand Down Expand Up @@ -312,16 +309,13 @@ final class ListElementPiece extends Piece {
/// delimiter (here `,` and 2).
int _commentsBeforeDelimiter = 0;

ListElementPiece(
List<Piece> leadingComments, Piece element, BlockFormat format)
ListElementPiece(List<Piece> leadingComments, Piece element)
: _leadingComments = [...leadingComments],
_content = element,
blockFormat = format;
_content = element;

ListElementPiece.comment(Piece comment)
: _leadingComments = const [],
_content = null,
blockFormat = BlockFormat.none {
_content = null {
_hangingComments.add(comment);
}

Expand Down Expand Up @@ -404,9 +398,10 @@ enum BlockFormat {
/// elements.
function,

/// The element is a collection literal.
/// The element is a collection literal or multiline string literal.
///
/// These can be block formatted even when there are other arguments.
/// If there is only one of these and no [BlockFormat.function] elements, then
/// it can be block formatted.
collection,

/// A function or method invocation.
Expand All @@ -416,7 +411,7 @@ enum BlockFormat {

/// The element is an adjacent strings expression that's in an list that
/// requires its subsequent lines to be indented (because there are other
/// string literal in the list).
/// string literals in the list).
indentedAdjacentStrings,

/// The element is an adjacent strings expression that's in an list that
Expand Down Expand Up @@ -459,18 +454,8 @@ class ListStyle {
/// // ^ ^
final bool spaceWhenUnsplit;

/// Whether an element in the list is allowed to have block-like formatting,
/// as in:
///
/// function(argument, [
/// block,
/// like,
/// ], argument);
final bool allowBlockElement;

const ListStyle(
{this.commas = Commas.trailing,
this.splitCost = Cost.normal,
this.spaceWhenUnsplit = false,
this.allowBlockElement = false});
this.spaceWhenUnsplit = false});
}
Loading

0 comments on commit 05d08d9

Please sign in to comment.