Skip to content

Commit

Permalink
Format switch cases that aren't valid patterns. (dart-lang#1177)
Browse files Browse the repository at this point in the history
* 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 dart-lang#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.
  • Loading branch information
munificent authored Feb 15, 2023
1 parent c9e5191 commit fc29f83
Show file tree
Hide file tree
Showing 4 changed files with 260 additions and 129 deletions.
79 changes: 57 additions & 22 deletions lib/src/dart_formatter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import 'dart:math' as math;

import 'package:analyzer/dart/analysis/features.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/token.dart';
Expand Down Expand Up @@ -83,25 +84,11 @@ class DartFormatter {
/// Returns a new [SourceCode] containing the formatted code and the resulting
/// selection, if any.
SourceCode formatSource(SourceCode source) {
// Enable all features that are enabled by default in the current analyzer
// version.
// TODO(paulberry): consider plumbing in experiment enable flags from the
// command line.
var featureSet = FeatureSet.fromEnableFlags2(
sdkLanguageVersion: Version(2, 19, 0),
flags: [
// TODO(rnystrom): This breaks existing switch cases containing constant
// expressions that aren't valid patterns. See:
// https://github.com/dart-lang/dart_style/issues/1164
'patterns',
'records',
'unnamed-libraries',
],
);

var inputOffset = 0;
var text = source.text;
var unitSourceCode = source;

// If we're parsing a single statement, wrap the source in a fake function.
if (!source.isCompilationUnit) {
var prefix = 'void foo() { ';
inputOffset = prefix.length;
Expand All @@ -118,12 +105,19 @@ class DartFormatter {
}

// Parse it.
var parseResult = parseString(
content: text,
featureSet: featureSet,
path: source.uri,
throwIfDiagnostics: false,
);
var parseResult = _parse(text, source.uri, patterns: true);

// If we couldn't parse it with patterns enabled, it may be because of
// one of the breaking syntax changes to switch cases. Try parsing it
// again without patterns.
if (parseResult.errors.isNotEmpty) {
var withoutPatternsResult = _parse(text, source.uri, patterns: false);

// If we succeeded this time, use this parse instead.
if (withoutPatternsResult.errors.isEmpty) {
parseResult = withoutPatternsResult;
}
}

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

return output;
}

/// Parse [source] from [uri].
///
/// If [patterns] is `true`, the parse at the latest language version
/// which supports patterns and treats switch cases as patterns. If `false`,
/// then parses using an older language version where switch cases are
/// constant expressions.
///
// TODO(rnystrom): This is a pretty big hack. Up until now, every language
// version was a strict syntactic superset of all previous versions. That let
// the formatter parse every file at the latest language version without
// having to detect each file's actual version, which requires digging around
// in the file system for package configs and looking for "@dart" comments in
// files. It also means the library API that parses arbitrary strings doesn't
// have to worry about what version the code should be interpreted as.
//
// But with patterns, a small number of switch cases are no longer
// syntactically valid. Breakage from this is very rare. Instead of adding
// the machinery to detect language versions (which is likely to be slow and
// brittle), we just try parsing everything with patterns enabled. When a
// parse error occurs, we try parsing it again with pattern disabled. If that
// happens to parse without error, then we use that result instead.
ParseStringResult _parse(String source, String? uri, {required bool patterns}) {
// Enable all features that are enabled by default in the current analyzer
// version.
var featureSet = FeatureSet.fromEnableFlags2(
sdkLanguageVersion: Version(2, 19, 0),
flags: [
if (patterns) 'patterns',
'records',
'unnamed-libraries',
],
);

return parseString(
content: source,
featureSet: featureSet,
path: uri,
throwIfDiagnostics: false,
);
}
}
Loading

0 comments on commit fc29f83

Please sign in to comment.