From fc29f837ff05c8e1dc9a1884918a8a6c4051c6d9 Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Tue, 14 Feb 2023 17:23:23 -0800 Subject: [PATCH] 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: https://github.com/dart-lang/sdk/issues/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. --- lib/src/dart_formatter.dart | 79 ++++++++---- lib/src/source_visitor.dart | 246 +++++++++++++++++++++--------------- pubspec.yaml | 2 +- test/whitespace/switch.stmt | 62 ++++++++- 4 files changed, 260 insertions(+), 129 deletions(-) diff --git a/lib/src/dart_formatter.dart b/lib/src/dart_formatter.dart index cddce79e..21fc30f2 100644 --- a/lib/src/dart_formatter.dart +++ b/lib/src/dart_formatter.dart @@ -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'; @@ -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; @@ -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. @@ -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, + ); + } } diff --git a/lib/src/source_visitor.dart b/lib/src/source_visitor.dart index 5f8d9d76..496a0037 100644 --- a/lib/src/source_visitor.dart +++ b/lib/src/source_visitor.dart @@ -347,8 +347,6 @@ class SourceVisitor extends ThrowingAstVisitor { @override void visitBinaryExpression(BinaryExpression node) { - builder.startSpan(); - // If a binary operator sequence appears immediately after a `=>`, don't // add an extra level of nesting. Instead, let the subsequent operands line // up with the first, as in: @@ -357,82 +355,14 @@ class SourceVisitor extends ThrowingAstVisitor { // argument && // argument && // argument; - var isArrowBody = node.parent is ExpressionFunctionBody; - if (!isArrowBody) builder.nestExpression(); - - // Start lazily so we don't force the operator to split if a line comment - // appears before the first operand. - builder.startLazyRule(); - - // Flatten out a tree/chain of the same precedence. If we need to split on - // any of them, we split on all of them. - var precedence = node.operator.type.precedence; - - void traverse(Expression e) { - if (e is BinaryExpression && e.operator.type.precedence == precedence) { - traverse(e.leftOperand); - - space(); - token(e.operator); - - split(); - traverse(e.rightOperand); - } else { - visit(e); - } - } - - // Blocks as operands to infix operators should always nest like regular - // operands. (Granted, this case is exceedingly rare in real code.) - builder.startBlockArgumentNesting(); + var nest = node.parent is! ExpressionFunctionBody; - traverse(node); - - builder.endBlockArgumentNesting(); - - if (!isArrowBody) builder.unnest(); - builder.endSpan(); - builder.endRule(); - } - - @override - void visitBinaryPattern(BinaryPattern node) { - builder.startSpan(); - builder.nestExpression(now: true); - - // Start lazily so we don't force the operator to split if a line comment - // appears before the first operand. - builder.startLazyRule(); - - // Flatten out a tree/chain of the same precedence. If we need to split on - // any of them, we split on all of them. - var precedence = node.operator.type.precedence; - - void traverse(DartPattern p) { - if (p is BinaryPattern && p.operator.type.precedence == precedence) { - traverse(p.leftOperand); - - space(); - token(p.operator); - - split(); - traverse(p.rightOperand); - } else { - visit(p); - } - } - - // Blocks as operands to infix patterns should always nest like regular - // operands. - builder.startBlockArgumentNesting(); - - traverse(node); - - builder.endBlockArgumentNesting(); - - builder.unnest(); - builder.endSpan(); - builder.endRule(); + _visitBinary( + node, + precedence: node.operator.type.precedence, + nest: nest, + (expression) => BinaryNode(expression.leftOperand, expression.operator, + expression.rightOperand)); } @override @@ -970,9 +900,7 @@ class SourceVisitor extends ThrowingAstVisitor { @override void visitDeclaredVariablePattern(DeclaredVariablePattern node) { - modifier(node.keyword); - visit(node.type, after: soloSplit); - token(node.name); + _visitVariablePattern(node.keyword, node.type, node.name); } @override @@ -2144,6 +2072,22 @@ class SourceVisitor extends ThrowingAstVisitor { typeArguments: node.typeArguments); } + @override + void visitLogicalAndPattern(LogicalAndPattern node) { + _visitBinary( + node, + (pattern) => BinaryNode( + pattern.leftOperand, pattern.operator, pattern.rightOperand)); + } + + @override + void visitLogicalOrPattern(LogicalOrPattern node) { + _visitBinary( + node, + (pattern) => BinaryNode( + pattern.leftOperand, pattern.operator, pattern.rightOperand)); + } + @override void visitMapLiteralEntry(MapLiteralEntry node) { builder.nestExpression(); @@ -2307,6 +2251,18 @@ class SourceVisitor extends ThrowingAstVisitor { }); } + @override + void visitNullAssertPattern(NullAssertPattern node) { + visit(node.pattern); + token(node.operator); + } + + @override + void visitNullCheckPattern(NullCheckPattern node) { + visit(node.pattern); + token(node.operator); + } + @override void visitNullLiteral(NullLiteral node) { token(node.literal); @@ -2399,6 +2355,25 @@ class SourceVisitor extends ThrowingAstVisitor { _visitAssignment(node.equals, node.expression); } + @override + void visitPatternField(PatternField node) { + var fieldName = node.name; + if (fieldName != null) { + var name = fieldName.name; + if (name != null) { + visitNamedNode(fieldName.name!, fieldName.colon, node.pattern); + } else { + // Named field with inferred name, like: + // + // var (:x) = (x: 1); + token(fieldName.colon); + visit(node.pattern); + } + } else { + visit(node.pattern); + } + } + @override void visitPatternVariableDeclaration(PatternVariableDeclaration node) { visitMetadata(node.metadata); @@ -2423,12 +2398,6 @@ class SourceVisitor extends ThrowingAstVisitor { token(node.operator); } - @override - void visitPostfixPattern(PostfixPattern node) { - visit(node.operand); - token(node.operator); - } - @override void visitPrefixedIdentifier(PrefixedIdentifier node) { CallChainVisitor(this, node).visit(); @@ -2488,25 +2457,6 @@ class SourceVisitor extends ThrowingAstVisitor { isRecord: true); } - @override - void visitRecordPatternField(RecordPatternField node) { - var fieldName = node.fieldName; - if (fieldName != null) { - var name = fieldName.name; - if (name != null) { - visitNamedNode(fieldName.name!, fieldName.colon, node.pattern); - } else { - // Named field with inferred name, like: - // - // var (:x) = (x: 1); - token(fieldName.colon); - visit(node.pattern); - } - } else { - visit(node.pattern); - } - } - @override void visitRecordTypeAnnotation(RecordTypeAnnotation node) { var namedFields = node.namedFields; @@ -3063,6 +3013,11 @@ class SourceVisitor extends ThrowingAstVisitor { _visitLoopBody(node.body); } + @override + void visitWildcardPattern(WildcardPattern node) { + _visitVariablePattern(node.keyword, node.type, node.name); + } + @override void visitWithClause(WithClause node) { _visitCombinator(node.withKeyword, node.mixinTypes); @@ -3205,6 +3160,67 @@ class SourceVisitor extends ThrowingAstVisitor { if (nest) builder.unnest(); } + /// Visits an infix operator-like AST node: a binary operator expression, or + /// binary pattern. + /// + /// In a tree of binary AST nodes, all operators at the same precedence are + /// treated as a single chain of operators that either all split or none do. + /// Operands within those (which may themselves be chains of higher + /// precedence binary operators) are then formatted independently. + /// + /// [T] is the type of node being visited and [destructureNode] is a callback + /// that takes one of those and yields the operands and operator. We need + /// this since there's no interface shared by the various binary operator + /// AST nodes. + /// + /// If [precedence] is given, then only flattens binary nodes with that same + /// precedence. If [nest] is `false`, then elides the nesting around the + /// expression. + void _visitBinary( + T node, BinaryNode Function(T node) destructureNode, + {int? precedence, bool nest = true}) { + builder.startSpan(); + + if (nest) builder.nestExpression(); + + // Start lazily so we don't force the operator to split if a line comment + // appears before the first operand. + builder.startLazyRule(); + + // Blocks as operands to infix operators should always nest like regular + // operands. (Granted, this case is exceedingly rare in real code.) + builder.startBlockArgumentNesting(); + + void traverse(AstNode e) { + if (e is! T) { + visit(e); + } else { + var binary = destructureNode(e); + if (precedence != null && + binary.operator.type.precedence != precedence) { + // Binary node, but a different precedence, so don't flatten. + visit(e); + } else { + traverse(binary.left); + + space(); + token(binary.operator); + + split(); + traverse(binary.right); + } + } + } + + traverse(node); + + builder.endBlockArgumentNesting(); + + if (nest) builder.unnest(); + builder.endSpan(); + builder.endRule(); + } + /// Visits the "with" and "implements" clauses in a type declaration. void _visitClauses( WithClause? withClause, ImplementsClause? implementsClause) { @@ -3283,6 +3299,13 @@ class SourceVisitor extends ThrowingAstVisitor { } } + /// Visits a variable or wildcard pattern. + void _visitVariablePattern(Token? keyword, TypeAnnotation? type, Token name) { + modifier(keyword); + visit(type, after: soloSplit); + token(name); + } + /// Visits a top-level function or method declaration. void _visitFunctionOrMethodDeclaration({ required NodeList metadata, @@ -4379,3 +4402,16 @@ class SourceVisitor extends ThrowingAstVisitor { int _startColumn(Token token) => _lineInfo.getLocation(token.offset).columnNumber; } + +/// Synthetic node for any kind of binary operator. +/// +/// Used to share formatting logic between binary operators, logic operators, +/// logic patterns, etc. +// TODO: Remove this and just use a record when those are available. +class BinaryNode { + final AstNode left; + final Token operator; + final AstNode right; + + BinaryNode(this.left, this.operator, this.right); +} diff --git a/pubspec.yaml b/pubspec.yaml index c12e4348..93ac48ea 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -9,7 +9,7 @@ environment: sdk: ">=2.18.0 <3.0.0" dependencies: - analyzer: ^5.4.0 + analyzer: ^5.5.0 args: ">=1.0.0 <3.0.0" path: ^1.0.0 pub_semver: ">=1.4.4 <3.0.0" diff --git a/test/whitespace/switch.stmt b/test/whitespace/switch.stmt index b2759516..baf4698e 100644 --- a/test/whitespace/switch.stmt +++ b/test/whitespace/switch.stmt @@ -230,4 +230,64 @@ var x = switch ( obj ) { var x = switch (obj) { 1 => 'one', var two => 'two' -}; \ No newline at end of file +}; +>>> handle cases that are not valid patterns +switch (obj) { + case {1, 2}: + case -pi: + case !true: + case ~1: + case 1 != 2: + case 1 == 2: + case 1 & 2: + case 1 | 2: + case 1 ^ 2: + case 1 ~/ 2: + case 1 >> 2: + case 1 >>> 2: + case 1 << 2: + case 1 + 2: + case 1 - 2: + case 1 * 2: + case 1 / 2: + case 1 % 2: + case 1 < 2: + case 1 <= 2: + case 1 > 2: + case 1 >= 2: + case 1 ?? 2: + case true ? 1 : 2: + case 's'.length: + case 1 is int: + case 1 is! int: +} +<<< +switch (obj) { + case {1, 2}: + case -pi: + case !true: + case ~1: + case 1 != 2: + case 1 == 2: + case 1 & 2: + case 1 | 2: + case 1 ^ 2: + case 1 ~/ 2: + case 1 >> 2: + case 1 >>> 2: + case 1 << 2: + case 1 + 2: + case 1 - 2: + case 1 * 2: + case 1 / 2: + case 1 % 2: + case 1 < 2: + case 1 <= 2: + case 1 > 2: + case 1 >= 2: + case 1 ?? 2: + case true ? 1 : 2: + case 's'.length: + case 1 is int: + case 1 is! int: +} \ No newline at end of file