Skip to content

Commit

Permalink
Allow rule values to affect the indentation of lines.
Browse files Browse the repository at this point in the history 
This is used in the rare cases where an argument list containing
collection literals splits in such a way that they should *not* get
the statement-level indentation they usually have.

Fix #421. Fix #469.

R=kevmoo@google.com

Review URL: https://codereview.chromium.org//1471683003 .
  • Loading branch information
@munificent
munificent committed Nov 24, 2015
1 parent 8899bd0 commit e6155f2
Show file tree
Hide file tree
Showing 12 changed files with 292 additions and 101 deletions.
81 changes: 46 additions & 35 deletions lib/src/argument_list_visitor.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
library dart_style.src.argument_list_visitor;

import 'package:analyzer/analyzer.dart';
import 'package:analyzer/src/generated/scanner.dart';

import 'chunk.dart';
import 'rule/argument.dart';
Expand Down Expand Up @@ -207,8 +208,9 @@ class ArgumentSublist {
/// The named arguments, in order.
final List<Expression> _named;

/// The arguments that are collection literals that get special formatting.
final Set<Expression> _collections;
/// Maps each argument that is a collection literal that get special
/// formatting to the token for the collection's open bracket.
final Map<Expression, Token> _collections;

/// The number of leading collections.
///
Expand All @@ -221,16 +223,12 @@ class ArgumentSublist {
final int _trailingCollections;

/// The rule used to split the bodies of all of the collection arguments.
Rule get _collectionRule {
// Lazy initialize.
if (_collectionRuleField == null && _collections.isNotEmpty) {
_collectionRuleField = new SimpleRule(cost: Cost.splitCollections);
}

return _collectionRuleField;
}
Rule get collectionRule => _collectionRule;
Rule _collectionRule;

Rule _collectionRuleField;
/// The most recent chunk that split before an argument.
Chunk get previousSplit => _previousSplit;
Chunk _previousSplit;

bool get _hasMultipleArguments => _positional.length + _named.length > 1;

Expand All @@ -241,20 +239,24 @@ class ArgumentSublist {
arguments.takeWhile((arg) => arg is! NamedExpression).toList();
var named = arguments.skip(positional.length).toList();

var collections = arguments.where(_isCollectionArgument).toSet();
var collections = {};
for (var argument in arguments) {
var bracket = _getCollectionBracket(argument);
if (bracket != null) collections[argument] = bracket;
}

// Count the leading arguments that are collection literals.
var leadingCollections = 0;
for (var argument in arguments) {
if (!collections.contains(argument)) break;
if (!collections.containsKey(argument)) break;
leadingCollections++;
}

// Count the trailing arguments that are collection literals.
var trailingCollections = 0;
if (leadingCollections != arguments.length) {
for (var argument in arguments.reversed) {
if (!collections.contains(argument)) break;
if (!collections.containsKey(argument)) break;
trailingCollections++;
}
}
Expand Down Expand Up @@ -287,6 +289,10 @@ class ArgumentSublist {
this._collections, this._leadingCollections, this._trailingCollections);

void visit(SourceVisitor visitor) {
if (_collections.isNotEmpty) {
_collectionRule = new SimpleRule(Cost.splitCollections);
}

var rule = _visitPositional(visitor);
_visitNamed(visitor, rule);
}
Expand All @@ -300,7 +306,7 @@ class ArgumentSublist {
if (_positional.length == 1) {
rule = new SinglePositionalRule(_collectionRule,
splitsOnInnerRules: _allArguments.length > 1 &&
!_isCollectionArgument(_positional.first));
!_collections.containsKey(_positional.first));
} else {
// Only count the positional bodies in the positional rule.
var leadingPositional = _leadingCollections;
Expand All @@ -315,13 +321,9 @@ class ArgumentSublist {

visitor.builder.startRule(rule);

var chunk;
if (_isFirstArgument(_positional.first)) {
chunk = visitor.zeroSplit();
} else {
chunk = visitor.split();
}
rule.beforeArgument(chunk);
_previousSplit =
visitor.builder.split(space: !_isFirstArgument(_positional.first));
rule.beforeArgument(_previousSplit);

// Try to not split the arguments.
visitor.builder.startSpan(Cost.positionalArguments);
Expand All @@ -331,7 +333,8 @@ class ArgumentSublist {

// Positional arguments split independently.
if (argument != _positional.last) {
rule.beforeArgument(visitor.split());
_previousSplit = visitor.split();
rule.beforeArgument(_previousSplit);
}
}

Expand All @@ -346,7 +349,9 @@ class ArgumentSublist {
if (_named.isEmpty) return;

var positionalRule = rule;
var namedRule = new NamedRule();
var namedRule = new NamedRule(
containsCollections: _leadingCollections > _positional.length ||
_trailingCollections > 0);
visitor.builder.startRule(namedRule);

// Let the positional args force the named ones to split.
Expand All @@ -355,14 +360,15 @@ class ArgumentSublist {
}

// Split before the first named argument.
namedRule.beforeArguments(
visitor.builder.split(space: !_isFirstArgument(_named.first)));
_previousSplit =
visitor.builder.split(space: !_isFirstArgument(_named.first));
namedRule.beforeArguments(_previousSplit);

for (var argument in _named) {
_visitArgument(visitor, namedRule, argument);

// Write the split.
if (argument != _named.last) visitor.split();
if (argument != _named.last) _previousSplit = visitor.split();
}

visitor.builder.endRule();
Expand All @@ -371,14 +377,14 @@ class ArgumentSublist {
void _visitArgument(
SourceVisitor visitor, ArgumentRule rule, Expression argument) {
// If we're about to write a collection argument, handle it specially.
if (_collections.contains(argument)) {
if (_collections.containsKey(argument)) {
if (rule != null) rule.beforeCollection();

// Tell it to use the rule we've already created.
visitor.setNextLiteralBodyRule(_collectionRule);
visitor.beforeCollection(_collections[argument], this);
} else if (_hasMultipleArguments) {
// Corner case: If there is just a single argument, don't bump the
// nesting. This lets us avoid spurious indentation in cases like:
// Edge case: If there is just a single argument, don't bump the nesting.
// This lets us avoid spurious indentation in cases like:
//
// function(function(() {
// body;
Expand All @@ -388,7 +394,7 @@ class ArgumentSublist {

visitor.visit(argument);

if (_collections.contains(argument)) {
if (_collections.containsKey(argument)) {
if (rule != null) rule.afterCollection();
} else if (_hasMultipleArguments) {
visitor.builder.endBlockArgumentNesting();
Expand All @@ -404,17 +410,22 @@ class ArgumentSublist {

bool _isLastArgument(Expression argument) => argument == _allArguments.last;

/// Returns true if [expression] denotes a collection literal argument.
/// Returns the token for the left bracket if [expression] denotes a
/// collection literal argument.
///
/// Similar to block functions, collection arguments can get special
/// indentation to make them look more statement-like.
static bool _isCollectionArgument(Expression expression) {
static Token _getCollectionBracket(Expression expression) {
if (expression is NamedExpression) {
expression = (expression as NamedExpression).expression;
}

// TODO(rnystrom): Should we step into parenthesized expressions?

return expression is ListLiteral || expression is MapLiteral;
if (expression is ListLiteral) return expression.leftBracket;
if (expression is MapLiteral) return expression.leftBracket;

// Not a collection literal.
return null;
}
}
56 changes: 47 additions & 9 deletions lib/src/chunk.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,13 @@ class Chunk extends Selection {
NestingLevel get nesting => _nesting;
NestingLevel _nesting;

/// If this chunk marks the beginning of a block, these are the chunks
/// contained in the block.
final blockChunks = <Chunk>[];
/// If this chunk marks the beginning of a block, this contains the child
/// chunks and other data about that nested block.
ChunkBlock get block => _block;
ChunkBlock _block;

/// Whether this chunk has a [block].
bool get isBlock => _block != null;

/// Whether it's valid to add more text to this chunk or not.
///
Expand Down Expand Up @@ -136,9 +140,9 @@ class Chunk extends Selection {
/// If `true`, then the line after this chunk and its contained block should
/// be flush left.
bool get flushLeftAfter {
if (blockChunks.isEmpty) return _flushLeft;
if (!isBlock) return _flushLeft;

return blockChunks.last.flushLeftAfter;
return _block.chunks.last.flushLeftAfter;
}

/// Whether this chunk should append an extra space if it does not split.
Expand All @@ -165,8 +169,10 @@ class Chunk extends Selection {
/// Does not include this chunk's own length, just the length of its child
/// block chunks (recursively).
int get unsplitBlockLength {
if (_block == null) return 0;

var length = 0;
for (var chunk in blockChunks) {
for (var chunk in _block.chunks) {
length += chunk.length + chunk.unsplitBlockLength;
}

Expand Down Expand Up @@ -229,6 +235,22 @@ class Chunk extends Selection {
if (_isDouble == null) _isDouble = isDouble;
}

/// Turns this chunk into one that can contain a block of child chunks.
void makeBlock(Chunk blockArgument) {
assert(_block == null);
_block = new ChunkBlock(blockArgument);
}

/// Returns `true` if the block body owned by this chunk should be expression
/// indented given a set of rule values provided by [getValue].
bool indentBlock(int getValue(Rule rule)) {
if (_block == null) return false;
if (_block.argument == null) return false;

return _block.argument.rule
.isSplit(getValue(_block.argument.rule), _block.argument);
}

// Mark whether this chunk can divide the range of chunks.
void markDivide(canDivide) {
// Should only do this once.
Expand All @@ -243,9 +265,9 @@ class Chunk extends Selection {
if (text.isNotEmpty) parts.add(text);

if (_indent != null) parts.add("indent:$_indent");
if (spaceWhenUnsplit) parts.add("space");
if (_isDouble) parts.add("double");
if (_flushLeft) parts.add("flush");
if (spaceWhenUnsplit == true) parts.add("space");
if (_isDouble == true) parts.add("double");
if (_flushLeft == true) parts.add("flush");

if (_rule == null) {
parts.add("(no split)");
Expand All @@ -263,6 +285,22 @@ class Chunk extends Selection {
}
}

/// The child chunks owned by a chunk that begins a "block" -- an actual block
/// statement, function expression, or collection literal.
class ChunkBlock {
/// If this block is for a collection literal in an argument list, this will
/// be the chunk preceding this literal argument.
///
/// That chunk is owned by the argument list and if it splits, this collection
/// may need extra expression-level indentation.
final Chunk argument;

/// The child chunks in this block.
final List<Chunk> chunks = [];

ChunkBlock(this.argument);
}

/// Constants for the cost heuristics used to determine which set of splits is
/// most desirable.
class Cost {
Expand Down
9 changes: 6 additions & 3 deletions lib/src/chunk_builder.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,9 +468,12 @@ class ChunkBuilder {
/// Starts a new block as a child of the current chunk.
///
/// Nested blocks are handled using their own independent [LineWriter].
ChunkBuilder startBlock() {
ChunkBuilder startBlock(Chunk argumentChunk) {
var chunk = _chunks.last;
chunk.makeBlock(argumentChunk);

var builder =
new ChunkBuilder._(this, _formatter, _source, _chunks.last.blockChunks);
new ChunkBuilder._(this, _formatter, _source, chunk.block.chunks);

// A block always starts off indented one level.
builder.indent();
Expand Down Expand Up @@ -733,7 +736,7 @@ class ChunkBuilder {
var chunk = _chunks[i];
if (!chunk.isHardSplit) return false;
if (chunk.nesting.isNested) return false;
if (chunk.blockChunks.isNotEmpty) return false;
if (chunk.isBlock) return false;

// Make sure we don't split the line in the middle of a rule.
var chunks = _ruleChunks[chunk.rule];
Expand Down
12 changes: 7 additions & 5 deletions lib/src/debug.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ void dumpChunks(int start, List<Chunk> chunks) {
for (var chunk in chunks) {
spans.addAll(chunk.spans);

addSpans(chunk.blockChunks);
if (chunk.isBlock) addSpans(chunk.block.chunks);
}
}

Expand Down Expand Up @@ -156,8 +156,10 @@ void dumpChunks(int start, List<Chunk> chunks) {

rows.add(row);

for (var j = 0; j < chunk.blockChunks.length; j++) {
addChunk(chunk.blockChunks, "$prefix$index.", j);
if (chunk.isBlock) {
for (var j = 0; j < chunk.block.chunks.length; j++) {
addChunk(chunk.block.chunks, "$prefix$index.", j);
}
}
}

Expand Down Expand Up @@ -204,7 +206,7 @@ void dumpLines(List<Chunk> chunks, int firstLineIndent, SplitSet splits) {
if (chunk.spaceWhenUnsplit) buffer.write(" ");

// Recurse into the block.
writeChunksUnsplit(chunk.blockChunks);
if (chunk.isBlock) writeChunksUnsplit(chunk.block.chunks);
}
}

Expand All @@ -220,7 +222,7 @@ void dumpLines(List<Chunk> chunks, int firstLineIndent, SplitSet splits) {
writeIndent(splits.getColumn(i));
}
} else {
writeChunksUnsplit(chunk.blockChunks);
if (chunk.isBlock) writeChunksUnsplit(chunk.block.chunks);

if (chunk.spaceWhenUnsplit) buffer.write(" ");
}
Expand Down
5 changes: 4 additions & 1 deletion lib/src/line_splitting/solve_state.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ library dart_style.src.line_splitting.solve_state;

import '../debug.dart' as debug;
import '../rule/rule.dart';
import '../whitespace.dart';
import 'line_splitter.dart';
import 'rule_set.dart';

Expand Down Expand Up @@ -307,6 +308,8 @@ class SolveState {

// And any expression nesting.
indent += chunk.nesting.totalUsedIndent;

if (chunk.indentBlock(getValue)) indent += Indent.expression;
}

_splits.add(i, indent);
Expand Down Expand Up @@ -373,7 +376,7 @@ class SolveState {
splitSpans.addAll(chunk.spans);

// Include the cost of the nested block.
if (chunk.blockChunks.isNotEmpty) {
if (chunk.isBlock) {
cost +=
_splitter.writer.formatBlock(chunk, _splits.getColumn(i)).cost;
}
Expand Down
Loading

0 comments on commit e6155f2

Please sign in to comment.