Skip to content

Commit

Permalink
Format call chains! \o/
Browse files Browse the repository at this point in the history
This handles unsplit call chains, fully split ones, as well as
block-style calls, like:

```
target.call(
  argument,
);
```

This doesn't fully implement the proposal I sketched out because it only
allows a single block call. There's a long TODO(tall) comment about
whether we want to do that, but I figured it makes sense to do that as a
separate iteration since there is so much in here.

But it implements most of it, and I've migrated all of the existing
tests over. It ended up being both simpler than the old formatter while
also producing better output.

The old formatter has a restriction where the indentation of a piece of
code must be fixed regardless of which way it splits. It can't handle:

```
// Block style, so indent argument +2:
target.property.call(
  argument
);

// Can't do block style, so indent +6:
target
    .slightlyLongerProperty
    .call(
      argument
    );
```

That leads to it sometimes producing gross output like:

```
target
    .longProperty
    .anotherLongProperty
    .method(
  wtfIsGoingOnHere,
);
```

To try to avoid that, it has lots of heuristics during Chunk
construction to try to not end up with output like that during line
splitting.

The new formatter doesn't need any of those heuristics because it can
freely choose indentation during line splitting.
  • Loading branch information
munificent committed Jan 11, 2024
1 parent 295e4c5 commit e8966c1
Show file tree
Hide file tree
Showing 13 changed files with 1,218 additions and 91 deletions.
90 changes: 25 additions & 65 deletions lib/src/front_end/ast_node_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import '../dart_formatter.dart';
import '../piece/adjacent.dart';
import '../piece/assign.dart';
import '../piece/block.dart';
import '../piece/chain.dart';
import '../piece/constructor.dart';
import '../piece/for.dart';
import '../piece/if.dart';
Expand All @@ -22,6 +21,7 @@ import '../piece/piece.dart';
import '../piece/variable.dart';
import '../source_code.dart';
import 'adjacent_builder.dart';
import 'chain_builder.dart';
import 'comment_writer.dart';
import 'delimited_list_builder.dart';
import 'piece_factory.dart';
Expand Down Expand Up @@ -412,18 +412,9 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

@override
Piece visitConstructorName(ConstructorName node) {
// If there is an import prefix and/or constructor name, then allow
// splitting before the `.`. This doesn't look good, but is consistent with
// constructor calls that don't have `new` or `const`. We allow splitting
// in the latter because there is no way to distinguish syntactically
// between a named constructor call and any other kind of method call or
// property access.
var operations = <Piece>[];

var builder = AdjacentBuilder(this);
if (node.type.importPrefix case var importPrefix?) {
builder.token(importPrefix.name);
operations.add(builder.build());
builder.token(importPrefix.period);
}

Expand All @@ -435,16 +426,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

// If this is a named constructor, the name.
if (node.name != null) {
operations.add(builder.build());
builder.token(node.period);
builder.visit(node.name);
}

// If there was a prefix or constructor name, then make a splittable piece.
// Otherwise, the current piece is a simple identifier for the name.
operations.add(builder.build());
if (operations.length == 1) return operations.first;
return ChainPiece(operations);
return builder.build();
}

@override
Expand Down Expand Up @@ -1027,35 +1015,19 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

@override
Piece visitIndexExpression(IndexExpression node) {
// TODO(tall): Allow splitting before and/or after the `[` when method
// chain formatting is fully implemented. For now, we just output the code
// so that tests of other language features that contain index expressions
// can run.
return buildPiece((b) {
b.visit(node.target);
b.token(node.leftBracket);
b.visit(node.index);
b.token(node.rightBracket);
});
Piece? targetPiece;
if (node.target case var target?) targetPiece = nodePiece(target);
return createIndexExpression(targetPiece, node);
}

