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.

Sign up for GitHub

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

Jump to bottom

Tweak block argument heuristics #1548

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Tweak block argument heuristics #1548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb61086
Reorganize the code for selecting a block argument.
munificent Aug 22, 2024
210b073
Change the block format heuristics.
munificent Aug 28, 2024
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
10 changes: 10 additions & 0 deletions lib/src/ast_extensions.dart
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 @@ -974,7 +974,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 @@ -990,7 +991,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
Open in desktop
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