From 5c1043fa72ae8ae5940a8ee7a07be00a8efd841e Mon Sep 17 00:00:00 2001 From: Kim de Vos Date: Thu, 21 Sep 2023 20:11:46 +0200 Subject: [PATCH] Rewrite FixItApplier to be string based --- .../DiagnosticExtensions.swift | 40 ++-- .../ParseDiagnosticsGenerator.swift | 26 ++- .../FixItApplier.swift | 118 ++++++++++++ Tests/SwiftParserTest/Assertions.swift | 54 +----- ...AvailabilityQueryUnavailabilityTests.swift | 2 +- .../PeerMacroTests.swift | 181 +++++++++++------- 6 files changed, 273 insertions(+), 148 deletions(-) create mode 100644 Sources/_SwiftSyntaxTestSupport/FixItApplier.swift diff --git a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift index 27fb25ccb5b..1d1f9983df5 100644 --- a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift +++ b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift @@ -48,6 +48,7 @@ extension FixIt { 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. static func makeMissing(_ token: TokenSyntax, transferTrivia: Bool = true) -> Self { @@ -55,21 +56,13 @@ extension FixIt.MultiNodeChange { } /// Replace present tokens with missing tokens. - /// If `transferTrivia` is `true`, the leading and trailing trivia of the - /// removed node will be transferred to the trailing trivia of the previous token. + /// + /// If `transferTrivia` is `true`, the leading trivia of the first token and + /// the trailing trivia of the last token will be transferred to their adjecent + /// tokens. static func makeMissing(_ tokens: [TokenSyntax], transferTrivia: Bool = true) -> Self { - precondition(!tokens.isEmpty) - precondition(tokens.allSatisfy({ $0.isPresent })) - var changes = tokens.map { - FixIt.Change.replace( - oldNode: Syntax($0), - newNode: Syntax($0.with(\.presence, .missing)) - ) - } - if transferTrivia { - changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: tokens).primitiveChanges - } - return FixIt.MultiNodeChange(primitiveChanges: changes) + precondition(tokens.allSatisfy(\.isPresent)) + return .makeMissing(tokens.map(Syntax.init), transferTrivia: transferTrivia) } /// If `transferTrivia` is `true`, the leading and trailing trivia of the @@ -104,6 +97,25 @@ extension FixIt.MultiNodeChange { return FixIt.MultiNodeChange() } } + + /// Replace present nodes with their missing equivalents. + /// + /// 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 { + precondition(!nodes.isEmpty) + var changes = nodes.map { + FixIt.Change.replace( + oldNode: $0, + newNode: MissingMaker().rewrite($0, detach: true) + ) + } + if transferTrivia { + changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: nodes).primitiveChanges + } + return FixIt.MultiNodeChange(primitiveChanges: changes) + } } // MARK: - Make present diff --git a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift index 2c8f339176d..4c216c1eb70 100644 --- a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift +++ b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift @@ -182,7 +182,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { correctToken.isMissing { // We are exchanging two adjacent tokens, transfer the trivia from the incorrect token to the corrected token. - changes += misplacedTokens.map { FixIt.MultiNodeChange.makeMissing($0, transferTrivia: false) } + changes.append(FixIt.MultiNodeChange.makeMissing(misplacedTokens, transferTrivia: false)) changes.append( FixIt.MultiNodeChange.makePresent( correctToken, @@ -236,7 +236,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { exchangeTokens( unexpected: misplacedSpecifiers, unexpectedTokenCondition: { EffectSpecifier(token: $0) != nil }, - correctTokens: [effectSpecifiers?.throwsSpecifier, effectSpecifiers?.asyncSpecifier], + correctTokens: [effectSpecifiers?.asyncSpecifier, effectSpecifiers?.throwsSpecifier], message: { EffectsSpecifierAfterArrow(effectsSpecifiersAfterArrow: $0) }, moveFixIt: { MoveTokensInFrontOfFixIt(movedTokens: $0, inFrontOf: .arrow) }, removeRedundantFixIt: { RemoveRedundantFixIt(removeTokens: $0) } @@ -764,10 +764,8 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { if let unexpected = node.unexpectedBetweenRequirementAndTrailingComma, let token = unexpected.presentTokens(satisfying: { $0.tokenKind == .binaryOperator("&&") }).first, let trailingComma = node.trailingComma, - trailingComma.isMissing, - let previous = node.unexpectedBetweenRequirementAndTrailingComma?.previousToken(viewMode: .sourceAccurate) + trailingComma.isMissing { - addDiagnostic( unexpected, .expectedCommaInWhereClause, @@ -775,9 +773,8 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { FixIt( message: ReplaceTokensFixIt(replaceTokens: [token], replacements: [.commaToken()]), changes: [ - .makeMissing(token), - .makePresent(trailingComma), - FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previous, newTrivia: [])), + .makeMissing(token, transferTrivia: false), + .makePresent(trailingComma, leadingTrivia: token.leadingTrivia, trailingTrivia: token.trailingTrivia), ] ) ], @@ -818,7 +815,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { fixIts: [ FixIt( message: RemoveNodesFixIt(nodes), - changes: nodes.map { .makeMissing($0) } + changes: .makeMissing(nodes) ) ], handledNodes: nodes.map { $0.id } @@ -1542,7 +1539,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { fixIts: [ FixIt( message: RemoveNodesFixIt(rawDelimiters), - changes: rawDelimiters.map { .makeMissing($0) } + changes: .makeMissing(rawDelimiters) ) ], handledNodes: rawDelimiters.map { $0.id } @@ -1862,8 +1859,8 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { replacements: [node.colon] ), changes: [ - FixIt.MultiNodeChange.makeMissing(equalToken), - FixIt.MultiNodeChange.makePresent(node.colon), + .makeMissing(equalToken, transferTrivia: false), + .makePresent(node.colon, leadingTrivia: equalToken.leadingTrivia, trailingTrivia: equalToken.trailingTrivia), ] ) ], @@ -1971,8 +1968,9 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { FixIt( message: fixItMessage, changes: [ - FixIt.MultiNodeChange.makePresent(detail.detail) - ] + unexpectedTokens.map { FixIt.MultiNodeChange.makeMissing($0) } + .makePresent(detail.detail), + .makeMissing(unexpectedTokens), + ] ) ], handledNodes: [detail.id] + unexpectedTokens.map(\.id) diff --git a/Sources/_SwiftSyntaxTestSupport/FixItApplier.swift b/Sources/_SwiftSyntaxTestSupport/FixItApplier.swift new file mode 100644 index 00000000000..ed43d45d549 --- /dev/null +++ b/Sources/_SwiftSyntaxTestSupport/FixItApplier.swift @@ -0,0 +1,118 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import SwiftDiagnostics +import SwiftSyntax + +public enum FixItApplier { + fileprivate struct Edit: Equatable { + let startUtf8Offset: Int + let endUtf8Offset: Int + let replacement: String + + var replacementLength: Int { + return replacement.utf8.count + } + } + + /// Applies selected or all Fix-Its from the provided diagnostics to a given syntax tree. + /// + /// - Parameters: + /// - diagnostics: An array of `Diagnostic` objects, each containing one or more Fix-Its. + /// - filterByMessages: An optional array of message strings to filter which Fix-Its to apply. + /// If `nil`, all Fix-Its in `diagnostics` are applied. + /// - tree: The syntax tree to which the Fix-Its will be applied. + /// + /// - Returns: A ``String`` representation of the modified syntax tree after applying the Fix-Its. + public static func applyFixes( + from diagnostics: [Diagnostic], + filterByMessages messages: [String]?, + to tree: any SyntaxProtocol + ) -> String { + let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message } + + let changes = + diagnostics + .flatMap(\.fixIts) + .filter { messages.contains($0.message.message) } + .flatMap(\.changes) + + var edits: [Edit] = [] + + for change in changes { + switch change { + case .replace(let oldNode, let newNode): + edits.append( + Edit( + startUtf8Offset: oldNode.position.utf8Offset, + endUtf8Offset: oldNode.endPosition.utf8Offset, + replacement: newNode.description + ) + ) + + case .replaceLeadingTrivia(let token, let newTrivia): + edits.append( + Edit( + startUtf8Offset: token.position.utf8Offset, + endUtf8Offset: token.endPosition.utf8Offset, + replacement: token.with(\.leadingTrivia, newTrivia).description + ) + ) + + case .replaceTrailingTrivia(let token, let newTrivia): + edits.append( + Edit( + startUtf8Offset: token.position.utf8Offset, + endUtf8Offset: token.endPosition.utf8Offset, + replacement: token.with(\.trailingTrivia, newTrivia).description + ) + ) + } + } + + var source = tree.description + var editedOffset = 0 + + // As we need to start apply the edits at the end of a source, start by reversing edit + // and then sort edits by decrementing start offset. If they are equal then descrementing end offset. + // edits = edits.reversed().sorted(by: { edit1, edit2 in + // if edit1.startUtf8Offset == edit2.startUtf8Offset { + // return edit1.endUtf8Offset > edit2.endUtf8Offset + // } else { + // return edit1.startUtf8Offset > edit2.startUtf8Offset + // } + // }) + + for edit in edits where edits.canInsert(editToApply: edit) { + let startUtf8Offset = edit.startUtf8Offset + editedOffset + let endUtf8Offset = edit.endUtf8Offset + editedOffset + + let startIndex = source.utf8.index(source.utf8.startIndex, offsetBy: startUtf8Offset) + let endIndex = source.utf8.index(source.utf8.startIndex, offsetBy: endUtf8Offset) + + source.replaceSubrange(startIndex.. Bool { + return self.contains { edit in + return !(editToApply.startUtf8Offset >= edit.startUtf8Offset + && editToApply.endUtf8Offset < edit.endUtf8Offset) + } + } +} diff --git a/Tests/SwiftParserTest/Assertions.swift b/Tests/SwiftParserTest/Assertions.swift index ffe3ccc4f7f..29216e25c02 100644 --- a/Tests/SwiftParserTest/Assertions.swift +++ b/Tests/SwiftParserTest/Assertions.swift @@ -276,58 +276,6 @@ struct DiagnosticSpec { } } -class FixItApplier: SyntaxRewriter { - var changes: [FixIt.Change] - - init(diagnostics: [Diagnostic], withMessages messages: [String]?) { - let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message } - - self.changes = - diagnostics - .flatMap { $0.fixIts } - .filter { - return messages.contains($0.message.message) - } - .flatMap { $0.changes } - - super.init(viewMode: .all) - } - - public override func visitAny(_ node: Syntax) -> Syntax? { - for change in changes { - switch change { - case .replace(oldNode: let oldNode, newNode: let newNode) where oldNode.id == node.id: - return newNode - default: - break - } - } - return nil - } - - override func visit(_ node: TokenSyntax) -> TokenSyntax { - var modifiedNode = node - for change in changes { - switch change { - case .replaceLeadingTrivia(token: let changedNode, newTrivia: let newTrivia) where changedNode.id == node.id: - modifiedNode = node.with(\.leadingTrivia, newTrivia) - case .replaceTrailingTrivia(token: let changedNode, newTrivia: let newTrivia) where changedNode.id == node.id: - modifiedNode = node.with(\.trailingTrivia, newTrivia) - default: - break - } - } - return modifiedNode - } - - /// If `messages` is `nil`, applies all Fix-Its in `diagnostics` to `tree` and returns the fixed syntax tree. - /// If `messages` is not `nil`, applies only Fix-Its whose message is in `messages`. - public static func applyFixes(in diagnostics: [Diagnostic], withMessages messages: [String]?, to tree: T) -> Syntax { - let applier = FixItApplier(diagnostics: diagnostics, withMessages: messages) - return applier.rewrite(tree) - } -} - /// Assert that `location` is the same as that of `locationMarker` in `tree`. func assertLocation( _ location: SourceLocation, @@ -679,7 +627,7 @@ extension ParserTestCase { if expectedDiagnostics.contains(where: { !$0.fixIts.isEmpty }) && expectedFixedSource == nil { XCTFail("Expected a fixed source if the test case produces diagnostics with Fix-Its", file: file, line: line) } else if let expectedFixedSource = expectedFixedSource { - let fixedTree = FixItApplier.applyFixes(in: diags, withMessages: applyFixIts, to: tree) + let fixedTree = FixItApplier.applyFixes(from: diags, filterByMessages: applyFixIts, to: tree) var fixedTreeDescription = fixedTree.description if options.contains(.normalizeNewlinesInFixedSource) { fixedTreeDescription = diff --git a/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift b/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift index b88ad0639f8..e975ee828cc 100644 --- a/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift +++ b/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift @@ -572,7 +572,7 @@ final class AvailabilityQueryUnavailabilityTests: ParserTestCase { ), ], fixedSource: """ - if #unavailable(*) , true { + if #unavailable(*), true { } """ ) diff --git a/Tests/SwiftSyntaxMacroExpansionTest/PeerMacroTests.swift b/Tests/SwiftSyntaxMacroExpansionTest/PeerMacroTests.swift index b7f2e9647f5..24b24c032ac 100644 --- a/Tests/SwiftSyntaxMacroExpansionTest/PeerMacroTests.swift +++ b/Tests/SwiftSyntaxMacroExpansionTest/PeerMacroTests.swift @@ -28,89 +28,117 @@ import XCTest final class PeerMacroTests: XCTestCase { private let indentationWidth: Trivia = .spaces(2) - func testAddCompletionHandler() { - struct AddCompletionHandler: PeerMacro { - static func expansion( - of node: AttributeSyntax, - providingPeersOf declaration: some DeclSyntaxProtocol, - in context: some MacroExpansionContext - ) throws -> [DeclSyntax] { - // Only on functions at the moment. We could handle initializers as well - // with a bit of work. - guard let funcDecl = declaration.as(FunctionDeclSyntax.self) else { - throw MacroExpansionErrorMessage("@addCompletionHandler only works on functions") - } + fileprivate struct AddCompletionHandler: PeerMacro { + static func expansion( + of node: AttributeSyntax, + providingPeersOf declaration: some DeclSyntaxProtocol, + in context: some MacroExpansionContext + ) throws -> [DeclSyntax] { + // Only on functions at the moment. We could handle initializers as well + // with a bit of work. + guard let funcDecl = declaration.as(FunctionDeclSyntax.self) else { + throw MacroExpansionErrorMessage("@addCompletionHandler only works on functions") + } - // This only makes sense for async functions. - if funcDecl.signature.effectSpecifiers?.asyncSpecifier == nil { - throw MacroExpansionErrorMessage( - "@addCompletionHandler requires an async function" - ) + // This only makes sense for async functions. + if funcDecl.signature.effectSpecifiers?.asyncSpecifier == nil { + let newEffects: FunctionEffectSpecifiersSyntax + if let existingEffects = funcDecl.signature.effectSpecifiers { + newEffects = existingEffects.with(\.asyncSpecifier, .keyword(.async)) + } else { + newEffects = FunctionEffectSpecifiersSyntax(asyncSpecifier: .keyword(.async)) } - // Form the completion handler parameter. - let resultType: TypeSyntax? = funcDecl.signature.returnClause?.type.trimmed - - let completionHandlerParam = - FunctionParameterSyntax( - firstName: .identifier("completionHandler"), - colon: .colonToken(trailingTrivia: .space), - type: TypeSyntax("(\(resultType ?? "")) -> Void") - ) - - // Add the completion handler parameter to the parameter list. - let parameterList = funcDecl.signature.parameterClause.parameters - var newParameterList = parameterList - if !parameterList.isEmpty { - // We need to add a trailing comma to the preceding list. - newParameterList[newParameterList.index(before: newParameterList.endIndex)].trailingComma = .commaToken(trailingTrivia: .space) - } - newParameterList.append(completionHandlerParam) + let newSignature = funcDecl.signature.with(\.effectSpecifiers, newEffects) + + let diag = Diagnostic( + node: Syntax(funcDecl.funcKeyword), + message: MacroExpansionErrorMessage( + "can only add a completion-handler variant to an 'async' function" + ), + fixIts: [ + FixIt( + message: MacroExpansionFixItMessage( + "add 'async'" + ), + changes: [ + FixIt.Change.replace( + oldNode: Syntax(funcDecl.signature), + newNode: Syntax(newSignature) + ) + ] + ) + ] + ) + + context.diagnose(diag) + return [] + } - let callArguments: [String] = parameterList.map { param in - let argName = param.secondName ?? param.firstName + // Form the completion handler parameter. + let resultType: TypeSyntax? = funcDecl.signature.returnClause?.type.trimmed + + let completionHandlerParam = + FunctionParameterSyntax( + firstName: .identifier("completionHandler"), + colon: .colonToken(trailingTrivia: .space), + type: TypeSyntax("(\(resultType ?? "")) -> Void") + ) + + // Add the completion handler parameter to the parameter list. + let parameterList = funcDecl.signature.parameterClause.parameters + var newParameterList = parameterList + if !parameterList.isEmpty { + // We need to add a trailing comma to the preceding list. + newParameterList[newParameterList.index(before: newParameterList.endIndex)].trailingComma = .commaToken(trailingTrivia: .space) + } + newParameterList.append(completionHandlerParam) - if param.firstName.text != "_" { - return "\(param.firstName.text): \(argName.text)" - } + let callArguments: [String] = parameterList.map { param in + let argName = param.secondName ?? param.firstName - return "\(argName.text)" + if param.firstName.text != "_" { + return "\(param.firstName.text): \(argName.text)" } - let call: ExprSyntax = - "\(funcDecl.name)(\(raw: callArguments.joined(separator: ", ")))" - - // FIXME: We should make CodeBlockSyntax ExpressibleByStringInterpolation, - // so that the full body could go here. - let newBody: ExprSyntax = - """ + return "\(argName.text)" + } - Task { - completionHandler(await \(call)) - } + let call: ExprSyntax = + "\(funcDecl.name)(\(raw: callArguments.joined(separator: ", ")))" - """ + // FIXME: We should make CodeBlockSyntax ExpressibleByStringInterpolation, + // so that the full body could go here. + let newBody: ExprSyntax = + """ - // Drop the @addCompletionHandler attribute from the new declaration. - let newAttributeList = funcDecl.attributes.filter { - guard case let .attribute(attribute) = $0 else { - return true + Task { + completionHandler(await \(call)) } - return attribute.attributeName.as(IdentifierTypeSyntax.self)?.name == "addCompletionHandler" - } - var newFunc = funcDecl - newFunc.signature.effectSpecifiers?.asyncSpecifier = nil // drop async - newFunc.signature.returnClause = nil // drop result type - newFunc.signature.parameterClause.parameters = newParameterList - newFunc.signature.parameterClause.trailingTrivia = [] - newFunc.body = CodeBlockSyntax { newBody } - newFunc.attributes = newAttributeList + """ - return [DeclSyntax(newFunc)] + // Drop the @addCompletionHandler attribute from the new declaration. + let newAttributeList = funcDecl.attributes.filter { + guard case let .attribute(attribute) = $0 else { + return true + } + return attribute.attributeName.as(IdentifierTypeSyntax.self)?.name == "addCompletionHandler" } + + var newFunc = funcDecl + newFunc.signature.effectSpecifiers?.asyncSpecifier = nil // drop async + newFunc.signature.returnClause = nil // drop result type + newFunc.signature.parameterClause.parameters = newParameterList + newFunc.signature.parameterClause.trailingTrivia = [] + newFunc.body = CodeBlockSyntax { newBody } + newFunc.attributes = newAttributeList + + return [DeclSyntax(newFunc)] } + } + func testAddCompletionHandler() { assertMacroExpansion( """ @addCompletionHandler @@ -193,4 +221,25 @@ final class PeerMacroTests: XCTestCase { ] ) } + + func testAddCompletionHandlerWhereThereIsNotAsync() { + assertMacroExpansion( + """ + @addCompletionHandler + func f(a: Int, for b: String, _ value: Double) -> String { } + """, + expandedSource: """ + func f(a: Int, for b: String, _ value: Double) -> String { } + """, + diagnostics: [ + DiagnosticSpec( + message: "can only add a completion-handler variant to an 'async' function", + line: 2, + column: 1, + fixIts: [FixItSpec(message: "add 'async'")] + ) + ], + macros: ["addCompletionHandler": AddCompletionHandler.self] + ) + } }