@override
Piece visitInstanceCreationExpression(InstanceCreationExpression node) {
var builder = AdjacentBuilder(this);
builder.token(node.keyword, spaceAfter: true);

// If there is an import prefix and/or constructor name, then allow
// splitting before the `.`. This doesn't look good, but is consistent with
// constructor calls that don't have `new` or `const`. We allow splitting
// in the latter because there is no way to distinguish syntactically
// between a named constructor call and any other kind of method call or
// property access.
var operations = <Piece>[];

var constructor = node.constructorName;
if (constructor.type.importPrefix case var importPrefix?) {
builder.token(importPrefix.name);
operations.add(builder.build());
builder.token(importPrefix.period);
}

Expand All @@ -1066,19 +1038,13 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

// If this is a named constructor call, the name.
if (constructor.name case var name?) {
operations.add(builder.build());
builder.token(constructor.period);
builder.visit(name);
}

builder.visit(node.argumentList);
operations.add(builder.build());

if (operations.length > 1) {
return ChainPiece(operations);
} else {
return operations.first;
}
return builder.build();
}

@override
Expand Down Expand Up @@ -1195,15 +1161,23 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

@override
Piece visitMethodInvocation(MethodInvocation node) {
return buildPiece((b) {
// TODO(tall): Support splitting at `.` or `?.`. Right now we just format
// it inline so that we can use method calls in other tests.
b.visit(node.target);
b.token(node.operator);
b.visit(node.methodName);
b.visit(node.typeArguments);
b.visit(node.argumentList);
});
// If there's no target, this is a "bare" function call like "foo(1, 2)",
// or a section in a cascade.
//
// If it looks like a constructor or static call, we want to keep the
// target and method together instead of including the method in the
// subsequent method chain.
if (node.target == null || node.looksLikeStaticCall) {
return buildPiece((b) {
b.visit(node.target);
b.token(node.operator);
b.visit(node.methodName);
b.visit(node.typeArguments);
b.visit(node.argumentList);
});
}

return ChainBuilder(this, node).build();
}

@override
Expand Down Expand Up @@ -1354,14 +1328,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

@override
Piece visitPrefixedIdentifier(PrefixedIdentifier node) {
// TODO(tall): Allow splitting before the `.` when method chain formatting
// is fully implemented. For now, we just output the code so that tests
// of other language features that contain prefixed identifiers can run.
return buildPiece((b) {
b.visit(node.prefix);
b.token(node.period);
b.visit(node.identifier);
});
return ChainBuilder(this, node).build();
}

