Tweak formatting rules. Fix #211.
Discard open spans when a hard split occurs.
This is a correctness issue, not a performance one. The previous
code would leave spans open even when a hard split occurred in
them. These were mostly innocuous since they ended up with
totally bogus ranges, but they apparently do affect some
corner-case formatting.
Some of this was triggered by the previous patch which was for
perf but also made some stuff more correct.
BUG=https://github.com/dart-lang/dart_style/issues/
R=pquitslund@google.com
Review URL: https://chromiumcodereview.appspot.com//1000513002
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 10ba9d2..78a6042 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,10 @@
* Update to latest analyzer (#177).
* Don't discard annotations on initializing formals (#197).
* Optimize formatting deeply nested expressions (#108).
+* Discard unused nesting level to improve performance (#108).
+* Discard unused spans to improve performance (#108).
+* Try to avoid splitting single-element lists (#211).
+* Avoid splitting when the first argument is a function expression (#211).
# 0.1.6
diff --git a/lib/src/chunk.dart b/lib/src/chunk.dart
index e060427..307cd95 100644
--- a/lib/src/chunk.dart
+++ b/lib/src/chunk.dart
@@ -230,9 +230,19 @@
/// greater number of nested spans.
static const normal = 1;
- /// The cost of splitting after a "=" both for assignment and initialization.
+ /// Splitting after a "=" both for assignment and initialization.
static const assignment = 2;
+ /// Splitting before the first argument when it happens to be a function
+ /// expression with a block body.
+ static const firstBlockArgument = 2;
+
+ /// The series of positional arguments.
+ static const positionalArguments = 2;
+
+ /// Splitting inside the brackets of a list with only one element.
+ static const singleElementList = 2;
+
/// The cost of a single character that goes past the page limit.
///
/// This cost is high to ensure any solution that fits in the page is
@@ -267,7 +277,8 @@
final implies = <SplitParam>[];
/// Creates a new [SplitParam].
- SplitParam([this.cost = Cost.normal]);
+ SplitParam([int cost])
+ : cost = cost != null ? cost : Cost.normal;
String toString() => "$id";
diff --git a/lib/src/line_writer.dart b/lib/src/line_writer.dart
index 0477e2f..3169aaa 100644
--- a/lib/src/line_writer.dart
+++ b/lib/src/line_writer.dart
@@ -74,7 +74,7 @@
/// The nested stack of spans that are currently being written.
final _openSpans = <Span>[];
- /// All of the spans that have been created, open and closed.
+ /// All of the spans that have been created and closed.
final _spans = <Span>[];
/// The current indentation and nesting levels.
@@ -371,6 +371,10 @@
/// Ends the innermost span.
void endSpan() {
var span = _openSpans.removeLast();
+
+ // If the span was discarded while it was still open, just forget about it.
+ if (span == null) return;
+
span.close(_currentChunkIndex);
// A span that just covers a single chunk can't be split anyway.
@@ -381,8 +385,9 @@
/// Starts a new [Multisplit].
///
/// Returns the [SplitParam] for the multisplit.
- SplitParam startMultisplit({bool separable}) {
- var multisplit = new Multisplit(_currentChunkIndex, separable: separable);
+ SplitParam startMultisplit({bool separable, int cost}) {
+ var multisplit = new Multisplit(_currentChunkIndex,
+ separable: separable, cost: cost);
_multisplits.add(multisplit);
return multisplit.param;
@@ -423,7 +428,7 @@
/// In particular, it's easy for the visitor to know that collections with a
/// large number of items must split. Doing that early avoids crashing the
/// splitter when it tries to recurse on huge collection literals.
- void preemptMultisplits() => _splitMultisplits();
+ void preemptMultisplits() => _handleHardSplit();
/// Increases the level of expression nesting.
///
@@ -622,7 +627,7 @@
_chunks.last.applySplit(indent, nesting, param,
isDouble: isDouble, spaceWhenUnsplit: spaceWhenUnsplit);
- if (_chunks.last.isHardSplit) _splitMultisplits();
+ if (_chunks.last.isHardSplit) _handleHardSplit();
}
/// Writes [text] to either the current chunk or a new one if the current
@@ -677,11 +682,6 @@
void _completeLine(int length) {
assert(_chunks.isNotEmpty);
- if (debugFormatter) {
- dumpChunks(_chunks.take(length).toList());
- print(_spans.join("\n"));
- }
-
// Write the newlines required by the previous line.
for (var i = 0; i < _bufferedNewlines; i++) {
_buffer.write(_formatter.lineEnding);
@@ -697,6 +697,11 @@
spans = spans.where((span) => span.start <= length).toList();
}
+ if (debugFormatter) {
+ dumpChunks(chunks);
+ print(spans.join("\n"));
+ }
+
var splitter = new LineSplitter(_formatter.lineEnding, _formatter.pageWidth,
chunks, spans, _beginningIndent);
var selection = splitter.apply(_buffer);
@@ -705,13 +710,21 @@
if (selection[1] != null) _selectionLength = selection[1] - _selectionStart;
}
- /// Handles multisplits when a hard line occurs.
+ /// Handles open multisplits and spans when a hard line occurs.
///
/// Any active separable multisplits will get split in two at this point.
/// Other multisplits are forced into the "hard" state. All of their previous
/// splits are turned into explicit hard splits and any new splits for that
/// multisplit become hard splits too.
- void _splitMultisplits() {
+ ///
+ /// All open spans get discarded since they will never be satisfied.
+ void _handleHardSplit() {
+ // Discard all open spans. We don't remove them from the stack so that we
+ // can still correctly count later calls to endSpan().
+ for (var i = 0; i < _openSpans.length; i++) {
+ _openSpans[i] = null;
+ }
+
if (_multisplits.isEmpty) return;
var splitParams = new Set();
diff --git a/lib/src/multisplit.dart b/lib/src/multisplit.dart
index fa2bf2c..6ad598d 100644
--- a/lib/src/multisplit.dart
+++ b/lib/src/multisplit.dart
@@ -53,15 +53,16 @@
/// The [SplitParam] that controls all of the split chunks.
SplitParam get param => _param;
- SplitParam _param = new SplitParam();
+ SplitParam _param;
/// `true` if a hard newline has forced this multisplit to be split.
bool _isSplit = false;
final bool _separable;
- Multisplit(this.startChunk, {bool separable})
- : _separable = separable != null ? separable : false;
+ Multisplit(this.startChunk, {bool separable, int cost})
+ : _separable = separable != null ? separable : false,
+ _param = new SplitParam(cost);
/// Handles a hard split occurring in the middle of this multisplit.
///
@@ -78,7 +79,7 @@
_isSplit = true;
if (_separable) {
- _param = new SplitParam(param.cost);
+ _param = new SplitParam(_param.cost);
// Previous splits may still remain unsplit.
return null;
diff --git a/lib/src/source_visitor.dart b/lib/src/source_visitor.dart
index 4fc1918..ec9ca70 100644
--- a/lib/src/source_visitor.dart
+++ b/lib/src/source_visitor.dart
@@ -105,13 +105,51 @@
// Nest around the parentheses in case there are comments before or after
// them.
_writer.nestExpression();
+
token(node.leftParenthesis);
+ // Corner case: If the first argument to a method is a block-bodied
+ // function, it looks bad if its parameter list gets wrapped to the next
+ // line. Bump the cost to try to avoid that. This prefers:
+ //
+ // receiver
+ // .method()
+ // .chain((parameter, list) {
+ // ...
+ // });
+ //
+ // over:
+ //
+ // receiver.method().chain(
+ // (parameter, list) {
+ // ...
+ // });
+ // TODO(rnystrom): This causes a function expression's long parameter list
+ // to get split instead, like:
+ //
+ // receiver.method((longParameter,
+ // anotherParameter) {
+ // ...
+ // });
+ //
+ // Instead of bumping the cost, this should wrap a span around the "("
+ // before the argument list and the function's parameter list. That requires
+ // spans to not strictly be a stack, though, so would be a larger change
+ // than I want to do right now.
+ var cost = Cost.normal;
+ if (node.arguments.isNotEmpty) {
+ var firstArg = node.arguments.first;
+ if (firstArg is FunctionExpression &&
+ firstArg.body is BlockFunctionBody) {
+ cost = Cost.firstBlockArgument;
+ }
+ }
+
// Allow splitting after "(".
- var lastParam = zeroSplit();
+ var lastParam = zeroSplit(cost);
// Try to keep the positional arguments together.
- _writer.startSpan();
+ _writer.startSpan(Cost.positionalArguments);
var i = 0;
for (; i < node.arguments.length; i++) {
@@ -979,8 +1017,11 @@
}
visitListLiteral(ListLiteral node) {
+ // Corner case: Splitting inside a list looks bad if there's only one
+ // element, so make those more costly.
+ var cost = node.elements.length <= 1 ? Cost.singleElementList : Cost.normal;
_visitCollectionLiteral(
- node, node.leftBracket, node.elements, node.rightBracket);
+ node, node.leftBracket, node.elements, node.rightBracket, cost);
}
visitMapLiteral(MapLiteral node) {
@@ -1542,11 +1583,11 @@
/// Visits the collection literal [node] whose body starts with [leftBracket],
/// ends with [rightBracket] and contains [elements].
void _visitCollectionLiteral(TypedLiteral node, Token leftBracket,
- Iterable<AstNode> elements, Token rightBracket) {
+ Iterable<AstNode> elements, Token rightBracket, [int cost]) {
modifier(node.constKeyword);
visit(node.typeArguments);
- _startBody(leftBracket);
+ _startBody(leftBracket, cost: cost);
// Each list element takes at least 3 characters (one character for the
// element, one for the comma, one for the space), so force it to split if
@@ -1649,11 +1690,11 @@
///
/// If [space] is `true`, then the initial multisplit will use a space if not
/// split.
- void _startBody(Token leftBracket, {bool space: false}) {
+ void _startBody(Token leftBracket, {int cost, bool space: false}) {
token(leftBracket);
// Indent the body.
- _writer.startMultisplit();
+ _writer.startMultisplit(cost: cost);
_writer.indent();
// Split after the bracket.
@@ -1746,7 +1787,7 @@
/// Writes a split that is the empty string when unsplit.
///
/// Returns the newly created [SplitParam].
- SplitParam zeroSplit() => _writer.writeSplit();
+ SplitParam zeroSplit([int cost]) => _writer.writeSplit(cost: cost);
/// Emit [token], along with any comments and formatted whitespace that comes
/// before it.
diff --git a/test/regression/121.stmt b/test/regression/121.stmt
index b381bdf..e834810 100644
--- a/test/regression/121.stmt
+++ b/test/regression/121.stmt
@@ -14,7 +14,9 @@
if (!firstArgument.isString) {
DartType type = defaultValue.getType(compiler.coreTypes);
compiler.reportFatalError(send.arguments.head,
- MessageKind.NOT_ASSIGNABLE,
- {'fromType': type, 'toType': compiler.stringClass.rawType});
+ MessageKind.NOT_ASSIGNABLE, {
+ 'fromType': type,
+ 'toType': compiler.stringClass.rawType
+ });
return null;
}
\ No newline at end of file
diff --git a/test/regression/211.unit b/test/regression/211.unit
new file mode 100644
index 0000000..721bc1e
--- /dev/null
+++ b/test/regression/211.unit
@@ -0,0 +1,69 @@
+>>> (indent 2)
+ LiteralMap mapLiteral(List<LiteralMapEntry> entries, {bool isConst: false}) {
+ return new LiteralMap(null, // Type arguments.
+ new NodeList(symbolToken(OPEN_CURLY_BRACKET_INFO),
+ linkedList(entries), symbolToken(CLOSE_CURLY_BRACKET_INFO), ','),
+ isConst ? keywordToken('const') : null);
+ }
+<<<
+ LiteralMap mapLiteral(List<LiteralMapEntry> entries, {bool isConst: false}) {
+ return new LiteralMap(null, // Type arguments.
+ new NodeList(symbolToken(OPEN_CURLY_BRACKET_INFO), linkedList(entries),
+ symbolToken(CLOSE_CURLY_BRACKET_INFO), ','),
+ isConst ? keywordToken('const') : null);
+ }
+>>>
+void defineProperty(var obj, String property, var value) {
+ JS('void', 'Object.defineProperty(#, #, '
+ '{value: #, enumerable: false, writable: true, configurable: true})',
+ obj, property, value);
+}
+<<<
+void defineProperty(var obj, String property, var value) {
+ JS('void', 'Object.defineProperty(#, #, '
+ '{value: #, enumerable: false, writable: true, configurable: true})', obj,
+ property, value);
+}
+>>> (indent 4)
+ main() {
+ return searchEngine.searchTopLevelDeclarations('').then(
+ (List<SearchMatch> matches) {
+ _addNonImportedElementSuggestions(matches, excludedLibs);
+ return true;
+ });
+ }
+<<<
+ main() {
+ return searchEngine
+ .searchTopLevelDeclarations('')
+ .then((List<SearchMatch> matches) {
+ _addNonImportedElementSuggestions(matches, excludedLibs);
+ return true;
+ });
+ }
+>>> (indent 2)
+ test_getRelationships_empty() {
+ return store.getRelationships(elementA, relationship).then(
+ (List<Location> locations) {
+ expect(locations, isEmpty);
+ });
+ }
+<<<
+ test_getRelationships_empty() {
+ return store
+ .getRelationships(elementA, relationship)
+ .then((List<Location> locations) {
+ expect(locations, isEmpty);
+ });
+ }
+>>> (indent 2)
+ _asRuntimeType() {
+ return [
+ _class._jsConstructor
+ ].addAll(typeArguments.map((t) => t._asRuntimeType()));
+ }
+<<<
+ _asRuntimeType() {
+ return [_class._jsConstructor]
+ .addAll(typeArguments.map((t) => t._asRuntimeType()));
+ }
\ No newline at end of file
