diff --git a/Examples/Tests/MacroExamples/Implementation/ComplexMacros/DictionaryIndirectionMacroTests.swift b/Examples/Tests/MacroExamples/Implementation/ComplexMacros/DictionaryIndirectionMacroTests.swift index 03191b51563..c98cd461a1e 100644 --- a/Examples/Tests/MacroExamples/Implementation/ComplexMacros/DictionaryIndirectionMacroTests.swift +++ b/Examples/Tests/MacroExamples/Implementation/ComplexMacros/DictionaryIndirectionMacroTests.swift @@ -32,7 +32,7 @@ final class DictionaryStorageMacroTests: XCTestCase { """, expandedSource: """ struct Point { - var x: Int = 1 { + var x: Int { get { _storage["x", default: 1] as! Int } @@ -40,7 +40,7 @@ final class DictionaryStorageMacroTests: XCTestCase { _storage["x"] = newValue } } - var y: Int = 2 { + var y: Int { get { _storage["y", default: 2] as! Int } diff --git a/Examples/Tests/MacroExamples/Implementation/ComplexMacros/ObservableMacroTests.swift b/Examples/Tests/MacroExamples/Implementation/ComplexMacros/ObservableMacroTests.swift index 13706868314..183cbb535e0 100644 --- a/Examples/Tests/MacroExamples/Implementation/ComplexMacros/ObservableMacroTests.swift +++ b/Examples/Tests/MacroExamples/Implementation/ComplexMacros/ObservableMacroTests.swift @@ -77,7 +77,7 @@ final class ObservableMacroTests: XCTestCase { } } - var isHappy: Bool = true { + var isHappy: Bool { get { _registrar.beginAccess(\.isHappy) defer { diff --git a/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift b/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift index 0e4a54a8d6c..39c1f19f599 100644 --- a/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift +++ b/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift @@ -307,7 +307,7 @@ private func expandAccessorMacroWithExistingAccessors( return nil } - // Separate the accessor from any existing accessors by two spaces + // Separate the accessor from any existing accessors by an empty line let indentedSource = "\n" + expanded.indented(by: attachedTo.indentationOfFirstLine + indentationWidth) return "\(raw: indentedSource)" } @@ -702,13 +702,26 @@ private class MacroApplication: SyntaxRewriter { context.addDiagnostics(from: MacroApplicationError.accessorMacroOnVariableWithMultipleBindings, node: node) return DeclSyntax(node) } - node.bindings[node.bindings.startIndex].accessorBlock = expandAccessors(of: node, existingAccessors: binding.accessorBlock) + + let expansion = expandAccessors(of: node, existingAccessors: binding.accessorBlock) + if expansion.accessors != binding.accessorBlock { + 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 + } else { + node.bindings[node.bindings.startIndex].accessorBlock = expansion.accessors + } + } + return DeclSyntax(node) } override func visit(_ node: SubscriptDeclSyntax) -> DeclSyntax { var node = super.visit(node).cast(SubscriptDeclSyntax.self) - node.accessorBlock = expandAccessors(of: node, existingAccessors: node.accessorBlock) + node.accessorBlock = expandAccessors(of: node, existingAccessors: node.accessorBlock).accessors return DeclSyntax(node) } } @@ -869,14 +882,23 @@ extension MacroApplication { } } - /// Expand all 'accessor' macros attached to `storage` and return the `storage` - /// node. + /// Expand all 'accessor' macros attached to `storage`. /// - /// - Returns: The storage node with all macro-synthesized accessors applied. - private func expandAccessors(of storage: some DeclSyntaxProtocol, existingAccessors: AccessorBlockSyntax?) -> AccessorBlockSyntax? { + /// - Returns: The final accessors block that includes both the existing + /// and expanded accessors, as well as whether any `get`/`set` were + /// expanded (in which case any initializer on `storage` should be + /// removed). + private func expandAccessors(of storage: some DeclSyntaxProtocol, existingAccessors: AccessorBlockSyntax?) -> ( + accessors: AccessorBlockSyntax?, expandsGetSet: Bool + ) { let accessorMacros = macroAttributes(attachedTo: DeclSyntax(storage), ofType: AccessorMacro.Type.self) var newAccessorsBlock = existingAccessors + var expandsGetSet = false + func checkExpansions(_ accessors: AccessorDeclListSyntax?) { + guard let accessors else { return } + expandsGetSet = expandsGetSet || accessors.contains(where: \.isGetOrSet) + } for macro in accessorMacros { do { @@ -894,6 +916,8 @@ extension MacroApplication { in: context, indentationWidth: indentationWidth ) { + checkExpansions(newAccessors) + // If existingAccessors is not `nil`, then we also set // `newAccessorBlock` above to a a non-nil value, so // `newAccessorsBlock` also isn’t `nil`. @@ -902,31 +926,33 @@ extension MacroApplication { indentationWidth: self.indentationWidth ) } - } else { - let newAccessors = try expandAccessorMacroWithoutExistingAccessors( - definition: macro.definition, - attributeNode: macro.attributeNode, - attachedTo: DeclSyntax(storage), - in: context, - indentationWidth: indentationWidth - ) - if newAccessorsBlock == nil { - newAccessorsBlock = newAccessors - } else if let newAccessors = newAccessors { - guard case .accessors(let accessorList) = newAccessors.accessors else { - throw MacroApplicationError.malformedAccessor - } - newAccessorsBlock = newAccessorsBlock!.addingAccessors( + } else if let newAccessors = try expandAccessorMacroWithoutExistingAccessors( + definition: macro.definition, + attributeNode: macro.attributeNode, + attachedTo: DeclSyntax(storage), + in: context, + indentationWidth: indentationWidth + ) { + guard case .accessors(let accessorList) = newAccessors.accessors else { + throw MacroApplicationError.malformedAccessor + } + + checkExpansions(accessorList) + + if let oldBlock = newAccessorsBlock { + newAccessorsBlock = oldBlock.addingAccessors( from: accessorList, indentationWidth: self.indentationWidth ) + } else { + newAccessorsBlock = newAccessors } } } catch { context.addDiagnostics(from: error, node: macro.attributeNode) } } - return newAccessorsBlock + return (newAccessorsBlock, expandsGetSet) } } @@ -1130,3 +1156,9 @@ private extension AttributeSyntax { return (detach(in: context, foldingWith: operatorTable) as Syntax).cast(Self.self) } } + +private extension AccessorDeclSyntax { + var isGetOrSet: Bool { + return accessorSpecifier.tokenKind == .keyword(.get) || accessorSpecifier.tokenKind == .keyword(.set) + } +} diff --git a/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift b/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift index 76eecb6c5f1..c283c1f787a 100644 --- a/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift +++ b/Tests/SwiftSyntaxMacroExpansionTest/AccessorMacroTests.swift @@ -320,4 +320,95 @@ final class AccessorMacroTests: XCTestCase { macros: ["Test": TestMacro.self] ) } + + func testInitializerRemovedForGetSet() { + assertMacroExpansion( + """ + @constantOne + var x: Int = 1 + """, + expandedSource: """ + var x: Int { + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + + // Bit of an odd case, compiler has the type but we don't know it in `MacroSystem` + assertMacroExpansion( + """ + @constantOne + var x = 1 + """, + expandedSource: """ + var x { + get { + return 1 + } + } + """, + macros: ["constantOne": ConstantOneGetter.self], + indentationWidth: indentationWidth + ) + } + + func testInitializerRemainsForObserver() { + struct DidSetAdder: AccessorMacro { + static func expansion( + of node: AttributeSyntax, + providingAccessorsOf declaration: some DeclSyntaxProtocol, + in context: some MacroExpansionContext + ) throws -> [AccessorDeclSyntax] { + return [ + """ + didSet { + } + """ + ] + } + } + + assertMacroExpansion( + """ + @addDidSet + var x = 1 + """, + expandedSource: """ + var x = 1 { + didSet { + } + } + """, + macros: ["addDidSet": DidSetAdder.self], + indentationWidth: indentationWidth + ) + + // Invalid semantically, but we shouldn't remove the initializer as the + // macro did not produce a getter/setter + assertMacroExpansion( + """ + @addDidSet + var x = 1 { + get { + return 1 + } + } + """, + expandedSource: """ + var x = 1 { + get { + return 1 + } + didSet { + } + } + """, + macros: ["addDidSet": DidSetAdder.self], + indentationWidth: indentationWidth + ) + } }