Smarter splitting around named collection arguments.

Fix #394.

R=kevmoo@google.com

Review URL: https://codereview.chromium.org//1493553002 .

CHANGELOG.md

@@ -5,6 +5,7 @@
 * Don't collapse states that differ by unbound rule constraints (#424).
 * Better handling for functions in method chains (#367, #398).
 * Better handling of large parameter metadata annotations (#387, #444).
+* Smarter splitting around collections in named parameters (#394).
 * Split calls if properties in a chain split (#399).
 * Don't allow splitting inside empty functions (#404).
 * Consider a rule live if it constrains a rule in the overflow line (#407).

lib/src/argument_list_visitor.dart

@@ -4,6 +4,8 @@
 
 library dart_style.src.argument_list_visitor;
 
+import 'dart:math' as math;
+
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/src/generated/scanner.dart';
 
@@ -261,16 +263,6 @@ class ArgumentSublist {
       }
     }
 
-    // If only some of the named arguments are collections, treat none of them
-    // specially. Avoids cases like:
-    //
-    //     function(
-    //         a: arg,
-    //         b: [
-    //       ...
-    //     ]);
-    if (trailingCollections < named.length) trailingCollections = 0;
-
     // Collections must all be a prefix or suffix of the argument list (and not
     // both).
     if (leadingCollections != collections.length) leadingCollections = 0;
@@ -308,69 +300,64 @@ class ArgumentSublist {
           splitsOnInnerRules: _allArguments.length > 1 &&
               !_collections.containsKey(_positional.first));
     } else {
-      // Only count the positional bodies in the positional rule.
-      var leadingPositional = _leadingCollections;
-      if (_leadingCollections == _positional.length + _named.length) {
-        leadingPositional -= _named.length;
-      }
-
-      var trailingPositional = _trailingCollections - _named.length;
+      // Only count the collections in the positional rule.
+      var leadingCollections =
+          math.min(_leadingCollections, _positional.length);
+      var trailingCollections =
+          math.max(_trailingCollections - _named.length, 0);
       rule = new MultiplePositionalRule(
-          _collectionRule, leadingPositional, trailingPositional);
-    }
-
-    visitor.builder.startRule(rule);
-
-    _previousSplit =
-        visitor.builder.split(space: !_isFirstArgument(_positional.first));
-    rule.beforeArgument(_previousSplit);
-
-    // Try to not split the arguments.
-    visitor.builder.startSpan(Cost.positionalArguments);
-
-    for (var argument in _positional) {
-      _visitArgument(visitor, rule, argument);
-
-      // Positional arguments split independently.
-      if (argument != _positional.last) {
-        _previousSplit = visitor.split();
-        rule.beforeArgument(_previousSplit);
-      }
+          _collectionRule, leadingCollections, trailingCollections);
     }
 
-    visitor.builder.endSpan();
-    visitor.builder.endRule();
-
+    _visitArguments(visitor, _positional, rule);
     return rule;
   }
 
   /// Writes the named arguments, if any.
-  void _visitNamed(SourceVisitor visitor, PositionalRule rule) {
+  void _visitNamed(SourceVisitor visitor, PositionalRule positionalRule) {
     if (_named.isEmpty) return;
 
-    var positionalRule = rule;
-    var namedRule = new NamedRule(
-        containsCollections: _leadingCollections > _positional.length ||
-            _trailingCollections > 0);
-    visitor.builder.startRule(namedRule);
+    // Only count the collections in the named rule.
+    var leadingCollections =
+        math.max(_leadingCollections - _positional.length, 0);
+    var trailingCollections = math.min(_trailingCollections, _named.length);
+    var namedRule =
+        new NamedRule(_collectionRule, leadingCollections, trailingCollections);
 
     // Let the positional args force the named ones to split.
     if (positionalRule != null) {
       positionalRule.setNamedArgsRule(namedRule);
     }
 
-    // Split before the first named argument.
+    _visitArguments(visitor, _named, namedRule);
+  }
+
+  void _visitArguments(
+      SourceVisitor visitor, List<Expression> arguments, ArgumentRule rule) {
+    visitor.builder.startRule(rule);
+
+    // Split before the first argument.
     _previousSplit =
-        visitor.builder.split(space: !_isFirstArgument(_named.first));
-    namedRule.beforeArguments(_previousSplit);
+        visitor.builder.split(space: !_isFirstArgument(arguments.first));
+    rule.beforeArgument(_previousSplit);
 
-    for (var argument in _named) {
-      _visitArgument(visitor, namedRule, argument);
+    // Try to not split the positional arguments.
+    if (arguments == _positional) {
+      visitor.builder.startSpan(Cost.positionalArguments);
+    }
+
+    for (var argument in arguments) {
+      _visitArgument(visitor, rule, argument);
 
       // Write the split.
-      if (argument != _named.last) _previousSplit = visitor.split();
+      if (argument != arguments.last) {
+        _previousSplit = visitor.split();
+        rule.beforeArgument(_previousSplit);
+      }
     }
 
+    if (arguments == _positional) visitor.builder.endSpan();
+
     visitor.builder.endRule();
   }
 

lib/src/call_chain_visitor.dart

@@ -8,6 +8,7 @@ import 'package:analyzer/analyzer.dart';
 
 import 'argument_list_visitor.dart';
 import 'rule/argument.dart';
+import 'rule/rule.dart';
 import 'source_visitor.dart';
 
 /// Helper class for [SourceVisitor] that handles visiting and writing a
@@ -307,22 +308,20 @@ class CallChainVisitor {
 
     // Don't split right after a non-empty curly-bodied function.
     if (expression is FunctionExpression) {
-      var function = expression as FunctionExpression;
+      if (expression.body is! BlockFunctionBody) return false;
 
-      if (function.body is! BlockFunctionBody) return false;
-
-      return (function.body as BlockFunctionBody).block.statements.isEmpty;
+      return (expression.body as BlockFunctionBody).block.statements.isEmpty;
     }
 
     // If the expression ends in an argument list, base the splitting on the
     // last argument.
     var argumentList;
     if (expression is MethodInvocation) {
-      argumentList = (expression as MethodInvocation).argumentList;
+      argumentList = expression.argumentList;
     } else if (expression is InstanceCreationExpression) {
-      argumentList = (expression as InstanceCreationExpression).argumentList;
+      argumentList = expression.argumentList;
     } else if (expression is FunctionExpressionInvocation) {
-      argumentList = (expression as FunctionExpressionInvocation).argumentList;
+      argumentList = expression.argumentList;
     }
 
     // Any other kind of expression always splits.
@@ -420,7 +419,7 @@ class CallChainVisitor {
     if (_ruleEnabled) return;
 
     // If the properties split, force the calls to split too.
-    var rule = new NamedRule();
+    var rule = new Rule();
     if (_propertyRule != null) _propertyRule.setNamedArgsRule(rule);
 
     if (lazy) {

lib/src/dart_formatter.dart

@@ -110,7 +110,7 @@ class DartFormatter {
       // Make sure we consumed all of the source.
       var token = node.endToken.next;
       if (token.type != TokenType.EOF) {
-        var error = new AnalysisError.con2(
+        var error = new AnalysisError(
             stringSource,
             token.offset,
             math.max(token.length, 1),

lib/src/debug.dart

@@ -9,7 +9,6 @@ import 'dart:math' as math;
 
 import 'chunk.dart';
 import 'line_splitting/rule_set.dart';
-import 'rule/rule.dart';
 
 /// Set this to `true` to turn on diagnostic output while building chunks.
 bool traceChunkBuilder = false;

lib/src/line_splitting/solve_state.dart

@@ -97,7 +97,7 @@ class SolveState {
   ///
   /// It's important to track this, because we can't allow to states to overlap
   /// if one permits more values for some unbound rule than the other does.
-  Map<Rule, List<int>> _unboundConstraints;
+  Map<Rule, Set<int>> _unboundConstraints;
 
   /// The bound rules that appear inside lines also containing unbound rules.
   ///