From 8bb0a0abf8810a849a81bd5c4c69867a3b09e60c Mon Sep 17 00:00:00 2001 From: Kai Lau Date: Tue, 9 Jul 2024 16:01:08 -0700 Subject: [PATCH] Fix Accessor Macros Attached to Properties With Trailing Comments fixed the bug that trailing comments of a property would stay in place after macro expansion, commenting out the left brace of the newly created accessor block - the bug affected only properties without an accessor block while having trailing line comments - fixed by moving the trailing trivia of the variable declaration to the trailing trivia of the left brace of the accessor block added tests on properties with trailing comments and with/without an accessor block --- .../MacroSystem.swift | 22 +++- .../AccessorMacroTests.swift | 121 ++++++++++++++++++ 2 files changed, 138 insertions(+), 5 deletions(-) diff --git a/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift b/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift index 910e69553c6..bed3e1082f3 100644 --- a/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift +++ b/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift @@ -923,7 +923,9 @@ private class MacroApplication: SyntaxRewriter { return DeclSyntax(node) } - guard node.bindings.count == 1, let binding = node.bindings.first else { + guard node.bindings.count == 1, + var binding = node.bindings.first + else { contextGenerator(Syntax(node)).addDiagnostics( from: MacroApplicationError.accessorMacroOnVariableWithMultipleBindings, node: node @@ -931,17 +933,27 @@ private class MacroApplication: SyntaxRewriter { return DeclSyntax(node) } - let expansion = expandAccessors(of: node, existingAccessors: binding.accessorBlock) + var expansion = expandAccessors(of: node, existingAccessors: binding.accessorBlock) + if expansion.accessors != binding.accessorBlock { + if binding.accessorBlock == nil { + // remove the trailing trivia of the variable declaration and move it + // to the trailing trivia of the left brace of the newly created accessor block + expansion.accessors?.leftBrace.trailingTrivia = binding.trailingTrivia + binding.trailingTrivia = [] + } + if binding.initializer != nil, expansion.expandsGetSet { // The accessor block will have a leading space, but there will already be a // space between the variable and the to-be-removed initializer. Remove the // leading trivia on the accessor block so we don't double up. - node.bindings[node.bindings.startIndex].accessorBlock = expansion.accessors?.with(\.leadingTrivia, []) - node.bindings[node.bindings.startIndex].initializer = nil + binding.accessorBlock = expansion.accessors?.with(\.leadingTrivia, []) + binding.initializer = nil } else { - node.bindings[node.bindings.startIndex].accessorBlock = expansion.accessors + binding.accessorBlock = expansion.accessors } + + node.bindings = [binding] } return DeclSyntax(node) diff --git a/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift b/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift index 471a34c7684..3b4c2265911 100644 --- a/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift +++ b/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift @@ -44,6 +44,127 @@ fileprivate struct ConstantOneGetter: AccessorMacro { final class AccessorMacroTests: XCTestCase { private let indentationWidth: Trivia = .spaces(2) + func testAccessorOnVariableDeclWithTrailingLineCommentAndNoAccessorBlock() { + assertMacroExpansion( + """ + @constantOne + var x: Int /*1*/ // hello + """, + expandedSource: """ + var x: Int { /*1*/ // hello + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + + assertMacroExpansion( + """ + @constantOne + var x: Int /// hello + """, + expandedSource: """ + var x: Int { /// hello + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + + assertMacroExpansion( + """ + @constantOne + var x: Int = 1 /// hello + """, + expandedSource: """ + var x: Int { /// hello + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + + assertMacroExpansion( + """ + @constantOne + var x /// hello + """, + expandedSource: """ + var x { /// hello + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + } + + func testAccessorOnVariableDeclWithTrailingLineCommentAndAccessorBlock() { + assertMacroExpansion( + """ + @constantOne + var x: Int /*h*/ { // hello + 1 + } + """, + expandedSource: """ + var x: Int /*h*/ { // hello + get { + 1 + } + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + } + + func testAccessorOnVariableDeclWithTrailingCommentsAndSingleLineGetterExpansion() { + struct ConstantOneSingleLineGetter: AccessorMacro { + static let formatMode: FormatMode = .disabled + + static func expansion( + of node: AttributeSyntax, + providingAccessorsOf declaration: some DeclSyntaxProtocol, + in context: some MacroExpansionContext + ) throws -> [AccessorDeclSyntax] { + return [ + """ + get { 1 } + """ + ] + } + } + + assertMacroExpansion( + """ + @constantOne + var x: Int // hello + """, + expandedSource: """ + var x: Int { // hello + get { 1 } + } + """, + macros: ["constantOne": ConstantOneSingleLineGetter.self], + indentationWidth: indentationWidth + ) + } + func testAccessorOnVariableDeclWithExistingGetter() { assertMacroExpansion( """