Split metadata on parameters independently. (#1217)

Also, don't indent parameters with metadata annotations.

Before this change, the formatter indents any parameter that contains
metadata annotations that end up splitting:

    function(
        @required
            int n,
        @deprecated('Some long deprecation string.')
            String s) {
      ...
    }

It also treats all parameters in a parameter list as a unit when
deciding how to split their metadata. If any annotation in any parameter
splits, then they all do. That's why there's a split after `@required`
in the above example.

The intent of both of these is so that a parameter with unsplit metadata
doesn't get confused for an annotation on the next parameter:

    function(
        @FIRST('some string') int n,
        @second('another string forces a split')
        String s) {
      ...
    }

Note here that there are two parameters, `n`, and `s`, and not just a
single parameter `s` with two annotations.

Unfortunately line splitting all of the parameters as a unit is very bad
for performance when there is a large number of parameters (#1212).
Also, it's not very helpful. In practice, parameter metadata is rare and
most parameters that have any annotations only have one.

And the indentation is just strange looking and inconsistent with how
annotations are formatted elsewhere. It also means that parameters with
split metadata don't align with parameters that don't have metadata.

This change determines whether each parameter's annotations should split
independently from the other parameters' and removes that indentation.
The above example becomes:

    function(
        @required int n,
        @deprecated('Some long deprecation string.')
        String s) {
      ...
    }

This improves performance on large parameter lists and I think looks
better on real-world examples. I ran it on a large corpus (2,112,352
lines in 6,911 files) and I think the impact is small enough to not go
through the full change process:

293 insertions + 443 deletions = 736 changes
1 changed line for every 2870.04 lines of code
0.3484 changed lines for every 1,000 lines of code

The full diff is: https://gist.github.com/munificent/1dc7361438934a3587f6149049682e29

Fix #1212.

Author: munificent

CHANGELOG.md

@@ -1,5 +1,9 @@
 # 2.3.2-dev
 
+* Don't indent parameters that have metadata annotations. Instead, align them
+  with the metadata and other parameters.
+* Allow metadata annotations on parameters to split independently of annotations
+  on other parameters (#1212).
 * Don't split before `.` following a record literal (#1213).
 * Don't force split on a line comment before a switch expression case (#1215).
 

lib/src/chunk_builder.dart

@@ -60,23 +60,6 @@ class ChunkBuilder {
   /// Whether the next chunk should be flush left.
   bool _pendingFlushLeft = false;
 
-  /// Whether subsequent hard splits should be allowed to divide for line
-  /// splitting.
-  ///
-  /// Most rules used by multiple chunks will never have a hard split on a
-  /// chunk between two chunks using that rule. That means when we see a hard
-  /// split, we can line split the chunks before and after it independently.
-  ///
-  /// However, there are a couple of places where a single rule spans
-  /// multiple chunks where hard splits also appear interleaved between them.
-  /// Currently, that's the rule for splitting switch case bodies, and the
-  /// rule for parameter list metadata.
-  ///
-  /// In those circumstances, we mark the chunk with the hard split as not
-  /// being allowed to divide. That way, all of the chunks using the rule are
-  /// split together.
-  bool _pendingPreventDivide = false;
-
   /// Whether the most recently written output was a comment.
   bool _afterComment = false;
 
@@ -173,7 +156,6 @@ class ChunkBuilder {
 
     _nesting.commitNesting();
     _afterComment = false;
-    _pendingPreventDivide = false;
   }
 
   /// Writes one or two hard newlines.
@@ -186,14 +168,10 @@ class ChunkBuilder {
   /// nesting. If [nest] is `true` then the next line will use expression
   /// nesting.
   void writeNewline(
-      {bool isDouble = false,
-      bool flushLeft = false,
-      bool nest = false,
-      bool preventDivide = false}) {
+      {bool isDouble = false, bool flushLeft = false, bool nest = false}) {
     _pendingNewlines = isDouble ? 2 : 1;
     _pendingFlushLeft = flushLeft;
     _pendingNested = nest;
-    _pendingPreventDivide |= preventDivide;
   }
 
   /// Writes a space before the subsequent non-whitespace text.
@@ -877,11 +855,7 @@ class ChunkBuilder {
           isHard: isHard, isDouble: isDouble, space: space);
     }
 
-    if (chunk.rule.isHardened) {
-      _handleHardSplit();
-
-      if (_pendingPreventDivide) chunk.preventDivide();
-    }
+    if (chunk.rule.isHardened) _handleHardSplit();
 
     _pendingNewlines = 0;
     _pendingNested = false;

lib/src/source_visitor.dart

@@ -81,14 +81,6 @@ class SourceVisitor extends ThrowingAstVisitor {
   /// split.
   final List<bool> _collectionSplits = [];
 
-  /// The stack of current rules for handling parameter metadata.
-  ///
-  /// Each time a parameter (or type parameter) list is begun, a single rule
-  /// for all of the metadata annotations on parameters in that list is pushed
-  /// onto this stack. We reuse this rule for all annotations so that they split
-  /// in unison.
-  final List<Rule> _metadataRules = [];
-
   /// The mapping for blocks that are managed by the argument list that contains
   /// them.
   ///
@@ -1343,8 +1335,6 @@ class SourceVisitor extends ThrowingAstVisitor {
     if (nestExpression) builder.nestExpression();
     token(node.leftParenthesis);
 
-    _metadataRules.add(Rule());
-
     PositionalRule? rule;
     if (requiredParams.isNotEmpty) {
       rule = PositionalRule(null, argumentCount: requiredParams.length);
@@ -1405,8 +1395,6 @@ class SourceVisitor extends ThrowingAstVisitor {
       token(node.rightDelimiter);
     }
 
-    _metadataRules.removeLast();
-
     token(node.rightParenthesis);
     if (nestExpression) builder.unnest();
   }
@@ -2507,7 +2495,6 @@ class SourceVisitor extends ThrowingAstVisitor {
       return;
     }
 
-    _metadataRules.add(Rule());
     token(node.leftParenthesis);
     builder.startRule();
 
@@ -2562,7 +2549,6 @@ class SourceVisitor extends ThrowingAstVisitor {
 
     builder = builder.endBlock(forceSplit: force);
     builder.endRule();
-    _metadataRules.removeLast();
 
     // Now write the delimiter(s) themselves.
     _writeText(firstClosingDelimiter.lexeme, firstClosingDelimiter);
@@ -2983,11 +2969,7 @@ class SourceVisitor extends ThrowingAstVisitor {
 
   @override
   void visitTypeParameterList(TypeParameterList node) {
-    _metadataRules.add(Rule());
-
     _visitGenericList(node.leftBracket, node.rightBracket, node.typeParameters);
-
-    _metadataRules.removeLast();
   }
 
   @override
@@ -3135,24 +3117,12 @@ class SourceVisitor extends ThrowingAstVisitor {
       return;
     }
 
-    // Split before all of the annotations or none.
-    builder.startLazyRule(_metadataRules.last);
+    // Split before all of the annotations on this parameter or none of them.
+    builder.startLazyRule();
 
-    visitNodes(metadata, between: split, after: () {
-      // Don't nest until right before the last metadata. Ensures we only
-      // indent the parameter and not any of the metadata:
-      //
-      //     function(
-      //         @LongAnnotation
-      //         @LongAnnotation
-      //             indentedParameter) {}
-      builder.nestExpression(now: true);
-      split();
-    });
+    visitNodes(metadata, between: split, after: split);
     visitParameter();
 
-    builder.unnest();
-
     // Wrap the rule around the parameter too. If it splits, we want to force
     // the annotations to split as well.
     builder.endRule();
@@ -3672,8 +3642,6 @@ class SourceVisitor extends ThrowingAstVisitor {
     // Can't have a trailing comma if there are no parameters.
     assert(parameters.parameters.isNotEmpty);
 
-    _metadataRules.add(Rule());
-
     // Always split the parameters.
     builder.startRule(Rule.hard());
 
@@ -3698,7 +3666,7 @@ class SourceVisitor extends ThrowingAstVisitor {
     builder = builder.startBlock();
 
     for (var parameter in parameters.parameters) {
-      builder.writeNewline(preventDivide: true);
+      builder.writeNewline();
       visit(parameter);
       _writeCommaAfter(parameter);
 
@@ -3715,15 +3683,13 @@ class SourceVisitor extends ThrowingAstVisitor {
     var firstDelimiter =
         parameters.rightDelimiter ?? parameters.rightParenthesis;
     if (firstDelimiter.precedingComments != null) {
-      builder.writeNewline(preventDivide: true);
+      builder.writeNewline();
       writePrecedingCommentsAndNewlines(firstDelimiter);
     }
 
     builder = builder.endBlock();
     builder.endRule();
 
-    _metadataRules.removeLast();
-
     // Now write the delimiter itself.
     _writeText(firstDelimiter.lexeme, firstDelimiter);
     if (firstDelimiter != parameters.rightParenthesis) {

test/comments/functions.unit

@@ -235,17 +235,16 @@ function({
 }) {
   ;
 }
->>> metadata in trailing comma parameter list splits together even with comment
+>>> splitting in none parameter's metadata doesn't force others to split
 function(@Annotation longParameter,
   // Comment.
 @Annotation @Other @Third longParameter2,) {}
 <<<
 function(
-  @Annotation
-      longParameter,
+  @Annotation longParameter,
   // Comment.
   @Annotation
   @Other
   @Third
-      longParameter2,
+  longParameter2,
 ) {}

test/regression/0200/0247.unit

@@ -10,8 +10,8 @@
 <<<
   init(
       {@Option(help: 'The git Uri containing the jefe.yaml.', abbr: 'g')
-          String gitUri,
+      String gitUri,
       @Option(help: 'The directory to install into', abbr: 'd')
-          String installDirectory: '.',
+      String installDirectory: '.',
       @Flag(help: 'Skips the checkout of the develop branch', abbr: 's')
-          bool skipCheckout: false}) async {}
+      bool skipCheckout: false}) async {}

test/regression/0300/0387.unit

@@ -7,13 +7,10 @@ greet(@Rest(valueHelp: 'who', help: 'Name(s) to greet.') List<String> who,
     @Option(name: 'greeting', help: 'Alternate word to greet with e.g. "Hi".')
     String salutation: 'Hello'}) {}
 <<<
-greet(
-    @Rest(valueHelp: 'who', help: 'Name(s) to greet.')
-        List<String> who,
+greet(@Rest(valueHelp: 'who', help: 'Name(s) to greet.') List<String> who,
     {@Group.start(title: 'Output')
     @Option(help: 'How many !\'s to append.')
-        int enthusiasm: 0,
-    @Flag(abbr: 'l', help: 'Put names on separate lines.')
-        bool lineMode: false,
+    int enthusiasm: 0,
+    @Flag(abbr: 'l', help: 'Put names on separate lines.') bool lineMode: false,
     @Option(name: 'greeting', help: 'Alternate word to greet with e.g. "Hi".')
-        String salutation: 'Hello'}) {}
+    String salutation: 'Hello'}) {}

test/regression/0400/0444.unit

@@ -17,10 +17,8 @@ class MyComponent {
   Object secondArgument;
 
   MyComponent(
-      @Inject(const MyFirstArgument())
-          this.firstArgument,
+      @Inject(const MyFirstArgument()) this.firstArgument,
       @Inject(const MySuperDuperLongNamedArgument())
-          this.superDuperLongNamedArgument, // LOOK AT ME
-      @Inject(const MySecondArgument())
-          this.secondArgument);
+      this.superDuperLongNamedArgument, // LOOK AT ME
+      @Inject(const MySecondArgument()) this.secondArgument);
 }