Skip to content

Commit

Permalink
Support eagerly pinning a piece to a specific state. (#1291)
Browse files Browse the repository at this point in the history
* Support eagerly pinning a piece to a specific state.

The BlockPiece had some bespoke support for being in an "always split"
configuration that overrode the two different ways it could be
formatted. While working on conditional expressions, I found myself
needing something similar to InfixPiece.

Instead of adding similar code to that and likely other pieces in the
future, I figured it made more sense to bake support for locking a
piece into a specific state directly in the Piece base class.

* Revamp how pieces enumerate their states.

- Rename State.initial to `State.unsplit` to describe what it
  represents. (This is a little bit of a misnomer for SequencePiece
  where there is still a hard newline between each child piece, but it's
  clearer for all of the other pieces.)

- Piece subclasses implement Piece.additionalStates to enumerate the
  possible set of states, if they support more than just `State.unsplit`.

- Have Piece.states return only the pinned state if there is one and
  otherwise return `State.unsplit` followed by `additionalStates`.

- Also reorganized the format code in InfixPiece to have less
  copy/paste.

* Re-add comment on unsplit block pieces.
  • Loading branch information
munificent authored Oct 21, 2023
1 parent ee3dd8a commit 15fef68
Show file tree
Hide file tree
Showing 14 changed files with 85 additions and 75 deletions.
7 changes: 3 additions & 4 deletions lib/src/back_end/solution.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ class PieceStateSet {
PieceStateSet._(this._pieces, this._pieceStates);

/// The state this solution selects for [piece].
State pieceState(Piece piece) => _pieceStates[piece] ?? State.initial;
///
/// If no state has been selected, defaults to the first state.
State pieceState(Piece piece) => _pieceStates[piece] ?? piece.states.first;

/// Gets the first piece that doesn't have a state selected yet, or `null` if
/// all pieces have selected states.
Expand Down Expand Up @@ -86,9 +88,6 @@ class Solution implements Comparable<Solution> {
if (piece == null) return const [];

return [
// All pieces support a default state.
Solution(root, pageWidth, _state.cloneWith(piece, State.initial)),

for (var state in piece.states)
Solution(root, pageWidth, _state.cloneWith(piece, state))
];
Expand Down
7 changes: 3 additions & 4 deletions lib/src/back_end/solver.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ import 'solution.dart';
/// possible states, so it isn't feasible to brute force. There are a few
/// techniques we use to avoid that:
///
/// - All pieces default to being in [State.initial]. Every piece is
/// implemented such that that state has no line splits (or only mandatory
/// ones) and zero cost. Thus, it tries solutions with a minimum number of
/// line splits first.
/// - The initial state for each piece has no line splits or only mandatory
/// ones. Thus, it tries solutions with a minimum number of line splits
/// first.
///
/// - Solutions are explored in priority order. We explore solutions with the
/// the lowest cost first. This way, as soon as we find a solution with no
Expand Down
7 changes: 4 additions & 3 deletions lib/src/piece/assign.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import 'piece.dart';
///
/// These constructs can be formatted three ways:
///
/// [State.initial] No split at all:
/// [State.unsplit] No split at all:
///
/// ```
/// var x = 123;
Expand Down Expand Up @@ -60,13 +60,14 @@ class AssignPiece extends Piece {
: _isValueDelimited = isValueDelimited;

@override
List<State> get states => [if (_isValueDelimited) _insideValue, _atEquals];
List<State> get additionalStates =>
[if (_isValueDelimited) _insideValue, _atEquals];

@override
void format(CodeWriter writer, State state) {
// A split in either child piece forces splitting after the "=" unless it's
// a delimited expression.
if (state == State.initial) writer.setAllowNewlines(false);
if (state == State.unsplit) writer.setAllowNewlines(false);

// Don't indent a split delimited expression.
if (state != _insideValue) writer.setIndent(Indent.expression);
Expand Down
18 changes: 8 additions & 10 deletions lib/src/piece/block.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,22 @@ class BlockPiece extends Piece {
/// The closing delimiter.
final Piece rightBracket;

/// Whether the block should always split its contents.
///
/// True for most blocks, but false for enums and blocks containing only
/// inline block comments.
final bool _alwaysSplit;

/// If [alwaysSplit] is true, then the block should always split its contents.
/// This is true for most blocks, but false for enums and blocks containing
/// only inline block comments.
BlockPiece(this.leftBracket, this.contents, this.rightBracket,
{bool alwaysSplit = true})
: _alwaysSplit = alwaysSplit;
{bool alwaysSplit = true}) {
if (alwaysSplit) pin(State.split);
}

@override
List<State> get states => _alwaysSplit ? const [] : const [State.split];
List<State> get additionalStates => const [State.split];

@override
void format(CodeWriter writer, State state) {
writer.format(leftBracket);

if (_alwaysSplit || state == State.split) {
if (state == State.split) {
if (contents.isNotEmpty) {
writer.newline(indent: Indent.block);
writer.format(contents);
Expand Down
3 changes: 0 additions & 3 deletions lib/src/piece/do_while.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ class DoWhilePiece extends Piece {

DoWhilePiece(this._body, this._condition);

@override
List<State> get states => const [];

@override
void format(CodeWriter writer, State state) {
writer.setIndent(Indent.none);
Expand Down
2 changes: 1 addition & 1 deletion lib/src/piece/function.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class FunctionTypePiece extends Piece {
FunctionTypePiece(this._returnType, this._signature);

@override
List<State> get states => const [State.split];
List<State> get additionalStates => const [State.split];

@override
void format(CodeWriter writer, State state) {
Expand Down
4 changes: 2 additions & 2 deletions lib/src/piece/if.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ class IfPiece extends Piece {

/// If there is at least one else or else-if clause, then it always splits.
@override
List<State> get states => _isUnbracedIfThen ? const [State.split] : const [];
List<State> get additionalStates => [if (_isUnbracedIfThen) State.split];

@override
void format(CodeWriter writer, State state) {
if (_isUnbracedIfThen) {
// A split in the condition or statement forces moving the entire
// statement to the next line.
writer.setAllowNewlines(state != State.initial);
writer.setAllowNewlines(state != State.unsplit);

var section = _sections.single;
writer.format(section.header);
Expand Down
8 changes: 4 additions & 4 deletions lib/src/piece/import.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import 'piece.dart';
///
/// Combinators can be split like so:
///
/// [State.initial] All on one line:
/// [State.unsplit] All on one line:
///
/// ```
/// import 'animals.dart' show Ant, Bat hide Cat, Dog;
Expand Down Expand Up @@ -109,7 +109,7 @@ class ImportPiece extends Piece {
}

@override
List<State> get states => [
List<State> get additionalStates => [
_beforeCombinators,
if (_combinators.length > 1) ...[
_firstCombinator,
Expand All @@ -126,13 +126,13 @@ class ImportPiece extends Piece {

if (_combinators.isNotEmpty) {
_combinators[0].format(writer,
splitKeyword: state != State.initial,
splitKeyword: state != State.unsplit,
splitNames: state == _firstCombinator || state == State.split);
}

if (_combinators.length > 1) {
_combinators[1].format(writer,
splitKeyword: state != State.initial,
splitKeyword: state != State.unsplit,
splitNames: state == _secondCombinator || state == State.split);
}
}
Expand Down
24 changes: 9 additions & 15 deletions lib/src/piece/infix.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,19 @@ class InfixPiece extends Piece {
InfixPiece(this.operands);

@override
List<State> get states => const [State.split];
List<State> get additionalStates => const [State.split];

@override
void format(CodeWriter writer, State state) {
switch (state) {
case State.initial:
writer.setAllowNewlines(false);
for (var i = 0; i < operands.length; i++) {
writer.format(operands[i]);

if (i < operands.length - 1) writer.space();
}
if (state == State.unsplit) {
writer.setAllowNewlines(false);
} else {
writer.setNesting(Indent.expression);
}

case State.split:
writer.setNesting(Indent.expression);
for (var i = 0; i < operands.length; i++) {
writer.format(operands[i]);
if (i < operands.length - 1) writer.newline();
}
for (var i = 0; i < operands.length; i++) {
writer.format(operands[i]);
if (i < operands.length - 1) writer.splitIf(state == State.split);
}
}

Expand Down
17 changes: 7 additions & 10 deletions lib/src/piece/list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,12 @@ class ListPiece extends Piece {
this._isTypeList);

@override
List<State> get states {
// Don't split between an empty pair of brackets.
if (_arguments.isEmpty) return const [];

// Type lists are more expensive to split.
if (_isTypeList) return const [_splitTypes];

return const [State.split];
}
List<State> get additionalStates => [
if (_isTypeList)
_splitTypes // Type lists are more expensive to split.
else if (_arguments.isNotEmpty)
State.split // Don't split between an empty pair of brackets.
];

@override
void format(CodeWriter writer, State state) {
Expand All @@ -78,7 +75,7 @@ class ListPiece extends Piece {
// });
// ```
switch (state) {
case State.initial:
case State.unsplit:
// All arguments on one line with no trailing comma.
writer.setAllowNewlines(false);
for (var i = 0; i < _arguments.length; i++) {
Expand Down
50 changes: 39 additions & 11 deletions lib/src/piece/piece.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,53 @@ import '../back_end/code_writer.dart';
/// formatting and line splitting. The final output is then determined by
/// deciding which pieces split and how.
abstract class Piece {
/// The ordered list of indexes identifying each way this piece can split.
/// The ordered list of ways this piece may split.
///
/// Each piece determines what each value in the list represents. The list
/// returned by this function should be sorted so that earlier states in the
/// list compare less than later states.
/// If the piece is aready pinned, then this is just the one pinned state.
/// Otherwise, it's [State.unsplit], which all pieces support, followed by
/// any other [additionalStates].
List<State> get states {
if (_pinnedState case var state?) {
return [state];
} else {
return [State.unsplit, ...additionalStates];
}
}

/// The ordered list of all possible ways this piece could split.
///
/// Piece subclasses should override this if they support being split in
/// multiple different ways.
///
/// Each piece determines what each [State] in the list represents, including
/// the automatically included [State.unsplit]. The list returned by this
/// function should be sorted so that earlier states in the list compare less
/// than later states.
List<State> get additionalStates => const [];

/// If this piece has been pinned to a specific state, that state.
///
/// In addition to the values returned here, each piece should implicitly
/// support a [State.initial] which is the least split form the piece allows.
List<State> get states;
/// This is used when a piece which otherwise supports multiple ways of
/// splitting should be eagerly constrained to a specific splitting choice
/// because of the context where it appears. For example, if conditional
/// expressions are nested, then all of them are forced to split because it's
/// too hard to read nested conditionals all on one line. We can express that
/// by pinning the Piece used for a conditional expression to its split state
/// when surrounded by or containing other conditionals.
State? get pinnedState => _pinnedState;
State? _pinnedState;

/// Given that this piece is in [state], use [writer] to produce its formatted
/// output.
void format(CodeWriter writer, State state);

/// Invokes [callback] on each piece contained in this piece.
void forEachChild(void Function(Piece piece) callback);

/// Forces this piece to always use [state].
void pin(State state) {
_pinnedState = state;
}
}

/// A simple atomic piece of code.
Expand All @@ -49,9 +80,6 @@ class TextPiece extends Piece {
/// multiline strings, etc.
bool _containsNewline = false;

@override
List<State> get states => const [];

/// Whether the last line of this piece's text ends with [text].
bool endsWith(String text) => _lines.isNotEmpty && _lines.last.endsWith(text);

Expand Down Expand Up @@ -96,7 +124,7 @@ class TextPiece extends Piece {
/// Each state identifies one way that a piece can be split into multiple lines.
/// Each piece determines how its states are interpreted.
class State implements Comparable<State> {
static const initial = State(0, cost: 0);
static const unsplit = State(0, cost: 0);

/// The maximally split state a piece can be in.
///
Expand Down
4 changes: 2 additions & 2 deletions lib/src/piece/postfix.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ class PostfixPiece extends Piece {
PostfixPiece(this.pieces);

@override
List<State> get states => const [State.split];
List<State> get additionalStates => const [State.split];

@override
void format(CodeWriter writer, State state) {
// If any of the operands split, then force the postfix sequence to split
// too.
// TODO(tall): This will need to be revisited when we use PostfixPiece for
// actual postfix operators where this isn't always desired.
if (state == State.initial) writer.setAllowNewlines(false);
if (state == State.unsplit) writer.setAllowNewlines(false);

for (var piece in pieces) {
writer.splitIf(state == State.split, indent: Indent.expression);
Expand Down
3 changes: 0 additions & 3 deletions lib/src/piece/sequence.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ class SequencePiece extends Piece {
/// Whether this sequence has any contents.
bool get isNotEmpty => _contents.isNotEmpty;

@override
List<State> get states => const [];

@override
void format(CodeWriter writer, State state) {
for (var i = 0; i < _contents.length; i++) {
Expand Down
6 changes: 3 additions & 3 deletions lib/src/piece/variable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class VariablePiece extends Piece {
: _hasType = hasType;

@override
List<State> get states => [
List<State> get additionalStates => [
if (_variables.length > 1) _betweenVariables,
if (_hasType) _afterType,
];
Expand All @@ -73,7 +73,7 @@ class VariablePiece extends Piece {
if (state == _betweenVariables) writer.setIndent(Indent.expression);

// Force variables to split if an initializer does.
if (_variables.length > 1 && state == State.initial) {
if (_variables.length > 1 && state == State.unsplit) {
writer.setAllowNewlines(false);
}

Expand All @@ -82,7 +82,7 @@ class VariablePiece extends Piece {

for (var i = 0; i < _variables.length; i++) {
// Split between variables.
if (i > 0) writer.splitIf(state != State.initial);
if (i > 0) writer.splitIf(state != State.unsplit);

writer.format(_variables[i]);
}
Expand Down

0 comments on commit 15fef68

Please sign in to comment.