Skip to content

Commit

Permalink
Allow a factory constructor with metadata before it to split. (#1580)
Browse files Browse the repository at this point in the history
Prior to this PR, if there was metadata before a constructor, it would
be attached to the "header" piece of the constructor -- the leading
keywords and name -- instead of the entire constructor piece.

That in turn meant that the newline after the metadata would lead the
formatter to think a split had occurred inside the header itself,
instead of before the entire constructor. That would in turn confuse
the solver and it ended up trying to disallow splitting anywhere in the
constructor, even if that led to an overflowing line.

This fixes that so that the metadata is attached to the entire
constructor piece, not just the nested header part.
  • Loading branch information
munificent authored Oct 15, 2024
1 parent 63caad3 commit 89577e7
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 32 deletions.
66 changes: 34 additions & 32 deletions lib/src/front_end/ast_node_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -407,43 +407,45 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {

@override
void visitConstructorDeclaration(ConstructorDeclaration node) {
var header = pieces.build(metadata: node.metadata, () {
pieces.modifier(node.externalKeyword);
pieces.modifier(node.constKeyword);
pieces.modifier(node.factoryKeyword);
pieces.visit(node.returnType);
pieces.token(node.period);
pieces.token(node.name);
});
pieces.withMetadata(node.metadata, () {
var header = pieces.build(() {
pieces.modifier(node.externalKeyword);
pieces.modifier(node.constKeyword);
pieces.modifier(node.factoryKeyword);
pieces.visit(node.returnType);
pieces.token(node.period);
pieces.token(node.name);
});

var parameters = nodePiece(node.parameters);
var parameters = nodePiece(node.parameters);

Piece? redirect;
Piece? initializerSeparator;
Piece? initializers;
if (node.redirectedConstructor case var constructor?) {
var separator = pieces.build(() {
pieces.token(node.separator);
pieces.space();
});
Piece? redirect;
Piece? initializerSeparator;
Piece? initializers;
if (node.redirectedConstructor case var constructor?) {
var separator = pieces.build(() {
pieces.token(node.separator);
pieces.space();
});

redirect = AssignPiece(
separator, nodePiece(constructor, context: NodeContext.assignment),
canBlockSplitRight: false);
} else if (node.initializers.isNotEmpty) {
initializerSeparator = tokenPiece(node.separator!);
initializers = createCommaSeparated(node.initializers);
}
redirect = AssignPiece(
separator, nodePiece(constructor, context: NodeContext.assignment),
canBlockSplitRight: false);
} else if (node.initializers.isNotEmpty) {
initializerSeparator = tokenPiece(node.separator!);
initializers = createCommaSeparated(node.initializers);
}

var body = nodePiece(node.body);
var body = nodePiece(node.body);

pieces.add(ConstructorPiece(header, parameters, body,
canSplitParameters: node.parameters.parameters
.canSplit(node.parameters.rightParenthesis),
hasOptionalParameter: node.parameters.rightDelimiter != null,
redirect: redirect,
initializerSeparator: initializerSeparator,
initializers: initializers));
pieces.add(ConstructorPiece(header, parameters, body,
canSplitParameters: node.parameters.parameters
.canSplit(node.parameters.rightParenthesis),
hasOptionalParameter: node.parameters.rightDelimiter != null,
redirect: redirect,
initializerSeparator: initializerSeparator,
initializers: initializers));
});
}

@override
Expand Down
11 changes: 11 additions & 0 deletions test/tall/regression/other/dart.unit
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,15 @@ class Benchmark {
// A rate of one run per 2s, with a millisecond of noise. Some variation is
// needed for Golem's noise-based filtering and regression detection.
double measure() => (2000 + Random().nextDouble() - 0.5) * 1000;
}
>>>
class Uint8ClampedList {
@patch
factory Uint8ClampedList.fromList(List<int> elements) = NativeUint8ClampedList.fromList;
}
<<<
class Uint8ClampedList {
@patch
factory Uint8ClampedList.fromList(List<int> elements) =
NativeUint8ClampedList.fromList;
}

0 comments on commit 89577e7

Please sign in to comment.