Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 35 additions & 46 deletions lib/src/front_end/piece_factory.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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));
});
}

Expand Down
14 changes: 12 additions & 2 deletions lib/src/piece/clause.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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) {
Expand All @@ -123,6 +132,7 @@ class ClausePiece extends Piece {

@override
void forEachChild(void Function(Piece piece) callback) {
callback(_header);
_clauses.forEach(callback);
}
}
15 changes: 3 additions & 12 deletions lib/src/piece/type.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
}
Expand Down
19 changes: 18 additions & 1 deletion test/tall/declaration/class_clause.unit
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
3 changes: 2 additions & 1 deletion test/tall/declaration/extension.unit
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ extension Extension<LongTypeParameter, Another> on BaseClass {}
extension Extension<
LongTypeParameter,
Another
> on BaseClass {}
>
on BaseClass {}
>>> Unnamed.
extension on String {}
<<<
Expand Down
3 changes: 2 additions & 1 deletion test/tall/declaration/extension_type.unit
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ extension type LongExtensionType(LongTypeName a) implements Something {
<<<
extension type LongExtensionType(
LongTypeName a,
) implements Something {
)
implements Something {
method() {
;
}
Expand Down
17 changes: 17 additions & 0 deletions test/tall/regression/other/misc.unit
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
// ...
}