Skip to content

Commit 9d63166

Browse files
kimdvkimdv
authored
Merge pull request #1681 from kimdv/kimdv/cherry-pick-fix-newline-for-multiline-string-literal
[5.9] Fix wrong formatting of multiline string literal
2 parents 77e4aa4 + 5facc4d commit 9d63166

13 files changed

+377
-88
lines changed

‎Sources/SwiftBasicFormat/BasicFormat.swift

Lines changed: 68 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -143,35 +143,50 @@ open class BasicFormat: SyntaxRewriter {
143143
return node.requiresIndent
144144
}
145145

146-
/// Whether a leading newline on `token` should be added.
147-
open func requiresLeadingNewline(_ token: TokenSyntax) -> Bool {
148-
// We don't want to add newlines inside string interpolation
149-
if isInsideStringInterpolation(token) {
146+
open func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {
147+
// We don't want to add newlines inside string interpolation.
148+
// When first or second `TokenSyntax` is a multiline quote we want special handling
149+
// even if it's inside a string interpolation, because it still requires newline
150+
// after open quote and before close quote.
151+
if let first,
152+
isInsideStringInterpolation(first),
153+
first.tokenKind != .multilineStringQuote,
154+
second?.tokenKind != .multilineStringQuote
155+
{
150156
return false
151-
}
152-
153-
if token.requiresLeadingNewline {
154-
return true
155-
}
156-
157-
var ancestor: Syntax = Syntax(token)
158-
while let parent = ancestor.parent {
159-
ancestor = parent
160-
if ancestor.position != token.position || ancestor.firstToken(viewMode: viewMode) != token {
161-
break
162-
}
163-
if let ancestorsParent = ancestor.parent, childrenSeparatedByNewline(ancestorsParent) {
157+
} else if let second {
158+
if second.requiresLeadingNewline {
164159
return true
165160
}
166-
switch ancestor.keyPathInParent {
167-
case \IfConfigClauseSyntax.elements:
168-
return true
169-
default:
170-
break
161+
162+
var ancestor: Syntax = Syntax(second)
163+
while let parent = ancestor.parent {
164+
ancestor = parent
165+
if ancestor.position != second.position || ancestor.firstToken(viewMode: viewMode) != second {
166+
break
167+
}
168+
if let ancestorsParent = ancestor.parent, childrenSeparatedByNewline(ancestorsParent) {
169+
return true
170+
}
171+
switch ancestor.keyPathInParent {
172+
case \IfConfigClauseSyntax.elements:
173+
return true
174+
default:
175+
break
176+
}
171177
}
172178
}
173179

174-
return false
180+
switch (first?.tokenKind, second?.tokenKind) {
181+
case (.multilineStringQuote, .backslash), // string interpolation segment inside a multi-line string literal
182+
(.multilineStringQuote, .multilineStringQuote), // empty multi-line string literal
183+
(.multilineStringQuote, .stringSegment), // segment starting a multi-line string literal
184+
(.stringSegment, .multilineStringQuote), // ending a multi-line string literal that has a string interpolation segment at its end
185+
(.rightParen, .multilineStringQuote): // ending a multi-line string literal that has a string interpolation segment at its end
186+
return true
187+
default:
188+
return false
189+
}
175190
}
176191

177192
open func requiresWhitespace(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {
@@ -276,6 +291,12 @@ open class BasicFormat: SyntaxRewriter {
276291
let previousToken = self.previousToken ?? token.previousToken(viewMode: viewMode)
277292
let nextToken = token.nextToken(viewMode: viewMode)
278293

294+
/// In addition to existing trivia of `previousToken`, also considers
295+
/// `previousToken` as ending with whitespace if it and `token` should be
296+
/// separated by whitespace.
297+
/// It does not consider whether a newline should be added between
298+
/// `previousToken` and the `token` because that newline should be added to
299+
/// the next token's trailing trivia.
279300
lazy var previousTokenWillEndWithWhitespace: Bool = {
280301
guard let previousToken = previousToken else {
281302
return false
@@ -284,6 +305,8 @@ open class BasicFormat: SyntaxRewriter {
284305
|| (requiresWhitespace(between: previousToken, and: token) && isMutable(previousToken))
285306
}()
286307

308+
/// This method does not consider any posssible mutations to `previousToken`
309+
/// because newlines should be added to the next token's leading trivia.
287310
lazy var previousTokenWillEndWithNewline: Bool = {
288311
guard let previousToken = previousToken else {
289312
// Assume that the start of the tree is equivalent to a newline so we
@@ -293,10 +316,7 @@ open class BasicFormat: SyntaxRewriter {
293316
if previousToken.trailingTrivia.endsWithNewline {
294317
return true
295318
}
296-
if case .stringSegment(let segment) = previousToken.tokenKind, segment.last?.isNewline ?? false {
297-
return true
298-
}
299-
return false
319+
return previousToken.isStringSegmentWithLastCharacterBeingNewline
300320
}()
301321

302322
lazy var previousTokenIsStringLiteralEndingInNewline: Bool = {
@@ -305,26 +325,29 @@ open class BasicFormat: SyntaxRewriter {
305325
// don't add a leading newline to the file.
306326
return true
307327
}
308-
if case .stringSegment(let segment) = previousToken.tokenKind, segment.last?.isNewline ?? false {
309-
return true
310-
}
311-
return false
328+
return previousToken.isStringSegmentWithLastCharacterBeingNewline
312329
}()
313330

331+
/// Also considers `nextToken` as starting with a whitespace if a newline
332+
/// should be added to it. It does not check whether `token` and `nextToken`
333+
/// should be separated by whitespace because the whitespace should be added
334+
/// to the `token`’s leading trivia.
314335
lazy var nextTokenWillStartWithWhitespace: Bool = {
315336
guard let nextToken = nextToken else {
316337
return false
317338
}
318339
return nextToken.leadingTrivia.startsWithWhitespace
319-
|| (requiresLeadingNewline(nextToken) && isMutable(nextToken))
340+
|| (requiresNewline(between: token, and: nextToken) && isMutable(nextToken))
320341
}()
321342

343+
/// Also considers `nextToken` as starting with a leading newline if `token`
344+
/// and `nextToken` should be separated by a newline.
322345
lazy var nextTokenWillStartWithNewline: Bool = {
323346
guard let nextToken = nextToken else {
324347
return false
325348
}
326349
return nextToken.leadingTrivia.startsWithNewline
327-
|| (requiresLeadingNewline(nextToken) && isMutable(nextToken))
350+
|| (requiresNewline(between: token, and: nextToken) && isMutable(nextToken) && !token.trailingTrivia.endsWithNewline && !token.isStringSegmentWithLastCharacterBeingNewline)
328351
}()
329352

330353
/// This token's trailing trivia + any spaces or tabs at the start of the
@@ -337,7 +360,7 @@ open class BasicFormat: SyntaxRewriter {
337360
var leadingTrivia = token.leadingTrivia
338361
var trailingTrivia = token.trailingTrivia
339362

340-
if requiresLeadingNewline(token) {
363+
if requiresNewline(between: previousToken, and: token) {
341364
// Add a leading newline if the token requires it unless
342365
// - it already starts with a newline or
343366
// - the previous token ends with a newline
@@ -402,3 +425,14 @@ open class BasicFormat: SyntaxRewriter {
402425
return token.detach().with(\.leadingTrivia, leadingTrivia).with(\.trailingTrivia, trailingTrivia)
403426
}
404427
}
428+
429+
fileprivate extension TokenSyntax {
430+
var isStringSegmentWithLastCharacterBeingNewline: Bool {
431+
switch self.tokenKind {
432+
case .stringSegment(let segment):
433+
return segment.last?.isNewline ?? false
434+
default:
435+
return false
436+
}
437+
}
438+
}

‎Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,11 +167,12 @@ extension FixIt.MultiNodeChange {
167167
}
168168
}
169169

170-
if let previousToken = node.previousToken(viewMode: .all),
170+
if let previousToken = node.previousToken(viewMode: .fixedUp),
171171
previousToken.presence == .present,
172172
let firstToken = node.firstToken(viewMode: .all),
173173
previousToken.trailingTrivia.allSatisfy({ $0.isWhitespace }),
174-
!BasicFormat().requiresWhitespace(between: previousToken, and: firstToken)
174+
!BasicFormat().requiresWhitespace(between: previousToken, and: firstToken),
175+
!BasicFormat().requiresNewline(between: previousToken, and: firstToken)
175176
{
176177
// If the previous token and this node don't need to be separated, remove
177178
// the separation.

‎Sources/SwiftParserDiagnostics/MissingNodesError.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ fileprivate func findCommonAncestor(_ nodes: [Syntax]) -> Syntax? {
2222
}
2323

2424
class NoNewlinesFormat: BasicFormat {
25-
override func requiresLeadingNewline(_ token: TokenSyntax) -> Bool {
25+
override func requiresNewline(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool {
2626
return false
2727
}
2828
}

‎Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,12 +1066,10 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
10661066
}
10671067
if case .stringSegment(let segment) = node.segments.last {
10681068
if let invalidContent = segment.unexpectedBeforeContent?.onlyToken(where: { $0.trailingTrivia.contains(where: { $0.isBackslash }) }) {
1069-
let leadingTrivia = segment.content.leadingTrivia
1070-
let trailingTrivia = segment.content.trailingTrivia
10711069
let fixIt = FixIt(
10721070
message: .removeBackslash,
10731071
changes: [
1074-
.makePresent(segment.content, leadingTrivia: leadingTrivia, trailingTrivia: trailingTrivia),
1072+
.makePresent(segment.content),
10751073
.makeMissing(invalidContent, transferTrivia: false),
10761074
]
10771075
)

‎Tests/SwiftParserTest/ExpressionTests.swift

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -550,16 +550,28 @@ final class ExpressionTests: XCTestCase {
550550
NoteSpec(message: #"to match this opening '"""'"#)
551551
]
552552
)
553-
]
553+
],
554+
fixedSource: ##"""
555+
""""
556+
"""
557+
"""##
554558
)
555559

556560
assertParse(
557561
##"""
558-
"""""1️⃣
562+
ℹ️"""""1️⃣
559563
"""##,
560564
diagnostics: [
561-
DiagnosticSpec(message: #"expected '"""' to end string literal"#)
562-
]
565+
DiagnosticSpec(
566+
message: #"expected '"""' to end string literal"#,
567+
notes: [NoteSpec(message: #"to match this opening '"""'"#)],
568+
fixIts: [#"insert '"""'"#]
569+
)
570+
],
571+
fixedSource: ##"""
572+
"""""
573+
"""
574+
"""##
563575
)
564576

565577
assertParse(
@@ -589,17 +601,25 @@ final class ExpressionTests: XCTestCase {
589601
#"""1️⃣
590602
"""##,
591603
diagnostics: [
592-
DiagnosticSpec(message: ##"expected '"""#' to end string literal"##)
593-
]
604+
DiagnosticSpec(message: ##"expected '"""#' to end string literal"##, fixIts: [##"insert '"""#'"##])
605+
],
606+
fixedSource: ##"""
607+
#"""
608+
"""#
609+
"""##
594610
)
595611

596612
assertParse(
597613
##"""
598614
#"""a1️⃣
599615
"""##,
600616
diagnostics: [
601-
DiagnosticSpec(message: ##"expected '"""#' to end string literal"##)
602-
]
617+
DiagnosticSpec(message: ##"expected '"""#' to end string literal"##, fixIts: [##"insert '"""#'"##])
618+
],
619+
fixedSource: ##"""
620+
#"""a
621+
"""#
622+
"""##
603623
)
604624

605625
assertParse(

‎Tests/SwiftParserTest/translated/AvailabilityQueryTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ final class AvailabilityQueryTests: XCTestCase {
9696
DiagnosticSpec(message: "expected ',' joining parts of a multi-clause condition", fixIts: ["replace '&&' with ','"])
9797
],
9898
fixedSource: """
99-
if #available(OSX 10.51, *) , #available(OSX 10.52, *) {
99+
if #available(OSX 10.51, *), #available(OSX 10.52, *) {
100100
}
101101
"""
102102
)
@@ -385,7 +385,7 @@ final class AvailabilityQueryTests: XCTestCase {
385385
DiagnosticSpec(message: "expected ',' joining platforms in availability condition", fixIts: ["replace '||' with ','"])
386386
],
387387
fixedSource: """
388-
if #available(OSX 10.51 , iOS 8.0) {
388+
if #available(OSX 10.51, iOS 8.0) {
389389
}
390390
"""
391391
)

‎Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,15 @@ final class AvailabilityQueryUnavailabilityTests: XCTestCase {
7878
}
7979
""",
8080
diagnostics: [
81-
DiagnosticSpec(message: "expected ',' joining parts of a multi-clause condition", fixIts: ["replace '&&' with ','"])
82-
]
81+
DiagnosticSpec(
82+
message: "expected ',' joining parts of a multi-clause condition",
83+
fixIts: ["replace '&&' with ','"]
84+
)
85+
],
86+
fixedSource: """
87+
if #unavailable(OSX 10.51), #unavailable(OSX 10.52) {
88+
}
89+
"""
8390
)
8491
}
8592

@@ -367,8 +374,15 @@ final class AvailabilityQueryUnavailabilityTests: XCTestCase {
367374
}
368375
""",
369376
diagnostics: [
370-
DiagnosticSpec(message: "expected ',' joining platforms in availability condition")
371-
]
377+
DiagnosticSpec(
378+
message: "expected ',' joining platforms in availability condition",
379+
fixIts: ["replace '||' with ','"]
380+
)
381+
],
382+
fixedSource: """
383+
if #unavailable(OSX 10.51, iOS 8.0) {
384+
}
385+
"""
372386
)
373387
}
374388

‎Tests/SwiftParserTest/translated/MultilineErrorsTests.swift

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -582,13 +582,26 @@ final class MultilineErrorsTests: XCTestCase {
582582
func testMultilineErrors26() {
583583
assertParseWithAllNewlineEndings(
584584
##"""
585-
_ = """
585+
_ = ℹ️"""
586586
foo1️⃣\2️⃣
587587
"""##,
588588
diagnostics: [
589-
DiagnosticSpec(locationMarker: "1️⃣", message: "invalid escape sequence in literal"),
590-
DiagnosticSpec(locationMarker: "2️⃣", message: #"expected '"""' to end string literal"#),
591-
]
589+
DiagnosticSpec(
590+
locationMarker: "1️⃣",
591+
message: "invalid escape sequence in literal"
592+
),
593+
DiagnosticSpec(
594+
locationMarker: "2️⃣",
595+
message: #"expected '"""' to end string literal"#,
596+
notes: [NoteSpec(message: #"to match this opening '"""'"#)],
597+
fixIts: [#"insert '"""'"#]
598+
),
599+
],
600+
fixedSource: ##"""
601+
_ = """
602+
foo\
603+
"""
604+
"""##
592605
)
593606
}
594607

@@ -623,6 +636,7 @@ final class MultilineErrorsTests: XCTestCase {
623636
fixedSource: #"""
624637
_ = """
625638
\#u{20}
639+
626640
"""
627641
"""#
628642
)

‎Tests/SwiftParserTest/translated/MultilinePoundDiagnosticArgRdar41154797Tests.swift

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,26 @@ final class MultilinePoundDiagnosticArgRdar41154797Tests: XCTestCase {
1818
func testMultilinePoundDiagnosticArgRdar411547971() {
1919
assertParse(
2020
##"""
21-
#error("""1️⃣
21+
#error1️⃣(2️⃣"""3️⃣
2222
"""##,
2323
diagnostics: [
24-
DiagnosticSpec(message: #"expected '"""' to end string literal"#),
25-
DiagnosticSpec(message: "expected ')' to end macro expansion"),
26-
]
24+
DiagnosticSpec(
25+
locationMarker: "3️⃣",
26+
message: #"expected '"""' to end string literal"#,
27+
notes: [NoteSpec(locationMarker: "2️⃣", message: #"to match this opening '"""'"#)],
28+
fixIts: [#"insert '"""'"#]
29+
),
30+
DiagnosticSpec(
31+
locationMarker: "3️⃣",
32+
message: "expected ')' to end macro expansion",
33+
notes: [NoteSpec(locationMarker: "1️⃣", message: "to match this opening '('")],
34+
fixIts: ["insert ')'"]
35+
),
36+
],
37+
fixedSource: ##"""
38+
#error("""
39+
""")
40+
"""##
2741
)
2842
}
2943

0 commit comments

Comments
 (0)