Skip to content

Commit fc29f83

Browse files
authored
Format switch cases that aren't valid patterns. (#1177)
* Better style for inline case bodies. In the previous PR, any case body that fit on one line was allowed to even if other cases in the same switch didn't. I tested it on a corpus and I found that led to confusing switches where it wasn't always clear where the case body starts. I think you really want it all or nothing: either every single case fits on the same line in which case you can make the whole switch compact, or every case should be on its own line, even the ones that would fit. Unfortunately, it's a little tricky to have formatter rules that span code containing hard splits, so getting that working took some doing. It also regressed performance pretty badly. But I figured out some optimizations in ChunkBuilder and it's basically back to the same performance it had before. Also, this incidentally fixes a bug where parameter metadata in trailing comma parameter lists was also supposed to have that same all-or-nothing splitting logic but didn't. I've tried this on a corpus and I'm pretty happy with the results. Right now, relatively few switches benefit because the mandatory breaks mean a lot of switches have at least two statements (which always causes the case to split). But as those breaks are removed, I think we'll see more compact switches. Even today, this code does improve some switches where every case is just a short return statement. * Format switch cases that aren't valid patterns. Fix #1164. The solution is kind of hacky, but users will probably never run into it and it avoids complicated the user experience of the formatter. To get this working, I had to update to analyzer 5.5.0 because 5.4.0 had an assert failure when it tried to parse an invalid switch case. But 5.5.0 also has a bug which is causing a couple of formatter tests to fail: dart-lang/sdk#51415. I'll probably wait until there's a fix for that out before this gets merged to master. Analyzer 5.5.0 also changes some of the AST types. Refactored how binary expressions and patterns are formatted to avoid copy/paste from that change. * Better docs.
1 parent c9e5191 commit fc29f83

File tree

4 files changed

+260
-129
lines changed

4 files changed

+260
-129
lines changed

lib/src/dart_formatter.dart

Lines changed: 57 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import 'dart:math' as math;
55

66
import 'package:analyzer/dart/analysis/features.dart';
7+
import 'package:analyzer/dart/analysis/results.dart';
78
import 'package:analyzer/dart/analysis/utilities.dart';
89
import 'package:analyzer/dart/ast/ast.dart';
910
import 'package:analyzer/dart/ast/token.dart';
@@ -83,25 +84,11 @@ class DartFormatter {
8384
/// Returns a new [SourceCode] containing the formatted code and the resulting
8485
/// selection, if any.
8586
SourceCode formatSource(SourceCode source) {
86-
// Enable all features that are enabled by default in the current analyzer
87-
// version.
88-
// TODO(paulberry): consider plumbing in experiment enable flags from the
89-
// command line.
90-
var featureSet = FeatureSet.fromEnableFlags2(
91-
sdkLanguageVersion: Version(2, 19, 0),
92-
flags: [
93-
// TODO(rnystrom): This breaks existing switch cases containing constant
94-
// expressions that aren't valid patterns. See:
95-
// https://github.com/dart-lang/dart_style/issues/1164
96-
'patterns',
97-
'records',
98-
'unnamed-libraries',
99-
],
100-
);
101-
10287
var inputOffset = 0;
10388
var text = source.text;
10489
var unitSourceCode = source;
90+
91+
// If we're parsing a single statement, wrap the source in a fake function.
10592
if (!source.isCompilationUnit) {
10693
var prefix = 'void foo() { ';
10794
inputOffset = prefix.length;
@@ -118,12 +105,19 @@ class DartFormatter {
118105
}
119106

120107
// Parse it.
121-
var parseResult = parseString(
122-
content: text,
123-
featureSet: featureSet,
124-
path: source.uri,
125-
throwIfDiagnostics: false,
126-
);
108+
var parseResult = _parse(text, source.uri, patterns: true);
109+
110+
// If we couldn't parse it with patterns enabled, it may be because of
111+
// one of the breaking syntax changes to switch cases. Try parsing it
112+
// again without patterns.
113+
if (parseResult.errors.isNotEmpty) {
114+
var withoutPatternsResult = _parse(text, source.uri, patterns: false);
115+
116+
// If we succeeded this time, use this parse instead.
117+
if (withoutPatternsResult.errors.isEmpty) {
118+
parseResult = withoutPatternsResult;
119+
}
120+
}
127121

128122
// Infer the line ending if not given one. Do it here since now we know
129123
// where the lines start.
@@ -183,4 +177,45 @@ class DartFormatter {
183177

184178
return output;
185179
}
180+
181+
/// Parse [source] from [uri].
182+
///
183+
/// If [patterns] is `true`, the parse at the latest language version
184+
/// which supports patterns and treats switch cases as patterns. If `false`,
185+
/// then parses using an older language version where switch cases are
186+
/// constant expressions.
187+
///
188+
// TODO(rnystrom): This is a pretty big hack. Up until now, every language
189+
// version was a strict syntactic superset of all previous versions. That let
190+
// the formatter parse every file at the latest language version without
191+
// having to detect each file's actual version, which requires digging around
192+
// in the file system for package configs and looking for "@dart" comments in
193+
// files. It also means the library API that parses arbitrary strings doesn't
194+
// have to worry about what version the code should be interpreted as.
195+
//
196+
// But with patterns, a small number of switch cases are no longer
197+
// syntactically valid. Breakage from this is very rare. Instead of adding
198+
// the machinery to detect language versions (which is likely to be slow and
199+
// brittle), we just try parsing everything with patterns enabled. When a
200+
// parse error occurs, we try parsing it again with pattern disabled. If that
201+
// happens to parse without error, then we use that result instead.
202+
ParseStringResult _parse(String source, String? uri, {required bool patterns}) {
203+
// Enable all features that are enabled by default in the current analyzer
204+
// version.
205+
var featureSet = FeatureSet.fromEnableFlags2(
206+
sdkLanguageVersion: Version(2, 19, 0),
207+
flags: [
208+
if (patterns) 'patterns',
209+
'records',
210+
'unnamed-libraries',
211+
],
212+
);
213+
214+
return parseString(
215+
content: source,
216+
featureSet: featureSet,
217+
path: uri,
218+
throwIfDiagnostics: false,
219+
);
220+
}
186221
}

0 commit comments

Comments
 (0)