From acb3dba75aa49bdd7d358c0d5e92fd13bf6581ae Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 28 Mar 2023 15:01:01 -0700 Subject: [PATCH] Sligthly clean up `FixIt.Change` and `FixIt.Changes` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing API with `FixIt.Change` and `FixIt.Changes` and the fact that we passed `[FixIt.Changes]` around what fairly weird. Ideally, we would only have `FixIt.Change` and make it powerful enough to represent all the trivia transfer that `FixIt.Changes` is doing. But given that that will be a larger effort, let’s make some smaller changes now that reduce the badness of the public API impact: - Move `FixIt.Changes` to `SwiftParserDiagnostics`, make it internal and rename it to `MultiNodeChange`. That way this type is no longer exposed in the public API, so we can iterate on it or remove it without breaking clients. The only thing that remains exposed is `FixIt.Change`, which we want to continue to support. We will probably add more cases to it in the future. - Make `FixIt.Changes` no longer conform to `ExpressibleByArrayLiteral`. That just makes everything a lot more explicit and easier to follow. - Remove some unecessary initializations of `FixIt.Changes` where `FixIt.Change` was sufficient. - Make `presence` on `TokenSyntax` settable so you can do `token.with(\.presence, .missing)` - Slightly unrelated: Rename SyntaxOtherNodes.swift to TokenSyntax.swift because that’s the only node it contains. --- .../Diagnostics.swift | 2 +- Sources/SwiftDiagnostics/FixIt.swift | 22 +----- .../DiagnosticExtensions.swift | 79 +++++++++++++------ .../LexerDiagnosticMessages.swift | 6 +- .../MissingNodesError.swift | 4 +- .../MissingTokenError.swift | 2 +- ...ineStringLiteralDiagnosticsGenerator.swift | 2 +- .../ParseDiagnosticsGenerator.swift | 18 ++--- Sources/SwiftSyntax/CMakeLists.txt | 2 +- .../SwiftSyntax/Raw/RawSyntaxTokenView.swift | 15 ++++ Sources/SwiftSyntax/SyntaxData.swift | 8 ++ ...ntaxOtherNodes.swift => TokenSyntax.swift} | 7 +- Tests/SwiftParserTest/Assertions.swift | 2 +- utils/group.json | 2 +- 14 files changed, 105 insertions(+), 66 deletions(-) rename Sources/SwiftSyntax/{SyntaxOtherNodes.swift => TokenSyntax.swift} (96%) diff --git a/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift b/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift index f3c458a112f..72b9df65586 100644 --- a/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift +++ b/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift @@ -84,7 +84,7 @@ extension PluginMessage.Diagnostic { self.fixIts = syntaxDiag.fixIts.compactMap { PluginMessage.Diagnostic.FixIt( message: $0.message.message, - changes: $0.changes.changes.compactMap { + changes: $0.changes.compactMap { let range: SourceManager.SourceRange? let text: String switch $0 { diff --git a/Sources/SwiftDiagnostics/FixIt.swift b/Sources/SwiftDiagnostics/FixIt.swift index 727d945f372..3d83314def4 100644 --- a/Sources/SwiftDiagnostics/FixIt.swift +++ b/Sources/SwiftDiagnostics/FixIt.swift @@ -25,22 +25,6 @@ public protocol FixItMessage { /// A Fix-It that can be applied to resolve a diagnostic. public struct FixIt { - public struct Changes: ExpressibleByArrayLiteral { - public var changes: [Change] - - public init(changes: [Change]) { - self.changes = changes - } - - public init(arrayLiteral elements: FixIt.Change...) { - self.init(changes: elements) - } - - public init(combining: [Changes]) { - self.init(changes: combining.flatMap(\.changes)) - } - } - public enum Change { /// Replace `oldNode` by `newNode`. case replace(oldNode: Syntax, newNode: Syntax) @@ -54,10 +38,10 @@ public struct FixIt { public let message: FixItMessage /// The changes that need to be performed when the Fix-It is applied. - public let changes: Changes + public let changes: [Change] - public init(message: FixItMessage, changes: Changes) { - precondition(!changes.changes.isEmpty, "A Fix-It must have at least one change associated with it") + public init(message: FixItMessage, changes: [Change]) { + precondition(!changes.isEmpty, "A Fix-It must have at least one change associated with it") self.message = message self.changes = changes } diff --git a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift index 9d3d0b58f3e..5d1cb1eb950 100644 --- a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift +++ b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift @@ -15,8 +15,32 @@ import SwiftBasicFormat import SwiftSyntax extension FixIt { - public init(message: FixItMessage, changes: [Changes]) { - self.init(message: message, changes: FixIt.Changes(combining: changes)) + /// A more complex set of changes that affects multiple syntax nodes and thus + /// produces multiple `FixIt.Change`s. This allows us to e.g. mark a node as + /// missing but keep the trivia by transferring it to the previous or next + /// token. + struct MultiNodeChange { + var primitiveChanges: [Change] + + init(primitiveChanges: [Change]) { + self.primitiveChanges = primitiveChanges + } + + init(_ primitiveChanges: Change...) { + self.init(primitiveChanges: primitiveChanges) + } + + init(combining: [MultiNodeChange]) { + self.init(primitiveChanges: combining.flatMap(\.primitiveChanges)) + } + } + + init(message: FixItMessage, changes: [MultiNodeChange]) { + self.init(message: message, changes: MultiNodeChange(combining: changes)) + } + + init(message: FixItMessage, changes: MultiNodeChange) { + self.init(message: message, changes: changes.primitiveChanges) } // These overloads shouldn't be needed, but are currently required for the @@ -24,15 +48,18 @@ extension FixIt { // leading-dot syntax. // TODO: These can be dropped once we require a minimum of Swift 5.6 to // compile the library. - init(message: StaticParserFixIt, changes: Changes) { - self.init(message: message as FixItMessage, changes: changes) + init(message: StaticParserFixIt, changes: MultiNodeChange) { + self.init(message: message as FixItMessage, changes: changes.primitiveChanges) } - init(message: StaticParserFixIt, changes: [Changes]) { - self.init(message: message as FixItMessage, changes: FixIt.Changes(combining: changes)) + init(message: StaticParserFixIt, changes: [MultiNodeChange]) { + self.init(message: message as FixItMessage, changes: MultiNodeChange(combining: changes).primitiveChanges) + } + public init(message: StaticParserFixIt, changes: [Change]) { + self.init(message: message as FixItMessage, changes: changes) } } -extension FixIt.Changes { +extension FixIt.MultiNodeChange { /// Replaced a present token with a missing node. /// If `transferTrivia` is `true`, the leading and trailing trivia of the /// removed node will be transferred to the trailing trivia of the previous token. @@ -49,26 +76,26 @@ extension FixIt.Changes { var changes = tokens.map { FixIt.Change.replace( oldNode: Syntax($0), - newNode: Syntax(TokenSyntax($0.tokenKind, leadingTrivia: [], trailingTrivia: [], presence: .missing)) + newNode: Syntax($0.with(\.presence, .missing)) ) } if transferTrivia { - changes += FixIt.Changes.transferTriviaAtSides(from: tokens).changes + changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: tokens).primitiveChanges } - return FixIt.Changes(changes: changes) + return FixIt.MultiNodeChange(primitiveChanges: changes) } /// 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: SyntaxType?, transferTrivia: Bool = true) -> Self { guard let node = node else { - return FixIt.Changes(changes: []) + return FixIt.MultiNodeChange(primitiveChanges: []) } var changes = [FixIt.Change.replace(oldNode: Syntax(node), newNode: MissingMaker().visit(Syntax(node)))] if transferTrivia { - changes += FixIt.Changes.transferTriviaAtSides(from: [node]).changes + changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: [node]).primitiveChanges } - return FixIt.Changes(changes: changes) + return FixIt.MultiNodeChange(primitiveChanges: changes) } /// Make a node present. If `leadingTrivia` or `trailingTrivia` is specified, @@ -89,13 +116,13 @@ extension FixIt.Changes { let nextToken = node.nextToken(viewMode: .sourceAccurate), leadingTrivia == nil { - return [ + return FixIt.MultiNodeChange( .replace( oldNode: Syntax(node), newNode: Syntax(presentNode).with(\.leadingTrivia, nextToken.leadingTrivia) ), - .replaceLeadingTrivia(token: nextToken, newTrivia: []), - ] + .replaceLeadingTrivia(token: nextToken, newTrivia: []) + ) } else if node.leadingTrivia.isEmpty, let previousToken = node.previousToken(viewMode: .fixedUp), previousToken.presence == .present, @@ -105,19 +132,19 @@ extension FixIt.Changes { { /// If neither this nor the previous token are punctionation make sure they /// are separated by a space. - return [ + return FixIt.MultiNodeChange( .replace( oldNode: Syntax(node), newNode: Syntax(presentNode).with(\.leadingTrivia, .space) ) - ] + ) } else { - return [ + return FixIt.MultiNodeChange( .replace( oldNode: Syntax(node), newNode: Syntax(presentNode) ) - ] + ) } } @@ -128,10 +155,10 @@ extension FixIt.Changes { if !previousToken.trailingTrivia.isEmpty { presentToken = presentToken.with(\.trailingTrivia, previousToken.trailingTrivia) } - return [ + return FixIt.MultiNodeChange( .replaceTrailingTrivia(token: previousToken, newTrivia: []), - .replace(oldNode: Syntax(token), newNode: Syntax(presentToken)), - ] + .replace(oldNode: Syntax(token), newNode: Syntax(presentToken)) + ) } else { return .makePresent(token) } @@ -149,11 +176,11 @@ extension FixIt.Changes { // 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 [] + return FixIt.MultiNodeChange() } - return [.replaceTrailingTrivia(token: previousToken, newTrivia: mergedTrivia)] + return FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previousToken, newTrivia: mergedTrivia)) } else { - return [] + return FixIt.MultiNodeChange() } } } diff --git a/Sources/SwiftParserDiagnostics/LexerDiagnosticMessages.swift b/Sources/SwiftParserDiagnostics/LexerDiagnosticMessages.swift index 65478cad737..82f2ccbb7a5 100644 --- a/Sources/SwiftParserDiagnostics/LexerDiagnosticMessages.swift +++ b/Sources/SwiftParserDiagnostics/LexerDiagnosticMessages.swift @@ -192,7 +192,7 @@ public extension SwiftSyntax.TokenDiagnostic { .with(\.leadingTrivia, Trivia(pieces: token.leadingTrivia.map(replaceNonBreakingSpace))) .with(\.trailingTrivia, Trivia(pieces: token.trailingTrivia.map(replaceNonBreakingSpace))) return [ - FixIt(message: .replaceNonBreakingSpaceBySpace, changes: [[.replace(oldNode: Syntax(token), newNode: Syntax(fixedToken))]]) + FixIt(message: .replaceNonBreakingSpaceBySpace, changes: [.replace(oldNode: Syntax(token), newNode: Syntax(fixedToken))]) ] case .unicodeCurlyQuote: let (rawKind, text) = token.tokenKind.decomposeToRaw() @@ -206,7 +206,7 @@ public extension SwiftSyntax.TokenDiagnostic { let fixedToken = token.withKind(TokenKind.fromRaw(kind: rawKind, text: replacedText)) return [ - FixIt(message: .replaceCurlyQuoteByNormalQuote, changes: [[.replace(oldNode: Syntax(token), newNode: Syntax(fixedToken))]]) + FixIt(message: .replaceCurlyQuoteByNormalQuote, changes: [.replace(oldNode: Syntax(token), newNode: Syntax(fixedToken))]) ] case .equalMustHaveConsistentWhitespaceOnBothSides: let hasLeadingSpace = token.previousToken(viewMode: .all)?.trailingTrivia.contains(where: { $0.isSpaceOrTab }) ?? false @@ -226,7 +226,7 @@ public extension SwiftSyntax.TokenDiagnostic { } return [ - FixIt(message: .insertWhitespace, changes: FixIt.Changes(changes: changes)) + FixIt(message: .insertWhitespace, changes: changes) ] default: return [] diff --git a/Sources/SwiftParserDiagnostics/MissingNodesError.swift b/Sources/SwiftParserDiagnostics/MissingNodesError.swift index 705a6e67414..9e17832a686 100644 --- a/Sources/SwiftParserDiagnostics/MissingNodesError.swift +++ b/Sources/SwiftParserDiagnostics/MissingNodesError.swift @@ -316,7 +316,7 @@ extension ParseDiagnosticsGenerator { func handleMissingSyntax( _ node: T, overridePosition: AbsolutePosition? = nil, - additionalChanges: [FixIt.Changes] = [], + additionalChanges: [FixIt.MultiNodeChange] = [], additionalHandledNodes: [SyntaxIdentifier] = [] ) -> SyntaxVisitorContinueKind { if shouldSkip(node) { @@ -356,7 +356,7 @@ extension ParseDiagnosticsGenerator { } } - let changes = missingNodes.enumerated().map { (index, missingNode) -> FixIt.Changes in + let changes = missingNodes.enumerated().map { (index, missingNode) -> FixIt.MultiNodeChange in if index == 0, let token = missingNode.as(TokenSyntax.self), let previousTokenKind = missingNode.previousToken(viewMode: .sourceAccurate)?.tokenKind diff --git a/Sources/SwiftParserDiagnostics/MissingTokenError.swift b/Sources/SwiftParserDiagnostics/MissingTokenError.swift index c18699209a7..791cfe52855 100644 --- a/Sources/SwiftParserDiagnostics/MissingTokenError.swift +++ b/Sources/SwiftParserDiagnostics/MissingTokenError.swift @@ -97,7 +97,7 @@ extension ParseDiagnosticsGenerator { private func handleInvalidPeriod(invalidToken: TokenSyntax, missingToken: TokenSyntax, invalidTokenContainer: UnexpectedNodesSyntax) -> Bool { // Trailing trivia is the cause of this diagnostic, don't transfer it. - let changes: [FixIt.Changes] = [ + let changes: [FixIt.MultiNodeChange] = [ .makeMissing(invalidToken, transferTrivia: false), .makePresent(missingToken), ] diff --git a/Sources/SwiftParserDiagnostics/MultiLineStringLiteralDiagnosticsGenerator.swift b/Sources/SwiftParserDiagnostics/MultiLineStringLiteralDiagnosticsGenerator.swift index b7dfcf42dee..9eaac087175 100644 --- a/Sources/SwiftParserDiagnostics/MultiLineStringLiteralDiagnosticsGenerator.swift +++ b/Sources/SwiftParserDiagnostics/MultiLineStringLiteralDiagnosticsGenerator.swift @@ -115,7 +115,7 @@ final class MultiLineStringLiteralIndentatinDiagnosticsGenerator: SyntaxVisitor message: InvalidIndentationInMultiLineStringLiteralError(kind: currentDiagnostic.kind, lines: currentDiagnostic.lines), highlights: [], notes: [Note(node: Syntax(closeQuote), message: .shouldMatchIndentationOfClosingQuote)], - fixIts: [FixIt(message: .changeIndentationToMatchClosingDelimiter, changes: FixIt.Changes(changes: currentDiagnostic.changes))] + fixIts: [FixIt(message: .changeIndentationToMatchClosingDelimiter, changes: currentDiagnostic.changes)] ) finishedDiagnostics.append((diagnostic, currentDiagnostic.handledNodes)) diff --git a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift index b315dab41c5..da1a210f2a7 100644 --- a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift +++ b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift @@ -143,23 +143,23 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { // Ignore `correctTokens` that are already present. let correctAndMissingTokens = correctTokens.filter({ $0.presence == .missing }) - var changes: [FixIt.Changes] = [] + var changes: [FixIt.MultiNodeChange] = [] if let misplacedToken = misplacedTokens.only, let correctToken = correctTokens.only, misplacedToken.nextToken(viewMode: .all) == correctToken || misplacedToken.previousToken(viewMode: .all) == correctToken, correctToken.presence == .missing { // We are exchanging two adjacent tokens, transfer the trivia from the incorrect token to the corrected token. - changes += misplacedTokens.map { FixIt.Changes.makeMissing($0, transferTrivia: false) } + changes += misplacedTokens.map { FixIt.MultiNodeChange.makeMissing($0, transferTrivia: false) } changes.append( - FixIt.Changes.makePresent( + FixIt.MultiNodeChange.makePresent( correctToken, leadingTrivia: misplacedToken.leadingTrivia, trailingTrivia: misplacedToken.trailingTrivia ) ) } else { - changes += misplacedTokens.map { FixIt.Changes.makeMissing($0) } - changes += correctAndMissingTokens.map { FixIt.Changes.makePresent($0) } + changes += misplacedTokens.map { FixIt.MultiNodeChange.makeMissing($0) } + changes += correctAndMissingTokens.map { FixIt.MultiNodeChange.makePresent($0) } } var fixIts: [FixIt] = [] if changes.count > 1 { @@ -309,7 +309,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { node, .unexpectedSemicolon, fixIts: [ - FixIt(message: RemoveNodesFixIt(semicolons), changes: semicolons.map { FixIt.Changes.makeMissing($0) }) + FixIt(message: RemoveNodesFixIt(semicolons), changes: semicolons.map { FixIt.MultiNodeChange.makeMissing($0) }) ] ) } else if node.first?.as(TokenSyntax.self)?.tokenKind.isIdentifier == true, @@ -327,7 +327,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { FixIt( message: .joinIdentifiers, changes: [ - [.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joined), presence: .present)))], + FixIt.MultiNodeChange(.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joined), presence: .present)))), .makeMissing(tokens), ] ) @@ -338,7 +338,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { FixIt( message: .joinIdentifiersWithCamelCase, changes: [ - [.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joinedUsingCamelCase), presence: .present)))], + FixIt.MultiNodeChange(.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joinedUsingCamelCase), presence: .present)))), .makeMissing(tokens), ] ) @@ -821,7 +821,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { message: RemoveNodesFixIt(unexpectedName), changes: [ .makeMissing(unexpectedName), - FixIt.Changes(changes: [.replaceTrailingTrivia(token: previous, newTrivia: [])]), + FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previous, newTrivia: [])), ] ) ], diff --git a/Sources/SwiftSyntax/CMakeLists.txt b/Sources/SwiftSyntax/CMakeLists.txt index 0428a718549..5324a522c33 100644 --- a/Sources/SwiftSyntax/CMakeLists.txt +++ b/Sources/SwiftSyntax/CMakeLists.txt @@ -19,10 +19,10 @@ add_swift_host_library(SwiftSyntax SyntaxArena.swift SyntaxChildren.swift SyntaxData.swift - SyntaxOtherNodes.swift SyntaxText.swift SyntaxTreeViewMode.swift TokenDiagnostic.swift + TokenSyntax.swift Utils.swift Raw/RawSyntax.swift diff --git a/Sources/SwiftSyntax/Raw/RawSyntaxTokenView.swift b/Sources/SwiftSyntax/Raw/RawSyntaxTokenView.swift index f7dacd09f59..8168bfbfaa7 100644 --- a/Sources/SwiftSyntax/Raw/RawSyntaxTokenView.swift +++ b/Sources/SwiftSyntax/Raw/RawSyntaxTokenView.swift @@ -197,6 +197,21 @@ public struct RawSyntaxTokenView { } } + /// Returns a `RawSyntax` node with the presence changed to `newValue`. + @_spi(RawSyntax) + public func withPresence(_ newValue: SourcePresence, arena: SyntaxArena) -> RawSyntax { + switch raw.rawData.payload { + case .parsedToken(var payload): + payload.presence = newValue + return RawSyntax(arena: arena, payload: .parsedToken(payload)) + case .materializedToken(var payload): + payload.presence = newValue + return RawSyntax(arena: arena, payload: .materializedToken(payload)) + default: + preconditionFailure("'withKind()' is called on non-token raw syntax") + } + } + /// The length of the token without leading or trailing trivia, assuming this /// is a token node. @_spi(RawSyntax) diff --git a/Sources/SwiftSyntax/SyntaxData.swift b/Sources/SwiftSyntax/SyntaxData.swift index 5a14b338650..b2c513d5a58 100644 --- a/Sources/SwiftSyntax/SyntaxData.swift +++ b/Sources/SwiftSyntax/SyntaxData.swift @@ -336,6 +336,14 @@ struct SyntaxData { return self } } + + func withPresence(_ presence: SourcePresence, arena: SyntaxArena) -> SyntaxData { + if let raw = raw.tokenView?.withPresence(presence, arena: arena) { + return replacingSelf(raw, arena: arena) + } else { + return self + } + } } #if DEBUG diff --git a/Sources/SwiftSyntax/SyntaxOtherNodes.swift b/Sources/SwiftSyntax/TokenSyntax.swift similarity index 96% rename from Sources/SwiftSyntax/SyntaxOtherNodes.swift rename to Sources/SwiftSyntax/TokenSyntax.swift index 51fec81612e..78f83af3426 100644 --- a/Sources/SwiftSyntax/SyntaxOtherNodes.swift +++ b/Sources/SwiftSyntax/TokenSyntax.swift @@ -55,7 +55,12 @@ public struct TokenSyntax: SyntaxProtocol, SyntaxHashable { } public var presence: SourcePresence { - return tokenView.presence + get { + return tokenView.presence + } + set { + self = TokenSyntax(data.withPresence(newValue, arena: SyntaxArena())) + } } /// The text of the token as written in the source code, without any trivia. diff --git a/Tests/SwiftParserTest/Assertions.swift b/Tests/SwiftParserTest/Assertions.swift index 252af0dbb88..6635832feb0 100644 --- a/Tests/SwiftParserTest/Assertions.swift +++ b/Tests/SwiftParserTest/Assertions.swift @@ -285,7 +285,7 @@ class FixItApplier: SyntaxRewriter { return true } } - .flatMap { $0.changes.changes } + .flatMap { $0.changes } } public override func visitAny(_ node: Syntax) -> Syntax? { diff --git a/utils/group.json b/utils/group.json index 46687e23de2..19bb3704f62 100644 --- a/utils/group.json +++ b/utils/group.json @@ -27,10 +27,10 @@ "SyntaxDeclNodes.swift", "SyntaxExprNodes.swift", "SyntaxNodes.swift", - "SyntaxOtherNodes.swift", "SyntaxPatternNodes.swift", "SyntaxStmtNodes.swift", "SyntaxTypeNodes.swift", + "TokenSyntax.swift", ], "Utils": [ "CommonAncestor.swift",