Use a more general mechanism for tracking constraints between rules. (#1122)

* Use a more general mechanism for tracking constraints between rules.

Rules were initially decoupled from each other. Then I added _implies
to handle subexpressions forcing outer expressions to split. Then I
added some object-oriented "constrain()" and "addConstrainedRules()"
API so that Rules could dynamically add other ad-hoc constraints.

At this point, it's better to just have a single general mechanism for
expressing constraints between rules that can subsume both of those.
This does that. I think the resulting code is simpler. It is also easier
to extend for other kinds of constraints in the future.

* Add a missing argument constraint.

This (interestingly) was not caught by any existing tests, but did show
up when I ran the formatter on a big corpus. Added a test to catch it
in the future.

* Apply review feedback.

Author: munificent

lib/src/argument_list_visitor.dart

@@ -389,7 +389,10 @@ class ArgumentSublist {
     // Only count the blocks in the positional rule.
     var leadingBlocks = math.min(_leadingBlocks, _positional.length);
     var trailingBlocks = math.max(_trailingBlocks - _named.length, 0);
-    var rule = PositionalRule(_blockRule, leadingBlocks, trailingBlocks);
+    var rule = PositionalRule(_blockRule,
+        argumentCount: _positional.length,
+        leadingCollections: leadingBlocks,
+        trailingCollections: trailingBlocks);
     _visitArguments(visitor, _positional, rule);
 
     return rule;
@@ -406,7 +409,7 @@ class ArgumentSublist {
 
     // Let the positional args force the named ones to split.
     if (positionalRule != null) {
-      positionalRule.setNamedArgsRule(namedRule);
+      positionalRule.addNamedArgsConstraints(namedRule);
     }
 
     _visitArguments(visitor, _named, namedRule);

lib/src/call_chain_visitor.dart

@@ -186,7 +186,7 @@ class CallChainVisitor {
 
     if (splitOnTarget) {
       if (_properties.length > 1) {
-        _propertyRule = PositionalRule(null, 0, 0);
+        _propertyRule = PositionalRule(null, argumentCount: _properties.length);
         _visitor.builder.startLazyRule(_propertyRule);
       } else {
         _enableRule(lazy: true);
@@ -202,7 +202,7 @@ class CallChainVisitor {
       _properties.single.write(this);
     } else if (_properties.length > 1) {
       if (!splitOnTarget) {
-        _propertyRule = PositionalRule(null, 0, 0);
+        _propertyRule = PositionalRule(null, argumentCount: _properties.length);
         _visitor.builder.startRule(_propertyRule);
       }
 
@@ -377,7 +377,7 @@ class CallChainVisitor {
 
     // If the properties split, force the calls to split too.
     var rule = Rule();
-    _propertyRule?.setNamedArgsRule(rule);
+    _propertyRule?.addNamedArgsConstraints(rule);
 
     if (lazy) {
       _visitor.builder.startLazyRule(rule);

lib/src/chunk_builder.dart

@@ -462,7 +462,7 @@ class ChunkBuilder {
     // See if any of the rules that contain this one care if it splits.
     for (var outer in _rules) {
       if (!outer.splitsOnInnerRules) continue;
-      rule.imply(outer);
+      rule.constrainWhenSplit(outer);
     }
     _rules.add(rule);
   }

lib/src/rule/argument.dart

@@ -9,9 +9,6 @@ abstract class ArgumentRule extends Rule {
   /// The chunks prior to each positional argument.
   final List<Chunk?> _arguments = [];
 
-  /// The rule used to split collections in the argument list, if any.
-  Rule? _collectionRule;
-
   /// The number of leading collection arguments.
   ///
   /// This and [_trailingCollections] cannot both be positive. If every
@@ -34,20 +31,7 @@ abstract class ArgumentRule extends Rule {
   @override
   bool get splitsOnInnerRules => _trackInnerRules;
 
-  ArgumentRule(this._collectionRule, this._leadingCollections,
-      this._trailingCollections);
-
-  @override
-  void addConstrainedRules(Set<Rule> rules) {
-    super.addConstrainedRules(rules);
-    if (_collectionRule != null) rules.add(_collectionRule!);
-  }
-
-  @override
-  void forgetUnusedRules() {
-    super.forgetUnusedRules();
-    if (_collectionRule?.index == null) _collectionRule = null;
-  }
+  ArgumentRule._(this._leadingCollections, this._trailingCollections);
 
   /// Remembers [chunk] as containing the split that occurs right before an
   /// argument in the list.
@@ -86,17 +70,47 @@ abstract class ArgumentRule extends Rule {
 /// splits before all of the non-collection arguments, but does not split
 /// before the collections, so that they can split internally.
 class PositionalRule extends ArgumentRule {
-  /// If there are named arguments following these positional ones, this will
-  /// be their rule.
-  Rule? _namedArgsRule;
-
   /// Creates a new rule for a positional argument list.
   ///
-  /// If [_collectionRule] is given, it is the rule used to split the collection
-  /// arguments in the list.
-  PositionalRule(
-      Rule? collectionRule, int leadingCollections, int trailingCollections)
-      : super(collectionRule, leadingCollections, trailingCollections);
+  /// [argumentCount] is the number of arguments that will be added to the rule
+  /// by later calls to [beforeArgument()].
+  ///
+  /// If [collectionRule] is given, it is the rule used to split the collection
+  /// arguments in the list. It must be provided if [leadingCollections] or
+  /// [trailingCollections] is non-zero.
+  PositionalRule(Rule? collectionRule,
+      {required int argumentCount,
+      int leadingCollections = 0,
+      int trailingCollections = 0})
+      : super._(leadingCollections, trailingCollections) {
+    if (collectionRule != null) {
+      // Don't split inside collections if there are leading collections and
+      // we split before the first argument.
+      if (leadingCollections > 0) {
+        addConstraint(1, collectionRule, Rule.unsplit);
+      }
+
+      // If we're only splitting before the non-collection arguments, the
+      // intent is to split inside the collections, so force that here.
+      if (leadingCollections > 0 || trailingCollections > 0) {
+        addConstraint(argumentCount + 1, collectionRule, 1);
+      }
+
+      // Split before a single argument. If it's in the middle of the collection
+      // arguments, don't allow them to split.
+      for (var argument = 0; argument < leadingCollections; argument++) {
+        var value = argumentCount - argument + 1;
+        addConstraint(value, collectionRule, Rule.unsplit);
+      }
+
+      for (var argument = argumentCount - trailingCollections;
+          argument < argumentCount;
+          argument++) {
+        var value = argumentCount - argument + 1;
+        addConstraint(value, collectionRule, Rule.unsplit);
+      }
+    }
+  }
 
   @override
   int get numValues {
@@ -118,18 +132,6 @@ class PositionalRule extends ArgumentRule {
     return result;
   }
 
-  @override
-  void addConstrainedRules(Set<Rule> rules) {
-    super.addConstrainedRules(rules);
-    if (_namedArgsRule != null) rules.add(_namedArgsRule!);
-  }
-
-  @override
-  void forgetUnusedRules() {
-    super.forgetUnusedRules();
-    if (_namedArgsRule?.index == null) _namedArgsRule = null;
-  }
-
   @override
   bool isSplitAtValue(int value, Chunk chunk) {
     // Split only before the first argument. Keep the entire argument list
@@ -162,78 +164,19 @@ class PositionalRule extends ArgumentRule {
     return true;
   }
 
-  /// Remembers that [rule] is the [Rule] immediately following this positional
-  /// positional argument list.
+  /// Builds any constraints from this positional argument rule onto the [rule]
+  /// used for the subsequent named arguments in the same argument list.
   ///
-  /// This is normally a [NamedRule] but [PositionalRule] is also used for the
-  /// property accesses at the beginning of a call chain, in which case this
+  /// The [rule] is normally a [NamedRule] but [PositionalRule] is also used for
+  /// the property accesses at the beginning of a call chain, in which case this
   /// is just a [SimpleRule].
-  void setNamedArgsRule(Rule rule) {
-    _namedArgsRule = rule;
-  }
-
-  /// Constrains the named argument list to at least move to the next line if
-  /// there are any splits in the positional arguments. Prevents things like:
-  ///
-  ///      function(
-  ///          argument,
-  ///          argument, named: argument);
-  @override
-  int? constrain(int value, Rule other) {
-    var constrained = super.constrain(value, other);
-    if (constrained != null) return constrained;
+  void addNamedArgsConstraints(Rule rule) {
+    // If the positional args are one-per-line, the named args are too.
+    constrainWhenFullySplit(rule);
 
-    // Handle the relationship between the positional and named args.
-    if (other == _namedArgsRule) {
-      // If the positional args are one-per-line, the named args are too.
-      if (value == fullySplitValue) return _namedArgsRule!.fullySplitValue;
-
-      // Otherwise, if there is any split in the positional arguments, don't
-      // allow the named arguments on the same line as them.
-      if (value != 0) return -1;
-    }
-
-    // Decide how to constrain the collection rule.
-    if (other != _collectionRule) return null;
-
-    // If all of the collections are in the named arguments, [_collectionRule]
-    // will not be null, but we don't have to handle it.
-    if (_leadingCollections == 0 && _trailingCollections == 0) return null;
-
-    // If we aren't splitting any args, we can split the collection.
-    if (value == Rule.unsplit) return null;
-
-    // Split only before the first argument.
-    if (value == 1) {
-      if (_leadingCollections > 0) {
-        // We are splitting before a collection, so don't let it split
-        // internally.
-        return Rule.unsplit;
-      } else {
-        // The split is outside of the collections so they can split or not.
-        return null;
-      }
-    }
-
-    // Split before a single argument. If it's in the middle of the collection
-    // arguments, don't allow them to split.
-    if (value <= _arguments.length) {
-      var argument = _arguments.length - value + 1;
-      if (argument < _leadingCollections ||
-          argument >= _arguments.length - _trailingCollections) {
-        return Rule.unsplit;
-      }
-
-      return null;
-    }
-
-    // Only split before the non-collection arguments. This case only comes into
-    // play when we do want to split the collection, so force that here.
-    if (value == _arguments.length + 1) return 1;
-
-    // Split before all of the arguments, even the collections. We'll allow
-    // them to split but indent their bodies if they do.
-    return null;
+    // Otherwise, if there is any split in the positional arguments, don't
+    // allow the named arguments on the same line as them.
+    addRangeConstraint(1, fullySplitValue, rule, Rule.mustSplit);
   }
 
   @override
@@ -249,9 +192,23 @@ class NamedRule extends ArgumentRule {
   @override
   int get numValues => 3;
 
+  /// Creates a new rule for a named argument list.
+  ///
+  /// [argumentCount] is the number of arguments that will be added to the rule
+  /// by later calls to [beforeArgument()].
+  ///
+  /// If [collectionRule] is given, it is the rule used to split the collection
+  /// arguments in the list. It must be provided if [leadingCollections] or
+  /// [trailingCollections] is non-zero.
   NamedRule(
       Rule? collectionRule, int leadingCollections, int trailingCollections)
-      : super(collectionRule, leadingCollections, trailingCollections);
+      : super._(leadingCollections, trailingCollections) {
+    if (leadingCollections > 0 || trailingCollections > 0) {
+      // Split only before the first argument. Don't allow the collections to
+      // split.
+      addConstraint(1, collectionRule!, Rule.unsplit);
+    }
+  }
 
   @override
   bool isSplitAtValue(int value, Chunk chunk) {
@@ -262,30 +219,6 @@ class NamedRule extends ArgumentRule {
     return true;
   }
 
-  @override
-  int? constrain(int value, Rule other) {
-    var constrained = super.constrain(value, other);
-    if (constrained != null) return constrained;
-
-    // Decide how to constrain the collection rule.
-    if (other != _collectionRule) return null;
-
-    // If all of the collections are in the named arguments, [_collectionRule]
-    // will not be null, but we don't have to handle it.
-    if (_leadingCollections == 0 && _trailingCollections == 0) return null;
-
-    // If we aren't splitting any args, we can split the collection.
-    if (value == Rule.unsplit) return null;
-
-    // Split only before the first argument. Don't allow the collections to
-    // split.
-    if (value == 1) return Rule.unsplit;
-
-    // Split before all of the arguments, even the collections. We'll allow
-    // them to split but indent their bodies if they do.
-    return null;
-  }
-
   @override
   String toString() => 'Named${super.toString()}';
 }