Skip to content

Formatting support for named arguments anywhere. #1094

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

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Formatting support for named arguments anywhere. #1094

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
041b72d
Formatting support for named arguments anywhere.
munificent Feb 17, 2022
0330256
Rewrite doc comments.
munificent Feb 23, 2022
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 2.2.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you normally choose when to bump the feature version? Support for a new language feature could justify it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm usually pretty conservative. This isn't a breaking change and doesn't really add any new "features" (in terms of the CLI arguments or library API). Also, it doesn't change the formatting of any existing code. That sounds like a point release to me, even though it's significant in that it adds support for new syntax. But since no code using that syntax exists in the wild, from the user's perspective, there's no visible change in dart_style at all.


* Format named arguments anywhere (#1072).

# 2.2.1

* Require `package:analyzer` version `2.6.0`.
Expand Down
216 changes: 124 additions & 92 deletions lib/src/argument_list_visitor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,97 +63,19 @@ class ArgumentListVisitor {
Token leftParenthesis,
Token rightParenthesis,
List<Expression> arguments) {
// Look for a single contiguous range of block function arguments.
int? functionsStart;
int? functionsEnd;

for (var i = 0; i < arguments.length; i++) {
var argument = arguments[i];
if (_isBlockFunction(argument)) {
functionsStart ??= i;
var functionRange = _contiguousFunctions(arguments);

// The functions must be one contiguous section.
if (functionsEnd != null && functionsEnd != i) {
functionsStart = null;
functionsEnd = null;
break;
}

functionsEnd = i + 1;
}
}

// Edge case: If all of the arguments are named, but they aren't all
// functions, then don't handle the functions specially. A function with a
// bunch of named arguments tends to look best when they are all lined up,
// even the function ones (unless they are all functions).
//
// Prefers:
//
// function(
// named: () {
// something();
// },
// another: argument);
//
// Over:
//
// function(named: () {
// something();
// },
// another: argument);
if (functionsStart != null &&
arguments[0] is NamedExpression &&
(functionsStart > 0 || functionsEnd! < arguments.length)) {
functionsStart = null;
}

// Edge case: If all of the function arguments are named and there are
// other named arguments that are "=>" functions, then don't treat the
// block-bodied functions specially. In a mixture of the two function
// styles, it looks cleaner to treat them all like normal expressions so
// that the named arguments line up.
if (functionsStart != null &&
arguments[functionsStart] is NamedExpression) {
bool isArrow(NamedExpression named) {
var expression = named.expression;

if (expression is FunctionExpression) {
return expression.body is ExpressionFunctionBody;
}

return false;
}

for (var i = 0; i < functionsStart!; i++) {
var argument = arguments[i];
if (argument is! NamedExpression) continue;

if (isArrow(argument)) {
functionsStart = null;
break;
}
}

for (var i = functionsEnd!; i < arguments.length; i++) {
if (isArrow(arguments[i] as NamedExpression)) {
functionsStart = null;
break;
}
}
}

if (functionsStart == null) {
if (functionRange == null) {
// No functions, so there is just a single argument list.
return ArgumentListVisitor._(visitor, leftParenthesis, rightParenthesis,
arguments, ArgumentSublist(arguments, arguments), null, null);
}

// Split the arguments into two independent argument lists with the
// functions in the middle.
var argumentsBefore = arguments.take(functionsStart).toList();
var functions = arguments.sublist(functionsStart, functionsEnd);
var argumentsAfter = arguments.skip(functionsEnd!).toList();
var argumentsBefore = arguments.take(functionRange[0]).toList();
var functions = arguments.sublist(functionRange[0], functionRange[1]);
var argumentsAfter = arguments.skip(functionRange[1]).toList();

return ArgumentListVisitor._(
visitor,
Expand Down Expand Up @@ -224,6 +146,83 @@ class ArgumentListVisitor {
if (_isSingle) _visitor.builder.endSpan();
}

/// Look for a single contiguous range of block function [arguments] that
/// should receive special formatting.
///
/// Returns a list of (start, end] indexes if found, otherwise returns `null`.
static List<int>? _contiguousFunctions(List<Expression> arguments) {
int? functionsStart;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is essentially unchanged from line 66 on. The main difference is that the old code used to use arguments[0] is NamedExpression as a clever check to see if all arguments are named. That doesn't work with named args anywhere so now it iterates the list and checks them all.

var functionsEnd = -1;

// Find the range of block function arguments, if any.
for (var i = 0; i < arguments.length; i++) {
var argument = arguments[i];
if (_isBlockFunction(argument)) {
functionsStart ??= i;

// The functions must be one contiguous section.
if (functionsEnd != -1 && functionsEnd != i) return null;

functionsEnd = i + 1;
}
}

if (functionsStart == null) return null;

// Edge case: If all of the arguments are named, but they aren't all
// functions, then don't handle the functions specially. A function with a
// bunch of named arguments tends to look best when they are all lined up,
// even the function ones (unless they are all functions).
//
// Prefers:
//
// function(
// named: () {
// something();
// },
// another: argument);
//
// Over:
//
// function(named: () {
// something();
// },
// another: argument);
if (_isAllNamed(arguments) &&
(functionsStart > 0 || functionsEnd < arguments.length)) {
return null;
}

// Edge case: If all of the function arguments are named and there are
// other named arguments that are "=>" functions, then don't treat the
// block-bodied functions specially. In a mixture of the two function
// styles, it looks cleaner to treat them all like normal expressions so
// that the named arguments line up.
if (_isAllNamed(arguments.sublist(functionsStart, functionsEnd))) {
bool isNamedArrow(Expression expression) {
if (expression is! NamedExpression) return false;
expression = expression.expression;

return expression is FunctionExpression &&
expression.body is ExpressionFunctionBody;
}

for (var i = 0; i < functionsStart; i++) {
if (isNamedArrow(arguments[i])) return null;
}

for (var i = functionsEnd; i < arguments.length; i++) {
if (isNamedArrow(arguments[i])) return null;
}
}

return [functionsStart, functionsEnd];
}

/// Returns `true` if every expression in [arguments] is named.
static bool _isAllNamed(List<Expression> arguments) =>
arguments.every((argument) => argument is NamedExpression);

/// Returns `true` if [expression] is a [FunctionExpression] with a non-empty
/// block body.
static bool _isBlockFunction(Expression expression) {
Expand Down Expand Up @@ -295,10 +294,15 @@ class ArgumentSublist {
/// The full argument list from the AST.
final List<Expression> _allArguments;

/// The positional arguments, in order.
/// If all positional arguments occur before all named arguments, then this
/// contains the positional arguments, in order. Otherwise (there are no
/// positional arguments or they are interleaved with named ones), this is
/// empty.
final List<Expression> _positional;

/// The named arguments, in order.
/// The named arguments, in order. If there are any named arguments that occur
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] https://dart.dev/guides/language/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph

/// before positional arguments, then all arguments are treated as named and
/// end up in this list.
final List<Expression> _named;

/// Maps each block argument, excluding functions, to the first token for that
Expand All @@ -325,10 +329,9 @@ class ArgumentSublist {

factory ArgumentSublist(
List<Expression> allArguments, List<Expression> arguments) {
// Assumes named arguments follow all positional ones.
var positional =
arguments.takeWhile((arg) => arg is! NamedExpression).toList();
var named = arguments.skip(positional.length).toList();
var argumentLists = _splitArgumentLists(arguments);
var positional = argumentLists[0];
var named = argumentLists[1];

var blocks = <Expression, Token>{};
for (var argument in arguments) {
Expand Down Expand Up @@ -358,9 +361,7 @@ class ArgumentSublist {
if (trailingBlocks != blocks.length) trailingBlocks = 0;

// Ignore any blocks in the middle of the argument list.
if (leadingBlocks == 0 && trailingBlocks == 0) {
blocks.clear();
}
if (leadingBlocks == 0 && trailingBlocks == 0) blocks.clear();

return ArgumentSublist._(
allArguments, positional, named, blocks, leadingBlocks, trailingBlocks);
Expand Down Expand Up @@ -484,6 +485,37 @@ class ArgumentSublist {
}
}

/// Splits [arguments] into two lists: the list of leading positional
/// arguments and the list of trailing named arguments.
///
/// If positional arguments are interleaved with the named arguments then
/// all arguments are treat as named since that provides simpler, consistent
/// output.
///
/// Returns a list of two lists: the positional arguments then the named ones.
static List<List<Expression>> _splitArgumentLists(
List<Expression> arguments) {
var positional = <Expression>[];
var named = <Expression>[];
var inNamed = false;
for (var argument in arguments) {
if (argument is NamedExpression) {
inNamed = true;
} else if (inNamed) {
// Got a positional argument after a named one.
return [[], arguments];
}

if (inNamed) {
named.add(argument);
} else {
positional.add(argument);
}
}

return [positional, named];
}

/// If [expression] can be formatted as a block, returns the token that opens
/// the block, such as a collection's bracket.
///
Expand Down
Loading