@override
Expand All @@ -1382,14 +1349,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {

@override
Piece visitPropertyAccess(PropertyAccess node) {
// TODO(tall): Allow splitting before the `.` when method chain formatting
// is fully implemented. For now, we just output the code so that tests
// of other language features that contain property accesses can run.
return buildPiece((b) {
b.visit(node.target);
b.token(node.operator);
b.visit(node.propertyName);
});
return ChainBuilder(this, node).build();
}

@override
Expand Down
206 changes: 206 additions & 0 deletions lib/src/front_end/chain_builder.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/token.dart';

import '../ast_extensions.dart';
import '../piece/chain.dart';
import '../piece/piece.dart';
import 'piece_factory.dart';

/// Creates [Chain] pieces from method calls and property accesses, along with
/// postfix operations (`!`, index operators, and function invocation
/// expressions) that follow them.
///
/// In the AST for method calls, selectors are nested bottom up such that this
/// expression:
///
/// obj.a(1)[2].c(3)
///
/// Is structured like:
///
/// .c()
/// / \
/// [] 3
/// / \
/// .a() 2
/// / \
/// obj 1
///
/// This means visiting the AST from top down visits the selectors from right
/// to left. It's easier to format if we organize them as a linear series of
/// selectors from left to right. Further, we want to organize it into a
/// two-tier hierarchy. We have an outer list of method calls and property
/// accesses. Then each of those may have one or more postfix selectors
/// attached: indexers, null-assertions, or invocations. This mirrors how they
/// are formatted.
///
/// This lets us create a single [ChainPiece] for the entire series of dotted
/// operations, so that we can control splitting them or not as a unit.
class ChainBuilder {
final PieceFactory _visitor;

/// The left-most target of the chain.
late Piece _target;

/// Whether the target expression may contain newlines when the chain is not
/// fully split. (It may always contain newlines when the chain splits.)
///
/// This is true for most expressions but false for delimited ones to avoid
/// ugly formatting like:
///
/// function(
/// argument,
/// )
/// .method();
late final bool _allowSplitInTarget;

/// The dotted property accesses and method calls following the target.
final List<ChainCall> _calls = [];

ChainBuilder(this._visitor, Expression expression) {
_unwrapCall(expression);
}

Piece build() {
// If there are no calls, there's no chain.
if (_calls.isEmpty) return _target;

// Count the number of contiguous properties at the beginning of the chain.
var leadingProperties = 0;
while (leadingProperties < _calls.length &&
_calls[leadingProperties].type == CallType.property) {
leadingProperties++;
}

// See if there is a call that we can block format. It can either be the
// very last call, if non-empty:
//
// target.property.method().last(
// argument,
// );
//
// Or the second-to-last if the last call can't split:
//
// target.property.method().penultimate(
// argument,
// ).toList();
var blockCallIndex = switch (_calls) {
[..., ChainCall(canSplit: true)] => _calls.length - 1,
[..., ChainCall(canSplit: true), ChainCall(canSplit: false)] =>
_calls.length - 2,
_ => -1,
};

return ChainPiece(_target, _calls, leadingProperties, blockCallIndex,
allowSplitInTarget: _allowSplitInTarget);
}

/// Given [expression], which is the outermost expression for some call chain,
/// recursively traverses the selectors to fill in the list of [_calls].
///
/// Initializes [_target] with the innermost subexpression that isn't a part
/// of the call chain. For example, given:
///
/// foo.bar()!.baz[0][1].bang()
///
/// This returns `foo` and fills [_calls] with:
///
/// .bar()!
/// .baz[0][1]
/// .bang()
void _unwrapCall(Expression expression) {
switch (expression) {
case Expression(looksLikeStaticCall: true):
// Don't include things that look like static method or constructor
// calls in the call chain because that tends to split up named
// constructors from their class.
_visitTarget(expression);

// Selectors.
case MethodInvocation(:var target?):
_unwrapCall(target);

var callPiece = _visitor.buildPiece((b) {
b.token(expression.operator);
b.visit(expression.methodName);
b.visit(expression.typeArguments);
b.visit(expression.argumentList);
});

var canSplit = expression.argumentList.arguments
.canSplit(expression.argumentList.rightParenthesis);
_calls.add(ChainCall(callPiece,
canSplit ? CallType.splittableCall : CallType.unsplittableCall));

case PropertyAccess(:var target?):
_unwrapCall(target);

var piece = _visitor.buildPiece((b) {
b.token(expression.operator);
b.visit(expression.propertyName);
});

_calls.add(ChainCall(piece, CallType.property));

case PrefixedIdentifier(:var prefix):
_unwrapCall(prefix);

var piece = _visitor.buildPiece((b) {
b.token(expression.period);
b.visit(expression.identifier);
});

_calls.add(ChainCall(piece, CallType.property));

// Postfix expressions.
case FunctionExpressionInvocation():
_unwrapPostfix(expression.function, (target) {
return _visitor.buildPiece((b) {
b.add(target);
b.visit(expression.typeArguments);
b.visit(expression.argumentList);
});
});

case IndexExpression():
_unwrapPostfix(expression.target!, (target) {
return _visitor.createIndexExpression(target, expression);
});

case PostfixExpression() when expression.operator.type == TokenType.BANG:
_unwrapPostfix(expression.operand, (target) {
return _visitor.buildPiece((b) {
b.add(target);
b.token(expression.operator);
});
});

default:
// Otherwise, it isn't a selector so we've reached the target.
_visitTarget(expression);
}
}

/// Creates a Piece for [target].
void _visitTarget(Expression target) {
_allowSplitInTarget = target.canBlockSplit;
_target = _visitor.nodePiece(target);
}

void _unwrapPostfix(
Expression operand, Piece Function(Piece target) createPostfix) {
_unwrapCall(operand);
// If we don't have a preceding call to hang the postfix expression off of,
// wrap it around the target expression. For example:
//
// (list + another)!
if (_calls.isEmpty) {
_target = createPostfix(_target);
} else {
_calls.last.wrapPostfix(createPostfix);
}
}
}
Loading

0 comments on commit e8966c1

Please sign in to comment.