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

Support "block argument" formatting. #1322

Merged
merged 3 commits into from
Nov 17, 2023
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
66 changes: 55 additions & 11 deletions lib/src/ast_extensions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -126,17 +126,61 @@ extension ExpressionExtensions on Expression {
/// element,
/// ];
/// ```
bool get isDelimited => switch (this) {
FunctionExpression() => true,
InstanceCreationExpression() => true,
ListLiteral() => true,
MethodInvocation() => true,
ParenthesizedExpression(:var expression) => expression.isDelimited,
RecordLiteral() => true,
SetOrMapLiteral() => true,
SwitchExpression() => true,
_ => false,
};
///
/// Likewise, in an argument list, delimited expressions can be given
/// block-like formatting:
///
/// ```
/// function([
/// element,
/// ]);
/// ```
bool get isDelimited {
// Unwrap named expressions to get the real expression inside.
var expression = this;
if (expression is NamedExpression) {
expression = expression.expression;
}

// TODO(tall): We should also allow multi-line strings to be formatted
// like block arguments, at least in some cases like:
//
// ```
// function('''
// Lots of
// text
// ''');
// ```

// TODO(tall): Consider whether immediately-invoked function expressions
// should be block argument candidates, like:
//
// ```
// function(() {
// body;
// }());
// ```
return switch (expression) {
// A function expression can use either a non-empty parameter list or a
// non-empty block body for block formatting.
FunctionExpression(:var parameters!, :var body) =>
munificent marked this conversation as resolved.
Show resolved Hide resolved
!parameters.parameters.isEmptyBody(parameters.rightParenthesis) ||
body is BlockFunctionBody &&
munificent marked this conversation as resolved.
Show resolved Hide resolved
!body.block.statements.isEmptyBody(body.block.rightBracket),
ListLiteral(:var elements, :var rightBracket) ||
SetOrMapLiteral(:var elements, :var rightBracket) =>
!elements.isEmptyBody(rightBracket),
munificent marked this conversation as resolved.
Show resolved Hide resolved
RecordLiteral(:var fields, :var rightParenthesis) =>
!fields.isEmptyBody(rightParenthesis),
SwitchExpression(:var cases, :var rightBracket) =>
!cases.isEmptyBody(rightBracket),
InstanceCreationExpression(:var argumentList) ||
MethodInvocation(:var argumentList) =>
!argumentList.arguments.isEmptyBody(argumentList.rightParenthesis),
ParenthesizedExpression(:var expression) => expression.isDelimited,
_ => false,
};
}

/// Whether this is an argument in an argument list with a trailing comma.
bool get isTrailingCommaArgument {
Expand Down
28 changes: 13 additions & 15 deletions lib/src/front_end/ast_node_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,8 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>

@override
void visitArgumentList(ArgumentList node) {
createList(
leftBracket: node.leftParenthesis,
node.arguments,
rightBracket: node.rightParenthesis);
createArgumentList(
node.leftParenthesis, node.arguments, node.rightParenthesis);
}

@override
Expand All @@ -134,14 +132,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
@override
void visitAssertStatement(AssertStatement node) {
token(node.assertKeyword);
createList(
[
node.condition,
if (node.message case var message?) message,
],
leftBracket: node.leftParenthesis,
rightBracket: node.rightParenthesis,
);
createArgumentList(
node.leftParenthesis,
[
node.condition,
if (node.message case var message?) message,
],
node.rightParenthesis);
token(node.semicolon);
}

Expand Down Expand Up @@ -438,7 +435,6 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
spaceWhenUnsplit: true, splitListIfBeforeSplits: true));
builder.leftBracket(node.leftBracket);
node.constants.forEach(builder.visit);

