Skip to content

Commit 9c21818

Browse files
authored
Merge pull request swiftlang#105 from allevato/hugs
Improve wrapping of parenthesized expressions.
2 parents 4837fec + 12a4763 commit 9c21818

14 files changed

+416
-128
lines changed

Sources/SwiftFormatPrettyPrint/PrettyPrint.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,6 @@ public class PrettyPrinter {
292292
// scope. Additionally, continuation open breaks must indent when the break fires.
293293
let continuationBreakWillFire = openKind == .continuation
294294
&& (isAtStartOfLine || length > spaceRemaining || mustBreak)
295-
&& !isBreakingSupressed
296295
let contributesContinuationIndent = currentLineIsContinuation || continuationBreakWillFire
297296

298297
activeOpenBreaks.append(

Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift

Lines changed: 112 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -601,13 +601,26 @@ private final class TokenStreamCreator: SyntaxVisitor {
601601
}
602602

603603
func visit(_ node: TupleExprSyntax) -> SyntaxVisitorContinueKind {
604-
after(node.leftParen, tokens: .break(.open, size: 0), .open)
605-
before(node.rightParen, tokens: .close, .break(.close, size: 0))
606-
607-
insertTokens(.break(.same), betweenElementsOf: node.elementList)
608-
609-
for element in node.elementList {
610-
arrangeAsTupleExprElement(element)
604+
// We'll do nothing if it's a zero-element tuple, because we just want to keep the empty `()`
605+
// together.
606+
let elementCount = node.elementList.count
607+
608+
if elementCount == 1 {
609+
// A tuple with one element is a parenthesized expression; add a group around it to keep it
610+
// together when possible, but breaks are handled elsewhere (see calls to
611+
// `stackedIndentationBehavior`).
612+
after(node.leftParen, tokens: .open)
613+
before(node.rightParen, tokens: .close)
614+
} else if elementCount > 1 {
615+
// Tuples with more than one element are "true" tuples, and should indent as block structures.
616+
after(node.leftParen, tokens: .break(.open, size: 0), .open)
617+
before(node.rightParen, tokens: .close, .break(.close, size: 0))
618+
619+
insertTokens(.break(.same), betweenElementsOf: node.elementList)
620+
621+
for element in node.elementList {
622+
arrangeAsTupleExprElement(element)
623+
}
611624
}
612625

613626
return .visitChildren
@@ -1296,35 +1309,47 @@ private final class TokenStreamCreator: SyntaxVisitor {
12961309

12971310
if shouldRequireWhitespace(around: binOp) {
12981311
if isAssigningOperator(binOp) {
1299-
var beforeTokens: [Token] = [.break(.open(kind: .continuation))]
1300-
var afterTokens: [Token] = [.break(.close(mustBreak: false), size: 0)]
1312+
var beforeTokens: [Token]
1313+
1314+
// If the rhs starts with a parenthesized expression, stack indentation around it.
1315+
// Otherwise, use regular continuation breaks.
1316+
if let (unindentingNode, _) = stackedIndentationBehavior(after: binOp, rhs: rhs) {
1317+
beforeTokens = [.break(.open(kind: .continuation))]
1318+
after(unindentingNode.lastToken, tokens: [.break(.close(mustBreak: false), size: 0)])
1319+
} else {
1320+
beforeTokens = [.break(.continue)]
1321+
}
13011322

13021323
// When the RHS is a simple expression, even if is requires multiple lines, we don't add a
13031324
// group so that as much of the expression as possible can stay on the same line as the
13041325
// operator token.
13051326
if isCompoundExpression(rhs) {
13061327
beforeTokens.append(.open)
1307-
afterTokens.append(.close)
1328+
after(rhs.lastToken, tokens: .close)
13081329
}
1330+
13091331
after(binOp.lastToken, tokens: beforeTokens)
1310-
after(rhs.lastToken, tokens: afterTokens)
1311-
} else if shouldStackIndentation(after: binOp) {
1312-
// For certain operators like `&&` and `||`, we don't want to treat all continue breaks
1313-
// the same. If we did, then all operators would line up at the same alignment regardless
1314-
// of whether they were, for example, `&&` or something between a pair of `&&`. To make
1315-
// long conditionals format more cleanly, we use open-continuation/close pairs around such
1316-
// operators and their right-hand sides so that the continuation breaks inside those
1317-
// scopes "stack", instead of receiving the usual single-level "continuation line or not"
1318-
// behavior.
1332+
} else if let (unindentingNode, shouldReset) =
1333+
stackedIndentationBehavior(after: binOp, rhs: rhs)
1334+
{
1335+
// For parenthesized expressions and for unparenthesized usages of `&&` and `||`, we don't
1336+
// want to treat all continue breaks the same. If we did, then all operators would line up
1337+
// at the same alignment regardless of whether they were, for example, `&&` or something
1338+
// between a pair of `&&`. To make long expressions/conditionals format more cleanly, we
1339+
// use open-continuation/close pairs around such operators and their right-hand sides so
1340+
// that the continuation breaks inside those scopes "stack", instead of receiving the
1341+
// usual single-level "continuation line or not" behavior.
13191342
let openBreakTokens: [Token] = [.break(.open(kind: .continuation)), .open]
13201343
if wrapsBeforeOperator {
13211344
before(binOp.firstToken, tokens: openBreakTokens)
13221345
} else {
13231346
after(binOp.lastToken, tokens: openBreakTokens)
13241347
}
1325-
after(
1326-
rhs.lastToken,
1327-
tokens: [.break(.reset, size: 0), .break(.close(mustBreak: false), size: 0), .close])
1348+
1349+
let closeBreakTokens: [Token] =
1350+
(shouldReset ? [.break(.reset, size: 0)] : [])
1351+
+ [.break(.close(mustBreak: false), size: 0), .close]
1352+
after(unindentingNode.lastToken, tokens: closeBreakTokens)
13281353
} else {
13291354
if wrapsBeforeOperator {
13301355
before(binOp.firstToken, tokens: .break(.continue))
@@ -1422,14 +1447,19 @@ private final class TokenStreamCreator: SyntaxVisitor {
14221447
closeAfterToken = typeAnnotation.lastToken
14231448
}
14241449
if let initializer = node.initializer {
1425-
after(initializer.equal, tokens: .break(.open(kind: .continuation)))
1426-
closesNeeded += 1
1450+
let expr = initializer.value
1451+
1452+
if let (unindentingNode, _) = stackedIndentationBehavior(rhs: expr) {
1453+
after(initializer.equal, tokens: .break(.open(kind: .continuation)))
1454+
after(unindentingNode.lastToken, tokens: .break(.close(mustBreak: false), size: 0))
1455+
} else {
1456+
after(initializer.equal, tokens: .break(.continue))
1457+
}
14271458
closeAfterToken = initializer.lastToken
14281459

14291460
// When the RHS is a simple expression, even if is requires multiple lines, we don't add a
14301461
// group so that as much of the expression as possible can stay on the same line as the
14311462
// operator token.
1432-
let expr = initializer.value
14331463
if isCompoundExpression(expr) {
14341464
before(expr.firstToken, tokens: .open)
14351465
after(expr.lastToken, tokens: .close)
@@ -2474,13 +2504,16 @@ private final class TokenStreamCreator: SyntaxVisitor {
24742504
/// that are known to wrap an expressions, e.g. try expressions, are handled by checking the
24752505
/// expression that they contain.
24762506
private func isCompoundExpression(_ expr: ExprSyntax) -> Bool {
2477-
if let sequenceExpr = expr as? SequenceExprSyntax {
2507+
switch expr {
2508+
case let sequenceExpr as SequenceExprSyntax:
24782509
return sequenceExpr.elements.count > 1
2479-
}
2480-
if let tryExpr = expr as? TryExprSyntax {
2510+
case let tryExpr as TryExprSyntax:
24812511
return isCompoundExpression(tryExpr.expression)
2512+
case let tupleExpr as TupleExprSyntax where tupleExpr.elementList.count == 1:
2513+
return isCompoundExpression(tupleExpr.elementList.first!.expression)
2514+
default:
2515+
return false
24822516
}
2483-
return false
24842517
}
24852518

24862519
/// Returns whether the given operator behaves as an assignment, to assign a right-hand-side to a
@@ -2503,24 +2536,65 @@ private final class TokenStreamCreator: SyntaxVisitor {
25032536
return false
25042537
}
25052538

2506-
/// Returns a value indicating whether indentation should be stacked within subexpressions to the
2507-
/// right of the given operator.
2539+
/// Walks the expression and returns the leftmost subexpression if it is parenthesized (which
2540+
/// might be the expression itself).
2541+
///
2542+
/// - Parameter expr: The expression whose parenthesized leftmost subexpression should be
2543+
/// returned.
2544+
/// - Returns: The parenthesized leftmost subexpression, or nil if the leftmost subexpression was
2545+
/// not parenthesized.
2546+
private func parenthesizedLeftmostExpr(of expr: ExprSyntax) -> TupleExprSyntax? {
2547+
switch expr {
2548+
case let tupleExpr as TupleExprSyntax where tupleExpr.elementList.count == 1:
2549+
return tupleExpr
2550+
case let sequenceExpr as SequenceExprSyntax:
2551+
return parenthesizedLeftmostExpr(of: sequenceExpr.elements.first!)
2552+
case let ternaryExpr as TernaryExprSyntax:
2553+
return parenthesizedLeftmostExpr(of: ternaryExpr.conditionExpression)
2554+
default:
2555+
return nil
2556+
}
2557+
}
2558+
2559+
/// Determines if indentation should be stacked around a subexpression to the right of the given
2560+
/// operator, and, if so, returns the node after which indentation stacking should be closed and
2561+
/// whether or not the continuation state should be reset as well.
25082562
///
2509-
/// Operators that are good candidates for this behavior are ones that have low precedence and
2510-
/// may occur in chains, such as logical AND (`&&`) and OR (`||`) in conditional statements, where
2511-
/// the extra level of indentation helps to improve readability with the operators inside those
2512-
/// conditions.
2513-
private func shouldStackIndentation(after operatorExpr: ExprSyntax) -> Bool {
2563+
/// Stacking is applied around parenthesized expressions, but also for low-precedence operators
2564+
/// that frequently occur in long chains, such as logical AND (`&&`) and OR (`||`) in conditional
2565+
/// statements. In this case, the extra level of indentation helps to improve readability with the
2566+
/// operators inside those conditions even when parentheses are not used.
2567+
private func stackedIndentationBehavior(
2568+
after operatorExpr: ExprSyntax? = nil,
2569+
rhs: ExprSyntax
2570+
) -> (unindentingNode: ExprSyntax, shouldReset: Bool)? {
2571+
// Check for logical operators first, and if it's that kind of operator, stack indentation
2572+
// around the entire right-hand-side. We have to do this check before checking the RHS for
2573+
// parentheses because if the user writes something like `... && (foo) > bar || ...`, we don't
2574+
// want the indentation stacking that starts before the `&&` to stop after the closing
2575+
// parenthesis in `(foo)`.
2576+
//
2577+
// We also want to reset after undoing the stacked indentation so that we have a visual
2578+
// indication that the subexpression has ended.
25142579
if let binaryOperator = operatorExpr as? BinaryOperatorExprSyntax {
25152580
let operatorText = binaryOperator.operatorToken.text
25162581
if let precedence = operatorContext.infixOperator(named: operatorText)?.precedenceGroup,
25172582
precedence === operatorContext.precedenceGroup(named: .logicalConjunction)
25182583
|| precedence === operatorContext.precedenceGroup(named: .logicalDisjunction)
25192584
{
2520-
return true
2585+
return (unindentingNode: rhs, shouldReset: true)
25212586
}
25222587
}
2523-
return false
2588+
2589+
// If the right-hand-side of the operator is or starts with a parenthesized expression, stack
2590+
// indentation around the operator and those parentheses. We don't need to reset here because
2591+
// the parentheses are sufficient to provide a visual indication of the nesting relationship.
2592+
if let parenthesizedExpr = parenthesizedLeftmostExpr(of: rhs) {
2593+
return (unindentingNode: parenthesizedExpr, shouldReset: false)
2594+
}
2595+
2596+
// Otherwise, don't stack--use regular continuation breaks instead.
2597+
return nil
25242598
}
25252599

25262600
/// Returns a value indicating whether whitespace should be required around the given operator.

Tests/SwiftFormatPrettyPrintTests/AssignmentExprTests.swift

Lines changed: 47 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,13 @@ public class AssignmentExprTests: PrettyPrintTestCase {
3232
"""
3333
someVeryLongVariableName =
3434
anotherPrettyLongVariableName
35-
&& someOtherOperand
35+
&& someOtherOperand
3636
shortName =
3737
wxyz + aaaaaa + bbbbbb
38-
+ cccccc
38+
+ cccccc
3939
longerName =
4040
wxyz + aaaaaa + bbbbbb
41-
+ cccccc || zzzzzzz
41+
+ cccccc || zzzzzzz
4242
4343
"""
4444

@@ -60,27 +60,27 @@ public class AssignmentExprTests: PrettyPrintTestCase {
6060
"""
6161
result =
6262
firstOp + secondOp
63-
+ someOpFetchingFunc(
64-
foo, bar: bar, baz: baz)
63+
+ someOpFetchingFunc(
64+
foo, bar: bar, baz: baz)
6565
result = someOpFetchingFunc(
6666
foo, bar: bar, baz: baz)
6767
result += someOpFetchingFunc(
6868
foo, bar: bar, baz: baz)
6969
result =
7070
someOpFetchingFunc(
7171
foo, bar: bar, baz: baz)
72-
+ someOtherOperand
73-
+ andAThirdOneForReasons
72+
+ someOtherOperand
73+
+ andAThirdOneForReasons
7474
result =
7575
firstOp + secondOp + thirdOp
76-
+ someOpFetchingFunc(
77-
foo, bar, baz) + nextOp
78-
+ lastOp
76+
+ someOpFetchingFunc(
77+
foo, bar, baz) + nextOp
78+
+ lastOp
7979
result +=
8080
firstOp + secondOp + thirdOp
81-
+ someOpFetchingFunc(
82-
foo, bar, baz) + nextOp
83-
+ lastOp
81+
+ someOpFetchingFunc(
82+
foo, bar, baz) + nextOp
83+
+ lastOp
8484
8585
"""
8686

@@ -93,11 +93,11 @@ public class AssignmentExprTests: PrettyPrintTestCase {
9393
"""
9494
result =
9595
firstOp + secondOp
96-
+ someOpFetchingFunc(
97-
foo,
98-
bar: bar,
99-
baz: baz
100-
)
96+
+ someOpFetchingFunc(
97+
foo,
98+
bar: bar,
99+
baz: baz
100+
)
101101
result = someOpFetchingFunc(
102102
foo,
103103
bar: bar,
@@ -114,21 +114,21 @@ public class AssignmentExprTests: PrettyPrintTestCase {
114114
bar: bar,
115115
baz: baz
116116
) + someOtherOperand
117-
+ andAThirdOneForReasons
117+
+ andAThirdOneForReasons
118118
result =
119119
firstOp + secondOp + thirdOp
120-
+ someOpFetchingFunc(
121-
foo,
122-
bar,
123-
baz
124-
) + nextOp + lastOp
120+
+ someOpFetchingFunc(
121+
foo,
122+
bar,
123+
baz
124+
) + nextOp + lastOp
125125
result +=
126126
firstOp + secondOp + thirdOp
127-
+ someOpFetchingFunc(
128-
foo,
129-
bar,
130-
baz
131-
) + nextOp + lastOp
127+
+ someOpFetchingFunc(
128+
foo,
129+
bar,
130+
baz
131+
) + nextOp + lastOp
132132
133133
"""
134134
config.lineBreakBeforeEachArgument = true
@@ -149,20 +149,20 @@ public class AssignmentExprTests: PrettyPrintTestCase {
149149
"""
150150
let result =
151151
firstOp + secondOp
152-
+ someOpFetchingFunc(
153-
foo, bar: bar, baz: baz)
152+
+ someOpFetchingFunc(
153+
foo, bar: bar, baz: baz)
154154
let result = someOpFetchingFunc(
155155
foo, bar: bar, baz: baz)
156156
let result =
157157
someOpFetchingFunc(
158158
foo, bar: bar, baz: baz)
159-
+ someOtherOperand
160-
+ andAThirdOneForReasons
159+
+ someOtherOperand
160+
+ andAThirdOneForReasons
161161
let result =
162162
firstOp + secondOp + thirdOp
163-
+ someOpFetchingFunc(
164-
foo, bar, baz) + nextOp
165-
+ lastOp
163+
+ someOpFetchingFunc(
164+
foo, bar, baz) + nextOp
165+
+ lastOp
166166
167167
"""
168168

@@ -175,11 +175,11 @@ public class AssignmentExprTests: PrettyPrintTestCase {
175175
"""
176176
let result =
177177
firstOp + secondOp
178-
+ someOpFetchingFunc(
179-
foo,
180-
bar: bar,
181-
baz: baz
182-
)
178+
+ someOpFetchingFunc(
179+
foo,
180+
bar: bar,
181+
baz: baz
182+
)
183183
let result = someOpFetchingFunc(
184184
foo,
185185
bar: bar,
@@ -191,14 +191,14 @@ public class AssignmentExprTests: PrettyPrintTestCase {
191191
bar: bar,
192192
baz: baz
193193
) + someOtherOperand
194-
+ andAThirdOneForReasons
194+
+ andAThirdOneForReasons
195195
let result =
196196
firstOp + secondOp + thirdOp
197-
+ someOpFetchingFunc(
198-
foo,
199-
bar,
200-
baz
201-
) + nextOp + lastOp
197+
+ someOpFetchingFunc(
198+
foo,
199+
bar,
200+
baz
201+
) + nextOp + lastOp
202202
203203
"""
204204
config.lineBreakBeforeEachArgument = true

0 commit comments

Comments
 (0)