From cd125a0bb7932a0ffb1ad35c154647377bd6ced5 Mon Sep 17 00:00:00 2001 From: Kai Lau Date: Thu, 12 Sep 2024 17:06:58 -0700 Subject: [PATCH] Adopt the new merging API of `Trivia` and fix trivia transfer Fixed the erratic behaviors of `transferTriviaAtSides(from:)` by adopting the new trivia merging API. As a result, the issue of incorrect transfer of trivia after applying Fix-it has also been solved. Cleaned up the `FixIt.MultiNodeChange.makeMissing` methods. --- .../DiagnosticExtensions.swift | 73 +++++++++++-------- .../ParseDiagnosticsGenerator.swift | 4 +- .../CallToTrailingClosures.swift | 29 ++++---- Tests/SwiftParserTest/DeclarationTests.swift | 10 +-- .../translated/RecoveryTests.swift | 2 +- .../translated/SwitchTests.swift | 1 - 6 files changed, 65 insertions(+), 54 deletions(-) diff --git a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift index 42128ddd58d..f533496192c 100644 --- a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift +++ b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift @@ -52,13 +52,39 @@ extension FixIt { // MARK: - Make missing +private extension FixIt.Change { + /// Transfers the leading and trailing trivia of `nodes` to the previous token + /// While doing this, it tries to be smart, merging trivia where it makes sense + /// and refusing to add e.g. a space after punctuation, where it usually + /// doesn't make sense. + static func transferTriviaAtSides(from nodes: [Syntax]) -> Self? { + guard let first = nodes.first, let last = nodes.last else { + preconditionFailure() + } + let removedTriviaAtSides = first.leadingTrivia.mergingCommonSuffix(last.trailingTrivia) + guard !removedTriviaAtSides.isEmpty, let previousToken = first.previousToken(viewMode: .sourceAccurate) else { + return nil + } + let mergedTrivia = previousToken.trailingTrivia.mergingCommonPrefix(removedTriviaAtSides) + // We merge with the common prefix instead of the common suffix to preserve the original shape of the previous + // token's trailing trivia after merging. + guard !previousToken.tokenKind.isPunctuation || !mergedTrivia.allSatisfy(\.isSpaceOrTab) else { + // Punctuation is generally not followed by spaces in Swift. + // If this action would only add spaces to the punctuation, drop it. + // This generally yields better results. + return nil + } + return .replaceTrailingTrivia(token: previousToken, newTrivia: mergedTrivia) + } +} + extension FixIt.MultiNodeChange { - /// Replaced a present token with a missing node. + /// Replaced a present token with a missing token. /// /// If `transferTrivia` is `true`, the leading and trailing trivia of the - /// removed node will be transferred to the trailing trivia of the previous token. + /// removed token will be transferred to the trailing trivia of the previous token. static func makeMissing(_ token: TokenSyntax, transferTrivia: Bool = true) -> Self { - return makeMissing([token], transferTrivia: transferTrivia) + self.makeMissing([token], transferTrivia: transferTrivia) } /// Replace present tokens with missing tokens. @@ -68,40 +94,25 @@ extension FixIt.MultiNodeChange { /// tokens. static func makeMissing(_ tokens: [TokenSyntax], transferTrivia: Bool = true) -> Self { precondition(tokens.allSatisfy(\.isPresent)) - return .makeMissing(tokens.map(Syntax.init), transferTrivia: transferTrivia) + return self.makeMissing(tokens.map(Syntax.init), transferTrivia: transferTrivia) } /// If `transferTrivia` is `true`, the leading and trailing trivia of the /// removed node will be transferred to the trailing trivia of the previous token. static func makeMissing(_ node: (some SyntaxProtocol)?, transferTrivia: Bool = true) -> Self { - guard let node = node else { + guard let node else { return FixIt.MultiNodeChange(primitiveChanges: []) } - var changes = [FixIt.Change.replace(oldNode: Syntax(node), newNode: MissingMaker().rewrite(node, detach: true))] - if transferTrivia { - changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: [node]).primitiveChanges - } - return FixIt.MultiNodeChange(primitiveChanges: changes) + return self.makeMissing([node], transferTrivia: transferTrivia) } - /// Transfers the leading and trailing trivia of `nodes` to the previous token - /// While doing this, it tries to be smart, merging trivia where it makes sense - /// and refusing to add e.g. a space after punctuation, where it usually - /// doesn't make sense. - private static func transferTriviaAtSides(from nodes: [some SyntaxProtocol]) -> Self { - let removedTriviaAtSides = (nodes.first?.leadingTrivia ?? []).merging(nodes.last?.trailingTrivia ?? []) - if !removedTriviaAtSides.isEmpty, let previousToken = nodes.first?.previousToken(viewMode: .sourceAccurate) { - let mergedTrivia = previousToken.trailingTrivia.merging(removedTriviaAtSides) - if previousToken.tokenKind.isPunctuation, mergedTrivia.allSatisfy({ $0.isSpaceOrTab }) { - // Punctuation is generally not followed by spaces in Swift. - // If this action would only add spaces to the punctuation, drop it. - // This generally yields better results. - return FixIt.MultiNodeChange() - } - return FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previousToken, newTrivia: mergedTrivia)) - } else { - return FixIt.MultiNodeChange() - } + /// Replace present nodes with missing nodes. + /// + /// If `transferTrivia` is `true`, the leading trivia of the first node and + /// the trailing trivia of the last node will be transferred to their adjecent + /// tokens. + static func makeMissing(_ nodes: [some SyntaxProtocol], transferTrivia: Bool = true) -> Self { + self.makeMissing(nodes.map(Syntax.init), transferTrivia: transferTrivia) } /// Replace present nodes with their missing equivalents. @@ -109,7 +120,7 @@ extension FixIt.MultiNodeChange { /// If `transferTrivia` is `true`, the leading trivia of the first node and /// the trailing trivia of the last node will be transferred to their adjecent /// tokens. - static func makeMissing(_ nodes: [Syntax], transferTrivia: Bool = true) -> Self { + private static func makeMissing(_ nodes: [Syntax], transferTrivia: Bool = true) -> Self { precondition(!nodes.isEmpty) var changes = nodes.map { FixIt.Change.replace( @@ -117,8 +128,8 @@ extension FixIt.MultiNodeChange { newNode: MissingMaker().rewrite($0, detach: true) ) } - if transferTrivia { - changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: nodes).primitiveChanges + if transferTrivia, let transferredTrivia = FixIt.Change.transferTriviaAtSides(from: nodes) { + changes.append(transferredTrivia) } return FixIt.MultiNodeChange(primitiveChanges: changes) } diff --git a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift index 13030c5143e..16d795d5edf 100644 --- a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift +++ b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift @@ -213,7 +213,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { ) changes.append(FixIt.MultiNodeChange.makeMissing(misplacedTokens, transferTrivia: false)) } else { - changes += misplacedTokens.map { FixIt.MultiNodeChange.makeMissing($0) } + changes += [.makeMissing(misplacedTokens)] changes += correctAndMissingNodes.map { FixIt.MultiNodeChange.makePresent($0) } } var fixIts: [FixIt] = [] @@ -757,7 +757,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { node, .standaloneSemicolonStatement, fixIts: [ - FixIt(message: RemoveNodesFixIt(semicolon), changes: .makeMissing(semicolon)) + FixIt(message: RemoveNodesFixIt(semicolon), changes: .makeMissing(semicolon, transferTrivia: false)) ], handledNodes: [node.item.id] ) diff --git a/Sources/SwiftRefactor/CallToTrailingClosures.swift b/Sources/SwiftRefactor/CallToTrailingClosures.swift index c096bca63d8..d6e50673a71 100644 --- a/Sources/SwiftRefactor/CallToTrailingClosures.swift +++ b/Sources/SwiftRefactor/CallToTrailingClosures.swift @@ -78,7 +78,7 @@ extension FunctionCallExprSyntax { // Trailing comma won't exist any more, move its trivia to the end of // the closure instead if let comma = arg.trailingComma { - closure.trailingTrivia = closure.trailingTrivia.merging(triviaOf: comma) + closure.trailingTrivia = closure.trailingTrivia.mergingCommonSuffix(triviaOf: comma) } closures.append((arg, closure)) } @@ -88,12 +88,12 @@ extension FunctionCallExprSyntax { } // First trailing closure won't have label/colon. Transfer their trivia. - var trailingClosure = closures.first!.closure + let (firstOriginal, firstClosure) = closures.first! + var trailingClosure = firstClosure trailingClosure.leadingTrivia = - Trivia() - .merging(triviaOf: closures.first!.original.label) - .merging(triviaOf: closures.first!.original.colon) - .merging(closures.first!.closure.leadingTrivia) + (firstOriginal.label?.triviaByMergingCommonSuffix ?? []) + .mergingCommonSuffix(triviaOf: firstOriginal.colon) + .mergingCommonSuffix(firstClosure.leadingTrivia) .droppingLeadingWhitespace let additionalTrailingClosures = closures.dropFirst().map { MultipleTrailingClosureElementSyntax( @@ -118,21 +118,20 @@ extension FunctionCallExprSyntax { // No left paren any more, right paren is handled below since it makes // sense to keep its trivia of the end of the call, regardless of whether // it was removed or not. - if let leftParen = leftParen { - trailingClosure.leadingTrivia = Trivia() - .merging(triviaOf: leftParen) - .merging(trailingClosure.leadingTrivia) + if let leftParen { + trailingClosure.leadingTrivia = leftParen.triviaByMergingCommonSuffix + .mergingCommonSuffix(trailingClosure.leadingTrivia) } // No right paren anymore. Attach its trivia to the end of the call. - if let rightParen = rightParen { - additionalTriviaAtEndOfCall = Trivia().merging(triviaOf: rightParen) + if let rightParen { + additionalTriviaAtEndOfCall = rightParen.triviaByMergingCommonSuffix } } else { let last = argList.last! // Move the trailing trivia of the closing parenthesis to the end of the call after the last trailing, instead of // keeping it in the middle of the call where the new closing parenthesis lives. // Also ensure that we don't drop trivia from any comma we remove. - converted.rightParen?.trailingTrivia = Trivia().merging(triviaOf: last.trailingComma) + converted.rightParen?.trailingTrivia = last.trailingComma?.triviaByMergingCommonSuffix ?? [] additionalTriviaAtEndOfCall = rightParen?.trailingTrivia argList[argList.count - 1] = last.with(\.trailingComma, nil) } @@ -145,7 +144,9 @@ extension FunctionCallExprSyntax { } if let additionalTriviaAtEndOfCall { - converted.trailingTrivia = converted.trailingTrivia.merging(additionalTriviaAtEndOfCall.droppingLeadingWhitespace) + converted.trailingTrivia = converted.trailingTrivia.mergingCommonSuffix( + additionalTriviaAtEndOfCall.droppingLeadingWhitespace + ) } return converted diff --git a/Tests/SwiftParserTest/DeclarationTests.swift b/Tests/SwiftParserTest/DeclarationTests.swift index f5c70fe4a3d..81d403469dc 100644 --- a/Tests/SwiftParserTest/DeclarationTests.swift +++ b/Tests/SwiftParserTest/DeclarationTests.swift @@ -960,18 +960,18 @@ final class DeclarationTests: ParserTestCase { fixIts: ["move 'throws(any Error)' in front of '->'"] ) ], - fixedSource: "func test() throws(any Error) -> Int" + fixedSource: "func test() throws(any Error) -> Int" ) assertParse( - "func test() -> 1️⃣throws(any Error Int", + "func test() -> 1️⃣throws(any Error/* */ Int", diagnostics: [ DiagnosticSpec( message: "'throws(any Error' must precede '->'", fixIts: ["move 'throws(any Error' in front of '->'"] ) ], - fixedSource: "func test() throws(any Error -> Int" + fixedSource: "func test() throws(any Error -> /* */ Int" ) assertParse( @@ -986,14 +986,14 @@ final class DeclarationTests: ParserTestCase { ) assertParse( - "func test() -> 1️⃣throws (any Error) Int", + "func test() -> 1️⃣throws/**/ (any Error) Int", diagnostics: [ DiagnosticSpec( message: "'throws' must precede '->'", fixIts: ["move 'throws' in front of '->'"] ) ], - fixedSource: "func test() throws -> (any Error) Int" + fixedSource: "func test() throws -> /**/ (any Error) Int" ) } diff --git a/Tests/SwiftParserTest/translated/RecoveryTests.swift b/Tests/SwiftParserTest/translated/RecoveryTests.swift index aba6d85f9ea..476c7396a28 100644 --- a/Tests/SwiftParserTest/translated/RecoveryTests.swift +++ b/Tests/SwiftParserTest/translated/RecoveryTests.swift @@ -774,7 +774,7 @@ final class RecoveryTests: ParserTestCase { ), ], fixedSource: """ - for <#pattern#> in <#expression#> { + for <#pattern#> in <#expression#> { } """ ) diff --git a/Tests/SwiftParserTest/translated/SwitchTests.swift b/Tests/SwiftParserTest/translated/SwitchTests.swift index 1e9a2778236..aa3b81ab447 100644 --- a/Tests/SwiftParserTest/translated/SwitchTests.swift +++ b/Tests/SwiftParserTest/translated/SwitchTests.swift @@ -276,7 +276,6 @@ final class SwitchTests: ParserTestCase { fixedSource: """ switch x { case 0: - case 1: x = 0 }