builder.rightBracket(semicolon: node.semicolon, node.rightBracket);
pieces.give(builder.build());
} else {
Expand Down Expand Up @@ -959,8 +955,10 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>

@override
void visitMethodInvocation(MethodInvocation node) {
// TODO(tall): Support method invocation with explicit target expressions.
if (node.target != null) throw UnimplementedError();
// TODO(tall): Support splitting at `.` or `?.`. Right now we just format
// it inline so that we can use method calls in other tests.
visit(node.target);
token(node.operator);

visit(node.methodName);
visit(node.typeArguments);
Expand Down
6 changes: 3 additions & 3 deletions lib/src/front_end/comment_writer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ import 'piece_writer.dart';
/// couple of different ways.
///
/// Comments between top-level declarations, member declarations inside types,
/// and statements are handled directly by [SequenceBuilder].
/// and statements are handled directly by [SequenceBuilder]. Comments inside
/// argument lists, collection literals, and other similar constructs are
/// handled directly be [DelimitedPieceBuilder].
///
/// All other comments occur inside the middle of some expression or other
/// construct. These get directly embedded in the [TextPiece] of the code being
/// written. When that [TextPiece] is output later, it will include the comments
/// as well.
// TODO(tall): When argument lists and their comment handling is supported,
// mention that here.
mixin CommentWriter {
PieceWriter get pieces;

Expand Down
62 changes: 60 additions & 2 deletions lib/src/front_end/delimited_list_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ class DelimitedListBuilder {

final ListStyle _style;

/// The nodes that have been visited by this builder.
///
/// Used to find the block element from them, if desired.
final List<AstNode> _blockCandidates = [];

/// The list of comments following the most recently written element before
/// any comma following the element.
CommentSequence _commentsBeforeComma = CommentSequence.empty;
Expand All @@ -44,8 +49,15 @@ class DelimitedListBuilder {
/// literal, etc.
DelimitedListBuilder(this._visitor, [this._style = const ListStyle()]);

ListPiece build() =>
ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
/// Create the final [ListPiece] out of the added brackets, delimiters,
munificent marked this conversation as resolved.
Show resolved Hide resolved
/// elements, and style.
ListPiece build() {
var blockElement = -1;
if (_style.allowBlockElement) blockElement = _findBlockElement();

return ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket,
munificent marked this conversation as resolved.
Show resolved Hide resolved
_style, blockElement);
}

/// Adds the opening [bracket] to the built list.
///
Expand Down Expand Up @@ -144,6 +156,9 @@ class DelimitedListBuilder {
if (nextToken.lexeme == ',') {
_commentsBeforeComma = _visitor.takeCommentsBefore(nextToken);
}

// If this element might be block formatted, remember it.
if (_style.allowBlockElement) _blockCandidates.add(element);
}

/// Inserts an inner left delimiter between two elements.
Expand Down Expand Up @@ -373,4 +388,47 @@ class DelimitedListBuilder {
leading: leadingComments
);
}

/// If [_blockCandidates] contains a single expression that can receive
/// block formatting, then returns its index. Otherwise returns `-1`.
int _findBlockElement() {
// TODO(tall): These heuristics will probably need some iteration.
var functions = <int>[];
var others = <int>[];

for (var i = 0; i < _blockCandidates.length; i++) {
var node = _blockCandidates[i];
if (node is Expression && node.isDelimited) {
if (node is FunctionExpression) {
functions.add(i);
} else {
others.add(i);
}
}
}

// A function expression takes precedence over other block arguments.
if (functions.length == 1) return functions.first;

// Otherwise, if there is single block argument, it can be block formatted.
if (functions.isEmpty && others.length == 1) return others.first;

// There are no block arguments, or it's ambiguous as to which one should
// be it.
// TODO(tall): The old formatter allows multiple block arguments, like:
//
// ```
// function(() {
// body;
// }, () {
// more;
// });
// ```
//
// This doesn't seem very common in the Flutter repo, but does occur
// sometimes. We'll probably want to experiment to see if it's worth
// supporting multiple block arguments. If so, we should at least require
// them to be contiguous with no non-block arguments in the middle.
return -1;
}
}
26 changes: 16 additions & 10 deletions lib/src/front_end/piece_factory.dart
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ typedef BinaryOperation = (AstNode left, Token operator, AstNode right);
mixin PieceFactory implements CommentWriter {
void visit(AstNode? node, {void Function()? before, void Function()? after});

/// Creates a [ListPiece] for an argument list.
void createArgumentList(
Token leftBracket, Iterable<AstNode> elements, Token rightBracket) {
return createList(
leftBracket: leftBracket,
elements,
rightBracket: rightBracket,
style: const ListStyle(allowBlockElement: true));
}

/// Creates a [BlockPiece] for a given bracket-delimited block or declaration
/// body.
///
Expand Down Expand Up @@ -425,20 +435,16 @@ mixin PieceFactory implements CommentWriter {
/// Visits the `switch (expr)` part of a switch statement or expression.
void createSwitchValue(Token switchKeyword, Token leftParenthesis,
Expression value, Token rightParenthesis) {
// Format like an argument list since it is an expression surrounded by
// parentheses.
var builder = DelimitedListBuilder(
this, const ListStyle(commas: Commas.none, splitCost: 2));

// Attach the `switch ` as part of the `(`.
token(switchKeyword);
space();

builder.leftBracket(leftParenthesis);
builder.visit(value);
builder.rightBracket(rightParenthesis);

pieces.give(builder.build());
createList(
leftBracket: leftParenthesis,
[value],
rightBracket: rightParenthesis,
style: const ListStyle(
commas: Commas.none, splitCost: 2, allowBlockElement: true));
}

/// Creates a class, enum, extension, mixin, or mixin application class
Expand Down
27 changes: 11 additions & 16 deletions lib/src/piece/assign.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,32 +21,26 @@ import 'piece.dart';
/// var x = 123;
/// ```
///
/// [_insideValue] If the value is a delimited "block-like" expression,
/// then we can split inside the block but not at the `=` with no additional
/// indentation:
/// If the value is a delimited "block-like" expression, then we allow splitting
/// inside the value but not at the `=` with no additional indentation:
///
/// ```
/// var list = [
/// element,
/// ];
/// ```
///
/// [State.split] Split after the `=`:
/// [_atOperator] Split after the `=`:
///
/// ```
/// var name =
/// longValueExpression;
/// ```
class AssignPiece extends Piece {
/// Split inside the value but not at the `=`.
/// Split after the operator.
///
/// This is only allowed when the value is a delimited expression.
static const State _insideValue = State(1);

/// Split at the operator and allow splitting inside the value.
///
/// This is more costly because, when it's possible to split inside a
/// delimited value, we want to prefer that.
/// This is more costly because it's generally better to split either in the
/// value (if it's delimited) or in the target.
static const State _atOperator = State(2, cost: 2);

/// The left-hand side of the operation. Includes the operator unless it is
Expand Down Expand Up @@ -107,17 +101,18 @@ class AssignPiece extends Piece {
// ```

@override
List<State> get additionalStates =>
[if (_isValueDelimited) _insideValue, _atOperator];
List<State> get additionalStates => [_atOperator];

@override
void format(CodeWriter writer, State state) {
// A split in either child piece forces splitting after the "=" unless it's
// a delimited expression.
if (state == State.unsplit) writer.setAllowNewlines(false);
if (state == State.unsplit && !_isValueDelimited) {
writer.setAllowNewlines(false);
}

// Don't indent a split delimited expression.
if (state != _insideValue) writer.setIndent(Indent.expression);
if (state != State.unsplit) writer.setIndent(Indent.expression);

writer.format(target);
writer.splitIf(state == _atOperator);
Expand Down
Loading