Skip to content

Commit

Permalink
Handle imports with "as" and "if" clauses in either order. (#1555)
Browse files Browse the repository at this point in the history
PR #1550 fixed the bug where if you had both clauses, it would output
them in the *wrong* order. But it turns out that the parser today allows
them in *either* order even though the language spec doesn't.

Since I've seen a handful of cases like this in the wild, instead of
just failing to format, simply preserve the clause order that the user
wrote.
  • Loading branch information
munificent authored Sep 4, 2024
1 parent 953ecbc commit d3b5aed
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 6 deletions.
26 changes: 23 additions & 3 deletions lib/src/front_end/piece_factory.dart
Original file line number Diff line number Diff line change
Expand Up @@ -812,8 +812,26 @@ mixin PieceFactory {
pieces.visit(directive.uri);
});

// Include the `as` clause.
// Add all of the clauses and combinators.
var clauses = <Piece>[];

// The language specifies that configurations must appear after any `as`
// clause but the parser incorrectly accepts them before it and code in
// the wild relies on that. Instead of failing with an "unexpected output"
// error, just preserve the order of the clauses if they are out of order.
// See: https://github.com/dart-lang/sdk/issues/56641
var wroteConfigurations = false;
if (directive.configurations.isNotEmpty &&
asKeyword != null &&
directive.configurations.first.ifKeyword.offset < asKeyword.offset) {
for (var configuration in directive.configurations) {
clauses.add(nodePiece(configuration));
}

wroteConfigurations = true;
}

// Include the `as` clause.
if (asKeyword != null) {
clauses.add(pieces.build(() {
pieces.token(deferredKeyword, spaceAfter: true);
Expand All @@ -824,8 +842,10 @@ mixin PieceFactory {
}

// Include any `if` clauses.
for (var configuration in directive.configurations) {
clauses.add(nodePiece(configuration));
if (!wroteConfigurations) {
for (var configuration in directive.configurations) {
clauses.add(nodePiece(configuration));
}
}

// Include the `show` and `hide` clauses.
Expand Down
18 changes: 17 additions & 1 deletion lib/src/short/source_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1955,6 +1955,19 @@ class SourceVisitor extends ThrowingAstVisitor {
space();
visit(node.uri);

// The language specifies that configurations must appear after any `as`
// clause but the parser incorrectly accepts them before it and code in
// the wild relies on that. Instead of failing with an "unexpected output"
// error, just preserve the order of the clauses if they are out of order.
// See: https://github.com/dart-lang/sdk/issues/56641
var wroteConfigurations = false;
if (node.asKeyword case var asKeyword?
when node.configurations.isNotEmpty &&
node.configurations.first.ifKeyword.offset < asKeyword.offset) {
_visitConfigurations(node.configurations);
wroteConfigurations = true;
}

if (node.asKeyword != null) {
soloSplit();
token(node.deferredKeyword, after: space);
Expand All @@ -1963,7 +1976,10 @@ class SourceVisitor extends ThrowingAstVisitor {
visit(node.prefix);
}

_visitConfigurations(node.configurations);
if (!wroteConfigurations) {
_visitConfigurations(node.configurations);
}

_visitCombinators(node.combinators);
});
}
Expand Down
11 changes: 10 additions & 1 deletion test/short/whitespace/directives.unit
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,13 @@ part of 'uri.dart';
import 'foo.dart' as foo if (config == 'value') 'other.dart';
<<<
import 'foo.dart' as foo
if (config == 'value') 'other.dart';
if (config == 'value') 'other.dart';
>>> Configuration before prefix.
### This violates the language spec, but the parser currently allows it without
### reporting an error and code in the wild relies on that. So the formatter
### handles it gracefully. See: https://github.com/dart-lang/sdk/issues/56641
import 'foo.dart' if (config == 'value') 'other.dart' as foo;
<<<
import 'foo.dart'
if (config == 'value') 'other.dart'
as foo;
11 changes: 10 additions & 1 deletion test/tall/top_level/import.unit
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,13 @@ import 'foo.dart' as foo if (config == 'value') 'other.dart';
<<<
import 'foo.dart'
as foo
if (config == 'value') 'other.dart';
if (config == 'value') 'other.dart';
>>> Configuration before prefix.
### This violates the language spec, but the parser currently allows it without
### reporting an error and code in the wild relies on that. So the formatter
### handles it gracefully. See: https://github.com/dart-lang/sdk/issues/56641
import 'foo.dart' if (config == 'value') 'other.dart' as foo;
<<<
import 'foo.dart'
if (config == 'value') 'other.dart'
as foo;

0 comments on commit d3b5aed

Please sign in to comment.