If a class's type parameters split, force the clauses to split too. (#1511)

I think it looks weird if the `extends`, etc. clauses hang immediately after the `>` when the type parameters split.

Since import/export directives use the same piece, this nominally affects them too but in practice it's extremely rare for there to be a newline in an import before any other clauses.

While I was at it, I cleaned up the way we build a piece tree for directives. That's some of the oldest code in the new formatter and was more complex than it needed to be. There was no need to use both an InfixPiece and a ClausesPiece since they do basically the same thing.

Author: munificent

lib/src/front_end/piece_factory.dart

@@ -805,55 +805,48 @@ mixin PieceFactory {
   void writeImport(NamespaceDirective directive, Token keyword,
       {Token? deferredKeyword, Token? asKeyword, SimpleIdentifier? prefix}) {
     pieces.withMetadata(directive.metadata, () {
-      if (directive.configurations.isEmpty && asKeyword == null) {
-        // If there are no configurations or prefix (the common case), just
-        // write the import directly inline.
+      // Build a piece for the directive itself.
+      var directivePiece = pieces.build(() {
         pieces.token(keyword);
         pieces.space();
         pieces.visit(directive.uri);
-      } else {
-        // Otherwise, allow splitting between the configurations and prefix.
-        var sections = [
-          pieces.build(() {
-            pieces.token(keyword);
-            pieces.space();
-            pieces.visit(directive.uri);
-          })
-        ];
-
-        for (var configuration in directive.configurations) {
-          sections.add(nodePiece(configuration));
-        }
+      });
 
-        if (asKeyword != null) {
-          sections.add(pieces.build(() {
-            pieces.token(deferredKeyword, spaceAfter: true);
-            pieces.token(asKeyword);
-            pieces.space();
-            pieces.visit(prefix!);
-          }));
-        }
+      // Include any `if` clauses.
+      var clauses = <Piece>[];
+      for (var configuration in directive.configurations) {
+        clauses.add(nodePiece(configuration));
+      }
 
-        pieces.add(InfixPiece(const [], sections));
+      // Include the `as` clause.
+      if (asKeyword != null) {
+        clauses.add(pieces.build(() {
+          pieces.token(deferredKeyword, spaceAfter: true);
+          pieces.token(asKeyword);
+          pieces.space();
+          pieces.visit(prefix!);
+        }));
       }
 
-      if (directive.combinators.isNotEmpty) {
-        var combinators = <Piece>[];
-        for (var combinatorNode in directive.combinators) {
-          switch (combinatorNode) {
-            case HideCombinator(hiddenNames: var names):
-            case ShowCombinator(shownNames: var names):
-              combinators.add(InfixPiece(const [], [
-                tokenPiece(combinatorNode.keyword),
-                for (var name in names)
-                  tokenPiece(name.token, commaAfter: true),
-              ]));
-            default:
-              throw StateError('Unknown combinator type $combinatorNode.');
-          }
+      // Include the `show` and `hide` clauses.
+      for (var combinatorNode in directive.combinators) {
+        switch (combinatorNode) {
+          case HideCombinator(hiddenNames: var names):
+          case ShowCombinator(shownNames: var names):
+            clauses.add(InfixPiece(const [], [
+              tokenPiece(combinatorNode.keyword),
+              for (var name in names) tokenPiece(name.token, commaAfter: true),
+            ]));
+          default:
+            throw StateError('Unknown combinator type $combinatorNode.');
         }
+      }
 
-        pieces.add(ClausePiece(combinators));
+      // If there are clauses, include them.
+      if (clauses.isNotEmpty) {
+        pieces.add(ClausePiece(directivePiece, clauses));
+      } else {
+        pieces.add(directivePiece);
       }
 
       pieces.token(directive.semicolon);
@@ -1245,16 +1238,12 @@ mixin PieceFactory {
             [if (nativeClause.name case var name?) name]);
       }
 
-      ClausePiece? clausesPiece;
       if (clauses.isNotEmpty) {
-        clausesPiece = ClausePiece(clauses,
+        header = ClausePiece(header, clauses,
             allowLeadingClause: extendsClause != null || onClause != null);
       }
 
-      var bodyPiece = body();
-
-      pieces
-          .add(TypePiece(header, clausesPiece, bodyPiece, bodyType: bodyType));
+      pieces.add(TypePiece(header, body(), bodyType: bodyType));
     });
   }
 

lib/src/piece/clause.dart

@@ -65,6 +65,10 @@ class ClausePiece extends Piece {
   /// State where we split between the clauses but not before the first one.
   static const State _betweenClauses = State(1);
 
+  /// The leading construct the clauses are applied to: a class declaration,
+  /// import directive, etc.
+  final Piece _header;
+
   final List<Piece> _clauses;
 
   /// If `true`, then we're allowed to split between the clauses without
@@ -80,7 +84,7 @@ class ClausePiece extends Piece {
   ///     }
   final bool _allowLeadingClause;
 
-  ClausePiece(this._clauses, {bool allowLeadingClause = false})
+  ClausePiece(this._header, this._clauses, {bool allowLeadingClause = false})
       : _allowLeadingClause = allowLeadingClause && _clauses.length > 1;
 
   @override
@@ -89,7 +93,10 @@ class ClausePiece extends Piece {
 
   @override
   bool allowNewlineInChild(State state, Piece child) {
-    if (_allowLeadingClause && child == _clauses.first) {
+    if (child == _header) {
+      // If the header splits, force the clauses to split too.
+      return state == State.split;
+    } else if (_allowLeadingClause && child == _clauses.first) {
       // A split inside the first clause forces a split before the keyword.
       return state == State.split;
     } else {
@@ -101,6 +108,8 @@ class ClausePiece extends Piece {
 
   @override
   void format(CodeWriter writer, State state) {
+    writer.format(_header);
+
     writer.pushIndent(Indent.expression);
 
     for (var clause in _clauses) {
@@ -123,6 +132,7 @@ class ClausePiece extends Piece {
 
   @override
   void forEachChild(void Function(Piece piece) callback) {
+    callback(_header);
     _clauses.forEach(callback);
   }
 }

lib/src/piece/type.dart

@@ -2,27 +2,23 @@
 // 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.
 import '../back_end/code_writer.dart';
-import 'clause.dart';
 import 'piece.dart';
 
 /// Piece for a type declaration with a body containing members.
 ///
 /// Used for class, enum, and extension declarations.
 class TypePiece extends Piece {
-  /// The leading keywords and modifiers, type name, and type parameters.
+  /// The leading keywords and modifiers, type name, type parameters, and any
+  /// other `extends`, `with`, etc. clauses.
   final Piece _header;
 
-  /// The `extends`, `with`, and/or `implements` clauses, if there are any.
-  final ClausePiece? _clauses;
-
   /// The `native` clause, if any, and the type body.
   final Piece _body;
 
   /// What kind of body the type has.
   final TypeBodyType _bodyType;
 
-  TypePiece(this._header, this._clauses, this._body,
-      {required TypeBodyType bodyType})
+  TypePiece(this._header, this._body, {required TypeBodyType bodyType})
       : _bodyType = bodyType;
 
   @override
@@ -47,18 +43,13 @@ class TypePiece extends Piece {
   @override
   void format(CodeWriter writer, State state) {
     writer.format(_header);
-    if (_clauses case var clauses?) {
-      writer.format(clauses);
-    }
-
     if (_bodyType != TypeBodyType.semicolon) writer.space();
     writer.format(_body);
   }
 
   @override
   void forEachChild(void Function(Piece piece) callback) {
     callback(_header);
-    if (_clauses case var clauses?) callback(clauses);
     callback(_body);
   }
 }

test/tall/declaration/class_clause.unit

@@ -251,4 +251,21 @@ class C extends A
         B<
           LongTypeArgument,
           AnotherLongType
-        > {}
+        > {}
+>>> Split in class type parameters forces clause to split.
+class C<LongTypeParameter, AnotherLongType> extends Other {}
+<<<
+class C<
+  LongTypeParameter,
+  AnotherLongType
+>
+    extends Other {}
+>>> Split in class type parameters forces clauses to split.
+class C<LongTypeParameter, AnotherLongType> extends Other with Mixin {}
+<<<
+class C<
+  LongTypeParameter,
+  AnotherLongType
+>
+    extends Other
+    with Mixin {}

test/tall/declaration/extension.unit

@@ -35,7 +35,8 @@ extension Extension<LongTypeParameter, Another> on BaseClass {}
 extension Extension<
   LongTypeParameter,
   Another
-> on BaseClass {}
+>
+    on BaseClass {}
 >>> Unnamed.
 extension on String {}
 <<<

test/tall/declaration/extension_type.unit

@@ -48,7 +48,8 @@ extension type LongExtensionType(LongTypeName a) implements Something {
 <<<
 extension type LongExtensionType(
   LongTypeName a,
-) implements Something {
+)
+    implements Something {
   method() {
     ;
   }

test/tall/regression/other/misc.unit

@@ -19,4 +19,21 @@ SomeVeryLongReturnType someFunctionName(
         })
         .then(________traverseDeps_depender__deps_);
   }
+}
+>>> Split in long type parameter clause.
+abstract class StreamNotifierProviderBase<
+  NotifierT extends AsyncNotifierBase<T>,
+  T
+> extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
+    with FutureModifier<T> {
+  // ...
+}
+<<<
+abstract class StreamNotifierProviderBase<
+  NotifierT extends AsyncNotifierBase<T>,
+  T
+>
+    extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
+    with FutureModifier<T> {
+  // ...
 }