Smarter splitting around named collection arguments.

lib/src/argument_list_visitor.dart

 // Copyright (c) 2014, 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.
 
 library dart_style.src.argument_list_visitor;
 
+import 'dart:math' as math;
+
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/src/generated/scanner.dart';
 
 import 'chunk.dart';
 import 'rule/argument.dart';
 import 'rule/rule.dart';
 import 'source_visitor.dart';
 
 /// Helper class for [SourceVisitor] that handles visiting and writing an
 /// [ArgumentList], including all of the special code needed to handle function
@@ skipping 237 matching lines @@
 
     // Count the trailing arguments that are collection literals.
     var trailingCollections = 0;
     if (leadingCollections != arguments.length) {
       for (var argument in arguments.reversed) {
         if (!collections.containsKey(argument)) break;
         trailingCollections++;
       }
     }
 
-    // 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;
     if (trailingCollections != collections.length) trailingCollections = 0;
 
     // Ignore any collections in the middle of the argument list.
     if (leadingCollections == 0 && trailingCollections == 0) {
       collections.clear();
     }
 
@@ skipping 17 matching lines @@
   PositionalRule _visitPositional(SourceVisitor visitor) {
     if (_positional.isEmpty) return null;
 
     // Allow splitting after "(".
     var rule;
     if (_positional.length == 1) {
       rule = new SinglePositionalRule(_collectionRule,
           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);
+          _collectionRule, leadingCollections, trailingCollections);
     }
 
-    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);
-      }
-    }
-
-    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();
   }
 
   void _visitArgument(
       SourceVisitor visitor, ArgumentRule rule, Expression argument) {
     // If we're about to write a collection argument, handle it specially.
     if (_collections.containsKey(argument)) {
       if (rule != null) rule.beforeCollection();
 
       // Tell it to use the rule we've already created.
@@ skipping 38 matching lines @@
 
     // TODO(rnystrom): Should we step into parenthesized expressions?
 
     if (expression is ListLiteral) return expression.leftBracket;
     if (expression is MapLiteral) return expression.leftBracket;
 
     // Not a collection literal.
     return null;
   }
 }