Skip to content

Commit a059ab2

Browse files
committed
Fix a bug where an unfolded SequenceExpr would make it to the pretty-printer.
Since the `UseExplicitNilCheckInConditions` rule was using string interpolation based parsing, it returned a `SequenceExpr` instead of an `InfixOperatorExpr`. This unfolded expression then made it to the pretty printer, which it didn't expect (because the input is folded before being processed by the rules, but not after that). In this case, creating the new expression directly as nodes is barely more work than using the string-based parsing and then folding it, so I just did that. Tests didn't catch this because the sequence expr is valid Swift code when stringified (for comparison in unit tests). To address this, we now do a pretty-printer pass on all the format rule outputs. We don't assert based on the pretty-printed output (that's not really a "unit" test anymore), but it at least causes a test crash if the output wasn't suitable for pretty printing. Doing so caught a similar latent bug in `UseShorthandTypeNames`.
1 parent 01966bd commit a059ab2

File tree

4 files changed

+32
-10
lines changed

4 files changed

+32
-10
lines changed

Sources/SwiftFormat/PrettyPrint/TokenStreamCreator.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2079,7 +2079,11 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
20792079
}
20802080

20812081
override func visit(_ node: SequenceExprSyntax) -> SyntaxVisitorContinueKind {
2082-
preconditionFailure("SequenceExpr should have already been folded.")
2082+
preconditionFailure(
2083+
"""
2084+
SequenceExpr should have already been folded; found at byte offsets \
2085+
\(node.position.utf8Offset)..<\(node.endPosition.utf8Offset)
2086+
""")
20832087
}
20842088

20852089
override func visit(_ node: AssignmentExprSyntax) -> SyntaxVisitorContinueKind {

Sources/SwiftFormat/Rules/UseExplicitNilCheckInConditions.swift

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,18 @@ public final class UseExplicitNilCheckInConditions: SyntaxFormatRule {
4141
// trivia of the original node, since that token is being removed entirely.
4242
var value = initializerClause.value
4343
let trailingTrivia = value.trailingTrivia
44-
value.trailingTrivia = []
44+
value.trailingTrivia = [.spaces(1)]
4545

46-
return ConditionElementSyntax(
47-
condition: .expression("\(node.leadingTrivia)\(value) != nil\(trailingTrivia)"))
46+
var operatorExpr = BinaryOperatorExprSyntax(text: "!=")
47+
operatorExpr.trailingTrivia = [.spaces(1)]
48+
49+
var inequalExpr = InfixOperatorExprSyntax(
50+
leftOperand: value,
51+
operator: operatorExpr,
52+
rightOperand: NilLiteralExprSyntax())
53+
inequalExpr.leadingTrivia = node.leadingTrivia
54+
inequalExpr.trailingTrivia = trailingTrivia
55+
return ConditionElementSyntax(condition: .expression(ExprSyntax(inequalExpr)))
4856
default:
4957
return node
5058
}

Sources/SwiftFormat/Rules/UseShorthandTypeNames.swift

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -478,7 +478,7 @@ public final class UseShorthandTypeNames: SyntaxFormatRule {
478478
effectSpecifiers: TypeEffectSpecifiersSyntax?,
479479
arrow: TokenSyntax,
480480
returnType: TypeSyntax
481-
) -> SequenceExprSyntax? {
481+
) -> InfixOperatorExprSyntax? {
482482
guard
483483
let parameterExprs = expressionRepresentation(of: parameters),
484484
let returnTypeExpr = expressionRepresentation(of: returnType)
@@ -493,11 +493,10 @@ public final class UseShorthandTypeNames: SyntaxFormatRule {
493493
let arrowExpr = ArrowExprSyntax(
494494
effectSpecifiers: effectSpecifiers,
495495
arrow: arrow)
496-
497-
return SequenceExprSyntax(
498-
elements: ExprListSyntax([
499-
ExprSyntax(tupleExpr), ExprSyntax(arrowExpr), returnTypeExpr,
500-
]))
496+
return InfixOperatorExprSyntax(
497+
leftOperand: tupleExpr,
498+
operator: arrowExpr,
499+
rightOperand: returnTypeExpr)
501500
}
502501

503502
/// Returns the leading and trailing trivia from the front and end of the entire given node.

Tests/SwiftFormatTests/Rules/LintOrFormatRuleTestCase.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,5 +100,16 @@ class LintOrFormatRuleTestCase: DiagnosingTestCase {
100100
context: context,
101101
file: file,
102102
line: line)
103+
104+
// Verify that the pretty printer can consume the transformed tree (e.g., it does not contain
105+
// any unfolded `SequenceExpr`s). We don't need to check the actual output here (we don't want
106+
// the rule tests to be pretty-printer dependent), but this will catch invariants that aren't
107+
// satisfied.
108+
_ = PrettyPrinter(
109+
context: context,
110+
node: Syntax(actual),
111+
printTokenStream: false,
112+
whitespaceOnly: false
113+
).prettyPrint()
103114
}
104115
}

0 commit comments

Comments
 (0)