From 041b72d6984980eecabd8a19d532ef4bf06b9090 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Wed, 16 Feb 2022 17:59:58 -0800 Subject: [PATCH 1/2] Formatting support for named arguments anywhere. I opted to add support for this while being minimally invasive to the existing style and formatting. All existing code which does not have any positional arguments after named ones retains its previous formatting. For new argument lists that use positional arguments after named ones, I try to mostly follow the existing style rules even though they can be fairly complex. In particular, it will be pretty aggressive about applying block-style formatting to function literals inside argument lists even with named args anywhere. I think that's important to support the main use case I know of for the feature which is trailing positional closures like: function(named: 1, another: 2, () { block... }); In argument lists using named args anywhere that don't have block functions, it treats all arguments like named ones. That provides nice simple formatting like: function( argument1, named: argument2, argument3, another: argument4); I think that does a good job of highlighting which arguments are named, which is what we want. Fix #1072. --- CHANGELOG.md | 4 + lib/src/argument_list_visitor.dart | 212 ++++++++++++++----------- test/splitting/arguments.stmt | 131 +++++++++++++++ test/splitting/function_arguments.stmt | 110 ++++++++++++- test/splitting/list_arguments.stmt | 24 +++ 5 files changed, 388 insertions(+), 93 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae28abd9..a73d1619 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2.2.2 + +* Format named arguments anywhere (#1072). + # 2.2.1 * Require `package:analyzer` version `2.6.0`. diff --git a/lib/src/argument_list_visitor.dart b/lib/src/argument_list_visitor.dart index 9f4c6f2b..6c13f0f6 100644 --- a/lib/src/argument_list_visitor.dart +++ b/lib/src/argument_list_visitor.dart @@ -63,87 +63,9 @@ class ArgumentListVisitor { Token leftParenthesis, Token rightParenthesis, List 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); @@ -151,9 +73,9 @@ class ArgumentListVisitor { // 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, @@ -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? _contiguousFunctions(List arguments) { + int? functionsStart; + 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 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) { @@ -295,10 +294,11 @@ class ArgumentSublist { /// The full argument list from the AST. final List _allArguments; - /// The positional arguments, in order. + /// The leading positional arguments, in order. final List _positional; - /// The named arguments, in order. + /// The named arguments, in order, and any positional arguments that follow + /// at least one named argument. final List _named; /// Maps each block argument, excluding functions, to the first token for that @@ -325,10 +325,9 @@ class ArgumentSublist { factory ArgumentSublist( List allArguments, List 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 = {}; for (var argument in arguments) { @@ -358,9 +357,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); @@ -484,6 +481,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> _splitArgumentLists( + List arguments) { + var positional = []; + var named = []; + 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. /// diff --git a/test/splitting/arguments.stmt b/test/splitting/arguments.stmt index f9ecb7cb..6a4fb03e 100644 --- a/test/splitting/arguments.stmt +++ b/test/splitting/arguments.stmt @@ -215,4 +215,135 @@ fn(named: argument,another:argument, ); fn( named: argument, another: argument, +); +>>> many arguments mixed named +method(first, name1: second, third, name2: fourth, name3: fifth, sixth, +seventh, name4: eighth, ninth, tenth); +<<< +method( + first, + name1: second, + third, + name2: fourth, + name3: fifth, + sixth, + seventh, + name4: eighth, + ninth, + tenth); +>>> wrap before first argument, named first +longFunctionIsLoooooooooooooong(name: argument, argument); +<<< +longFunctionIsLoooooooooooooong( + name: argument, argument); +>>> named first +function(name: firstArg * second, third * fourthAndLongest); +<<< +function( + name: firstArg * second, + third * fourthAndLongest); +>>> arguments, nested +someFunctionOne(someArgument, +someFunctionTwo(name1: argument, argument, name2: argument), +someFunctionTwo(argument, name: argument, argument), +someArgument, someArgument); +<<< +someFunctionOne( + someArgument, + someFunctionTwo( + name1: argument, + argument, + name2: argument), + someFunctionTwo( + argument, + name: argument, + argument), + someArgument, + someArgument); +>>> keep mixed positional and named on first line +foo(arg, foo: arg, 1, bar: 2); +<<< +foo(arg, foo: arg, 1, bar: 2); +>>> treat mixed named and positional as all named +reallyLongMethod(argument, foo: first, second); +<<< +reallyLongMethod( + argument, foo: first, second); +>>> +reallyLongMethod(argument, argument, foo: first, bar: second, third); +<<< +reallyLongMethod( + argument, + argument, + foo: first, + bar: second, + third); +>>> +reallyLongMethod(leadingNamed: first, second, baz: third); +<<< +reallyLongMethod( + leadingNamed: first, + second, + baz: third); +>>> trailing collections are indented in mixed named/positional arguments +function(argument, a: argument, argument, b: argument, +[element, element, element, element], +c: {'key': value, 'other key': value, 'third key': value}); +<<< +function( + argument, + a: argument, + argument, + b: argument, + [ + element, + element, + element, + element + ], + c: { + 'key': value, + 'other key': value, + 'third key': value + }); +>>> all trailing collections +function([element, element], name: [element, element], {'key': value, 'other key': value, 'third key': value}); +<<< +function([ + element, + element +], name: [ + element, + element +], { + 'key': value, + 'other key': value, + 'third key': value +}); +>>> non-collection non-preceding argument forces all collections to indent +function([element, element, element, element], name: argument, +{'key': value, 'other key': value, 'third key': value}, () {;}); +<<< +function( + [ + element, + element, + element, + element + ], + name: argument, + { + 'key': value, + 'other key': value, + 'third key': value + }, () { + ; +}); +>>> trailing comma with mixed named +fn(argument,name: argument ,argument , ); +<<< +fn( + argument, + name: argument, + argument, ); \ No newline at end of file diff --git a/test/splitting/function_arguments.stmt b/test/splitting/function_arguments.stmt index a94e99d6..cf860fa6 100644 --- a/test/splitting/function_arguments.stmt +++ b/test/splitting/function_arguments.stmt @@ -201,6 +201,54 @@ longFunction( ; }, c: argument); +>>> named args anywhere with leading non-function +{ + longFunction(argument, b: () {;}, c: () {;}); + longFunction(a: argument, () {;}, c: () {;}); + longFunction(a: argument, b: () {;}, () {;}); +} +<<< +{ + longFunction(argument, b: () { + ; + }, c: () { + ; + }); + longFunction(a: argument, () { + ; + }, c: () { + ; + }); + longFunction(a: argument, b: () { + ; + }, () { + ; + }); +} +>>> named args anywhere with trailing non-function +{ + longFunction(() {;}, b: () {;}, c: argument); + longFunction(a: () {;}, () {;}, c: argument); + longFunction(a: () {;}, b: () {;}, argument); +} +<<< +{ + longFunction(() { + ; + }, b: () { + ; + }, c: argument); + longFunction(a: () { + ; + }, () { + ; + }, c: argument); + longFunction(a: () { + ; + }, b: () { + ; + }, argument); +} >>> no extra indent for expression function argument with trailing comma function(() => P(p,),a: () => [a,],); <<< @@ -228,4 +276,64 @@ function( P( p, ), -); \ No newline at end of file +); +>>> unsplit named arguments with trailing positional closure +function(a: argument, b: argument, () {;}); +<<< +function(a: argument, b: argument, () { + ; +}); +>>> split named arguments with trailing positional closure +function(a: argument, b: argument, c: argument, () {;}); +<<< +function( + a: argument, + b: argument, + c: argument, () { + ; +}); +>>> unsingle split leading positional, named arguments with trailing positional closure +function(argument, b: argument, () {;}); +<<< +function(argument, b: argument, () { + ; +}); +>>> single split leading positional, named arguments with trailing positional closure +function(argument, b: argument, c: argument, () {;}); +<<< +function(argument, + b: argument, c: argument, () { + ; +}); +>>> split leading positional, named arguments with trailing positional closure +function(argument, argument, argument, argument, argument, b: argument, c: argument, d: argument, () {;}); +<<< +function(argument, argument, argument, + argument, argument, + b: argument, + c: argument, + d: argument, () { + ; +}); +>>> multiple trailing positional closures +function(argument, b: argument, c: argument, () {;}, () {;}); +<<< +function(argument, + b: argument, c: argument, () { + ; +}, () { + ; +}); +>>> mixed named and positional trailing closures +function(argument, b: argument, c: argument, () {;}, d: () {;}, () {;}, e: () {;}); +<<< +function(argument, + b: argument, c: argument, () { + ; +}, d: () { + ; +}, () { + ; +}, e: () { + ; +}); \ No newline at end of file diff --git a/test/splitting/list_arguments.stmt b/test/splitting/list_arguments.stmt index ebb0248e..759510d0 100644 --- a/test/splitting/list_arguments.stmt +++ b/test/splitting/list_arguments.stmt @@ -233,6 +233,30 @@ method( element ], c: third); +>>> mixed named and positional forces nesting +method(a:first,[element, element, element, element],c:third); +<<< +method( + a: first, + [ + element, + element, + element, + element + ], + c: third); +>>> mixed named and positional forces nesting +method(a:first,b:[element, element, element, element],third); +<<< +method( + a: first, + b: [ + element, + element, + element, + element + ], + third); >>> nothing but named list args does not nest longFunctionName(a: [element, element, element, element], b: [element, element, element, element], c: [element, element, element, element]); From 0330256e3b48bededef628da9e41171e84ba360b Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Wed, 23 Feb 2022 14:16:30 -0800 Subject: [PATCH 2/2] Rewrite doc comments. --- lib/src/argument_list_visitor.dart | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/src/argument_list_visitor.dart b/lib/src/argument_list_visitor.dart index 6c13f0f6..d839600a 100644 --- a/lib/src/argument_list_visitor.dart +++ b/lib/src/argument_list_visitor.dart @@ -294,11 +294,15 @@ class ArgumentSublist { /// The full argument list from the AST. final List _allArguments; - /// The leading 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 _positional; - /// The named arguments, in order, and any positional arguments that follow - /// at least one named argument. + /// The named arguments, in order. If there are any named arguments that occur + /// before positional arguments, then all arguments are treated as named and + /// end up in this list. final List _named; /// Maps each block argument, excluding functions, to the first token for that