From e4c3dc1755653104ec039f74a814381e7c7309da Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Mon, 7 Oct 2024 18:54:51 -0700 Subject: [PATCH 1/2] Better formatting for long type parameter bounds. The tall style was already doing a better job on #1568 than the old short style does. (I don't know why it leaves an overly long line there. It seems like a bug but perhaps a nasty subtle one in the solver.) But it still didn't allow splitting before `extends` in a bound which could leave long lines in cases where the type parameter and bound name are both quite long. Rare, but we may as well do a better job. So this introduces a new piece that allows splitting before `extends` if needed. Fix #1568. --- lib/src/front_end/ast_node_visitor.dart | 20 ++++-- lib/src/piece/bound.dart | 65 ++++++++++++++++++ .../declaration/class_type_parameter.unit | 17 +++++ test/tall/regression/1500/1568.unit | 67 +++++++++++++++++++ 4 files changed, 164 insertions(+), 5 deletions(-) create mode 100644 lib/src/piece/bound.dart create mode 100644 test/tall/regression/1500/1568.unit diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 05587a04..32977654 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -10,6 +10,7 @@ import '../ast_extensions.dart'; import '../constants.dart'; import '../dart_formatter.dart'; import '../piece/assign.dart'; +import '../piece/bound.dart'; import '../piece/case.dart'; import '../piece/constructor.dart'; import '../piece/control_flow.dart'; @@ -1840,12 +1841,21 @@ final class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { @override void visitTypeParameter(TypeParameter node) { pieces.withMetadata(node.metadata, inlineMetadata: true, () { - pieces.token(node.name); if (node.bound case var bound?) { - pieces.space(); - pieces.token(node.extendsKeyword); - pieces.space(); - pieces.visit(bound); + var typeParameterPiece = pieces.build(() { + pieces.token(node.name); + }); + + var boundPiece = pieces.build(() { + pieces.token(node.extendsKeyword); + pieces.space(); + pieces.visit(bound); + }); + + pieces.add(BoundPiece(typeParameterPiece, boundPiece)); + } else { + // No bound. + pieces.token(node.name); } }); } diff --git a/lib/src/piece/bound.dart b/lib/src/piece/bound.dart new file mode 100644 index 00000000..bacb8bb3 --- /dev/null +++ b/lib/src/piece/bound.dart @@ -0,0 +1,65 @@ +// Copyright (c) 2024, 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. +import '../back_end/code_writer.dart'; +import '../constants.dart'; +import 'piece.dart'; + +/// A piece for a type parameter and its bound. +/// +/// Handles not splitting before `extends` if we can split inside the bound's +/// own type arguments, or splitting before `extends` if that isn't enough to +/// get it to fit. +final class BoundPiece extends Piece { + /// Split inside the type arguments of the bound, but not at `extends`, as in: + /// + /// class C< + /// T extends Map< + /// LongKeyType, + /// LongValueType + /// > + /// >{} + static const State _insideBound = State(1); + + /// Split at `extends`, like: + /// + /// class C< + /// LongTypeParameters + /// extends LongBoundType + /// >{} + static const State _beforeExtends = State(2); + + /// The type parameter name. + final Piece _typeParameter; + + /// The bound with the preceding `extends` keyword. + final Piece _bound; + + BoundPiece(this._typeParameter, this._bound); + + @override + List get additionalStates => const [_insideBound, _beforeExtends]; + + @override + bool allowNewlineInChild(State state, Piece child) => switch (state) { + State.unsplit => false, + _insideBound => child == _bound, + _beforeExtends => true, + _ => throw ArgumentError('Unexpected state.'), + }; + + @override + void format(CodeWriter writer, State state) { + if (state == _beforeExtends) writer.pushIndent(Indent.expression); + writer.format(_typeParameter); + writer.splitIf(state == _beforeExtends); + writer.format(_bound); + if (state == _beforeExtends) writer.popIndent(); + } + + @override + void forEachChild(void Function(Piece piece) callback) { + callback(_typeParameter); + callback(_bound); + } +} diff --git a/test/tall/declaration/class_type_parameter.unit b/test/tall/declaration/class_type_parameter.unit index 6e28b06b..065e1b6a 100644 --- a/test/tall/declaration/class_type_parameter.unit +++ b/test/tall/declaration/class_type_parameter.unit @@ -19,6 +19,23 @@ class LongClassName< Second, Third > {} +>>> Split inside bound. +class LongClassName {} +<<< +class LongClassName< + VeryLongTypeParameter + extends VeryLongBoundName +> {} +>>> Split at `extends` and inside bound type arguments. +class LongClassName> {} +<<< +class LongClassName< + VeryLongTypeParameter + extends VeryLongBoundName< + LongTypeArgument + > +> {} >>> Split inside type parameter bound splits type parameters. class LongClassName> {} <<< diff --git a/test/tall/regression/1500/1568.unit b/test/tall/regression/1500/1568.unit new file mode 100644 index 00000000..6530aed8 --- /dev/null +++ b/test/tall/regression/1500/1568.unit @@ -0,0 +1,67 @@ +>>> Conveniently fits in the new style. +abstract interface class TypeAnalyzerOperations< + TypeStructure extends SharedTypeStructure, + Variable extends Object, + TypeParameterStructure extends SharedTypeParameterStructure, + TypeDeclarationType extends Object, + TypeDeclaration extends Object> + implements FlowAnalysisOperations> { + // ... +} +<<< +abstract interface class TypeAnalyzerOperations< + TypeStructure extends SharedTypeStructure, + Variable extends Object, + TypeParameterStructure extends SharedTypeParameterStructure, + TypeDeclarationType extends Object, + TypeDeclaration extends Object +> + implements FlowAnalysisOperations> { + // ... +} +>>> Make some type arguments longer to force it to split. +abstract interface class TypeAnalyzerOperations< + TypeStructure extends SharedTypeStructure, + Variable extends Object, + TypeParameterStructure extends SharedTypeParameterStructure, + TypeDeclarationType extends Object, + TypeDeclaration extends Object> + implements FlowAnalysisOperations> { + // ... +} +<<< +abstract interface class TypeAnalyzerOperations< + TypeStructure extends SharedTypeStructure, + Variable extends Object, + TypeParameterStructure + extends SharedTypeParameterStructure, + TypeDeclarationType extends Object, + TypeDeclaration extends Object +> + implements FlowAnalysisOperations> { + // ... +} +>>> Even longer for more splitting. +abstract interface class TypeAnalyzerOperations< + TypeStructure extends SharedTypeStructure, + Variable extends Object, + TypeParameterStructure extends SharedTypeParameterStructure, + TypeDeclarationType extends Object, + TypeDeclaration extends Object> + implements FlowAnalysisOperations> { + // ... +} +<<< +abstract interface class TypeAnalyzerOperations< + TypeStructure extends SharedTypeStructure, + Variable extends Object, + TypeParameterStructure extends SharedTypeParameterStructure< + LongerTypeStructure, + AnotherTypeArgument + >, + TypeDeclarationType extends Object, + TypeDeclaration extends Object +> + implements FlowAnalysisOperations> { + // ... +} \ No newline at end of file From b0a0d6c4bf965ba8891a18e6a8c1600e7bd4bbe5 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Tue, 8 Oct 2024 15:14:43 -0700 Subject: [PATCH 2/2] Rename BoundPiece -> TypeParameterBoundPiece. --- lib/src/front_end/ast_node_visitor.dart | 4 ++-- lib/src/piece/{bound.dart => type_parameter_bound.dart} | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) rename lib/src/piece/{bound.dart => type_parameter_bound.dart} (94%) diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 32977654..c7e16893 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -10,7 +10,6 @@ import '../ast_extensions.dart'; import '../constants.dart'; import '../dart_formatter.dart'; import '../piece/assign.dart'; -import '../piece/bound.dart'; import '../piece/case.dart'; import '../piece/constructor.dart'; import '../piece/control_flow.dart'; @@ -18,6 +17,7 @@ import '../piece/infix.dart'; import '../piece/list.dart'; import '../piece/piece.dart'; import '../piece/type.dart'; +import '../piece/type_parameter_bound.dart'; import '../piece/variable.dart'; import '../profile.dart'; import '../source_code.dart'; @@ -1852,7 +1852,7 @@ final class AstNodeVisitor extends ThrowingAstVisitor with PieceFactory { pieces.visit(bound); }); - pieces.add(BoundPiece(typeParameterPiece, boundPiece)); + pieces.add(TypeParameterBoundPiece(typeParameterPiece, boundPiece)); } else { // No bound. pieces.token(node.name); diff --git a/lib/src/piece/bound.dart b/lib/src/piece/type_parameter_bound.dart similarity index 94% rename from lib/src/piece/bound.dart rename to lib/src/piece/type_parameter_bound.dart index bacb8bb3..3d7a5a7a 100644 --- a/lib/src/piece/bound.dart +++ b/lib/src/piece/type_parameter_bound.dart @@ -10,7 +10,7 @@ import 'piece.dart'; /// Handles not splitting before `extends` if we can split inside the bound's /// own type arguments, or splitting before `extends` if that isn't enough to /// get it to fit. -final class BoundPiece extends Piece { +final class TypeParameterBoundPiece extends Piece { /// Split inside the type arguments of the bound, but not at `extends`, as in: /// /// class C< @@ -35,7 +35,7 @@ final class BoundPiece extends Piece { /// The bound with the preceding `extends` keyword. final Piece _bound; - BoundPiece(this._typeParameter, this._bound); + TypeParameterBoundPiece(this._typeParameter, this._bound); @override List get additionalStates => const [_insideBound, _beforeExtends];