Add methods from RangeReplaceableCollection to SyntaxCollection

Especially the `+` operators make manipulation of `SyntaxCollection` a lot easier.

`SyntaxCollection` doesn’t conform to `RangeReplaceableCollection` because of two reasons:
 - `RangeReplaceableCollection` assumes that creating a new colleciton and adding all elements from another collection to it results in the same collection. This is not true for `SyntaxCollection` because the new collection wouldn’t have a parent. This causes eg. the default implementation of `filter` to misbehave.
 - It can’t make the complexity guarantees that `RangeReplaceableCollection` requires, e.g. `append` is `O(tree depth)` instead of `O(1)`.

For the same reason, also remove the conformance to `MutableCollection`.

Author: ahoppen

Sources/SwiftSyntax/SyntaxCollection.swift

@@ -10,7 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-public protocol SyntaxCollection: SyntaxProtocol, BidirectionalCollection, MutableCollection where Element: SyntaxProtocol {
+public protocol SyntaxCollection: SyntaxProtocol, BidirectionalCollection where Element: SyntaxProtocol, Index == SyntaxChildrenIndex {
   associatedtype Iterator = SyntaxCollectionIterator<Element>
 
   /// The ``SyntaxKind`` of the syntax node that conforms to ``SyntaxCollection``.
@@ -33,6 +33,17 @@ extension SyntaxCollection {
     self.init(Syntax(data))!
   }
 
+  /// Initialize the collection from a collection of the same type.
+  ///
+  /// - Note: This initializer is needed to improve source compatibility after
+  ///   the `+` operators have been added. Previously, they created an array
+  ///   that was converted to a ``SyntaxCollection`` and now they create a
+  ///   ``SyntaxCollection``, which makes the initializer a no-op.
+  @available(*, deprecated, message: "Call to initializer is not necessary.")
+  public init(_ collection: Self) {
+    self = collection
+  }
+
   public init<Children: Sequence>(_ children: Children) where Children.Element == Element {
     let arena = SyntaxArena()
     // Extend the lifetime of children so their arenas don't get destroyed
@@ -208,6 +219,170 @@ public struct SyntaxCollectionIterator<E: SyntaxProtocol>: IteratorProtocol {
   }
 }
 
+// MARK: Functions analogous to RangeReplaceableCollection
+
+/// Defines functions that are similar to those in `RangeReplaceableCollection`.
+///
+/// SyntaxCollection doesn’t conform to `RangeReplaceableCollection` because of
+/// two reasons:
+///  - `RangeReplaceableCollection` assumes that creating a new colleciton and
+///    adding all elements from another collection to it results in the same
+///    collection. This is not true for `SyntaxCollection` because the new
+///    collection wouldn’t have a parent. This causes eg. the default
+///    implementation of `filter` to misbehave.
+///  - It can’t make the complexity guarantees that `RangeReplaceableCollection`
+///    requires, e.g. `append` is `O(tree depth)` instead of `O(1)`.
+extension SyntaxCollection {
+  /// Replace the nodes in `subrange` by `newElements`.
+  public mutating func replaceSubrange(_ subrange: Range<Self.Index>, with newElements: some Collection<Element>) {
+    // We only access the raw nodes of `newElements` below.
+    // Keep `newElements` alive so their arena doesn't get deallocated.
+    withExtendedLifetime(newElements) {
+      var newLayout = layoutView.formLayoutArray()
+      let layoutRangeLowerBound = (subrange.lowerBound.data?.indexInParent).map(Int.init) ?? newLayout.endIndex
+      let layoutRangeUpperBound = (subrange.upperBound.data?.indexInParent).map(Int.init) ?? newLayout.endIndex
+      newLayout.replaceSubrange(layoutRangeLowerBound..<layoutRangeUpperBound, with: newElements.map { $0.raw })
+      self = replacingLayout(newLayout)
+    }
+  }
+
+  /// Adds an element to the end of the collection.
+  ///
+  /// - Parameter newElement: The element to append to the collection.
+  public mutating func append(_ newElement: Element) {
+    insert(newElement, at: endIndex)
+  }
+
+  /// Adds the elements of a sequence to the end of this collection.
+  ///
+  /// - Parameter newElements: The elements to append to the collection.
+  public mutating func append(contentsOf newElements: some Sequence<Element>) {
+    insert(contentsOf: Array(newElements), at: endIndex)
+  }
+
+  /// Inserts a new element into the collection at the specified position.
+  ///
+  /// - Parameter newElement: The new element to insert into the collection.
+  /// - Parameter i: The position at which to insert the new element.
+  ///   `index` must be a valid index into the collection.
+  public mutating func insert(_ newElement: Element, at i: Index) {
+    replaceSubrange(i..<i, with: CollectionOfOne(newElement))
+  }
+
+  /// Inserts the elements of a sequence into the collection at the specified
+  /// position.
+  ///
+  /// - Parameter newElements: The new elements to insert into the collection.
+  /// - Parameter i: The position at which to insert the new elements. `index`
+  ///   must be a valid index of the collection.
+  public mutating func insert(contentsOf newElements: some Collection<Element>, at i: Index) {
+    replaceSubrange(i..<i, with: newElements)
+  }
+
+  /// Removes and returns the element at the specified position.
+  ///
+  /// - Parameter position: The position of the element to remove. `position`
+  ///   must be a valid index of the collection that is not equal to the
+  ///   collection's end index.
+  /// - Returns: The removed element.
+  @discardableResult
+  public mutating func remove(at position: Index) -> Element {
+    precondition(!isEmpty, "Can't remove from an empty collection")
+    let result: Element = self[position]
+    replaceSubrange(position..<index(after: position), with: EmptyCollection())
+    return result
+  }
+
+  /// Removes the elements in the specified subrange from the collection.
+  ///
+  /// - Parameter bounds: The range of the collection to be removed. The
+  ///   bounds of the range must be valid indices of the collection.
+  public mutating func removeSubrange(_ bounds: Range<Index>) {
+    replaceSubrange(bounds, with: EmptyCollection())
+  }
+
+  /// Creates a new collection by concatenating the elements of a collection and
+  /// a sequence.
+  ///
+  /// - Parameters:
+  ///   - lhs: A ``SyntaxCollection``
+  ///   - rhs: A collection or finite sequence.
+  public static func + (lhs: Self, rhs: some Sequence<Element>) -> Self {
+    var result = lhs
+    result.append(contentsOf: rhs)
+    return result
+  }
+
+  /// Creates a new collection by concatenating the elements of a collection and
+  /// a sequence.
+  ///
+  /// - Parameters:
+  ///   - lhs: A ``SyntaxCollection``
+  ///   - rhs: A collection or finite sequence.
+  ///
+  /// - Note: This overload exists to resolve when adding an array to a ``SyntaxCollection``.
+  public static func + (lhs: Self, rhs: some RangeReplaceableCollection<Element>) -> Self {
+    var result = lhs
+    result.append(contentsOf: rhs)
+    return result
+  }
+
+  /// Creates a new collection by concatenating the elements of a sequence and a
+  /// collection.
+  ///
+  /// - Parameters:
+  ///   - lhs: A collection or finite sequence.
+  ///   - rhs: A range-replaceable collection.
+  public static func + (lhs: some Sequence<Element>, rhs: Self) -> Self {
+    var result = rhs
+    result.insert(contentsOf: Array(lhs), at: result.startIndex)
+    return result
+  }
+
+  /// Creates a new collection by concatenating the elements of a sequence and a
+  /// collection.
+  ///
+  /// - Parameters:
+  ///   - lhs: A collection or finite sequence.
+  ///   - rhs: A range-replaceable collection.
+  ///
+  /// - Note: This overload exists to resolve when adding an array to a ``SyntaxCollection``.
+  public static func + (lhs: some RangeReplaceableCollection<Element>, rhs: Self) -> Self {
+    var result = rhs
+    result.insert(contentsOf: Array(lhs), at: result.startIndex)
+    return result
+  }
+
+  /// Appends the elements of a sequence to a range-replaceable collection.
+  ///
+  /// - Parameters:
+  ///   - lhs: The ``SyntaxCollection`` to append to.
+  ///   - rhs: A collection or finite sequence.
+  ///
+  /// - Note: This will result in an allocation of a copy of this node.
+  public static func += (lhs: inout Self, rhs: some Sequence<Element>) {
+    lhs.append(contentsOf: rhs)
+  }
+
+  /// Returns a new ``SyntaxCollection`` that just contains the elements
+  /// satisfying the given predicate.
+  ///
+  /// - Parameter isIncluded: A closure that takes an element of the
+  ///   collection as its argument and returns a Boolean value indicating
+  ///   whether the element should be included in the returned collection.
+  /// - Returns: A ``SyntaxCollection`` of the elements that `isIncluded` allowed.
+  ///
+  /// - Note: This creates a new ``SyntaxCollection`` node. If the resulting node
+  ///   is not needed (e.g. because it’s only being iterated), convert the
+  ///   ``SyntaxCollection`` to an array first or use the `where` clause in
+  ///   a `for` statement.
+  public func filter(_ isIncluded: (Element) throws -> Bool) rethrows -> Self {
+    var result = self
+    result.replaceSubrange(self.startIndex..<self.endIndex, with: try Array(self).filter(isIncluded))
+    return result
+  }
+}
+
 /// Conformance to `BidirectionalCollection`.
 extension SyntaxCollection {
   public func makeIterator() -> SyntaxCollectionIterator<Element> {

Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift

@@ -461,13 +461,9 @@ extension MacroApplication {
       }
     }
 
-    // FIXME: Is there a better way to add N members to a decl?
-    return decl.with(
-      \.memberBlock,
-      newMembers.reduce(decl.memberBlock) { partialMembers, newMember in
-        partialMembers.addMember(.init(decl: newMember))
-      }
-    )
+    var decl = decl
+    decl.memberBlock.members += newMembers.map { MemberBlockItemSyntax(decl: $0) }
+    return decl
   }
 
   private func expandAttributes(
@@ -497,7 +493,7 @@ extension MacroApplication {
     }
 
     let newAttributes = attributes.reduce(attributedDecl.attributes ?? .init([])) {
-      AttributeListSyntax($0 + [AttributeListSyntax.Element($1)])
+      $0 + [AttributeListSyntax.Element($1)]
     }
 
     let newDecl = attributedDecl.with(\.attributes, newAttributes).as(DeclSyntax.self)!

Tests/SwiftSyntaxMacroExpansionTest/MacroSystemTests.swift

@@ -362,9 +362,7 @@ public struct AddCompletionHandler: PeerMacro {
         ]
       newParameterList = FunctionParameterListSyntax(newParameterListElements)
     } else {
-      newParameterList = FunctionParameterListSyntax(
-        parameterList + [completionHandlerParam]
-      )
+      newParameterList = parameterList + [completionHandlerParam]
     }
 
     let callArguments: [String] = parameterList.map { param in

Tests/SwiftSyntaxTest/SyntaxCollectionsTests.swift

@@ -28,7 +28,7 @@ public class SyntaxCollectionsTests: XCTestCase {
       integerLiteralElement(0)
     ])
 
-    let newArrayElementList = ArrayElementListSyntax(arrayElementList + [integerLiteralElement(1)])
+    let newArrayElementList = arrayElementList + [integerLiteralElement(1)]
     XCTAssert(newArrayElementList.kind.isSyntaxCollection)
     XCTAssertEqual(newArrayElementList.count, 2)
     XCTAssertNotNil(newArrayElementList.child(at: 1))
@@ -41,13 +41,13 @@ public class SyntaxCollectionsTests: XCTestCase {
       integerLiteralElement(1)
     ])
 
-    var newArrayElementList = ArrayElementListSyntax([integerLiteralElement(0)] + arrayElementList)
+    var newArrayElementList = [integerLiteralElement(0)] + arrayElementList
 
     XCTAssertEqual(newArrayElementList.count, 2)
     XCTAssertNotNil(newArrayElementList.child(at: 0))
     XCTAssertEqual("\(newArrayElementList.child(at: 0)!)", "0")
 
-    newArrayElementList = ArrayElementListSyntax(newArrayElementList + [integerLiteralElement(2)])
+    newArrayElementList += [integerLiteralElement(2)]
 
     XCTAssertEqual(newArrayElementList.count, 3)
     XCTAssertNotNil(newArrayElementList.child(at: 2))
@@ -59,7 +59,7 @@ public class SyntaxCollectionsTests: XCTestCase {
       integerLiteralElement(1)
     ])
 
-    let newArrayElementList = ArrayElementListSyntax([integerLiteralElement(0)] + arrayElementList)
+    let newArrayElementList = [integerLiteralElement(0)] + arrayElementList
 
     XCTAssertEqual(newArrayElementList.count, 2)
     XCTAssertNotNil(newArrayElementList.child(at: 0))
@@ -146,4 +146,32 @@ public class SyntaxCollectionsTests: XCTestCase {
     XCTAssertEqual("\(relems[1])", "1")
     XCTAssertEqual("\(relems[0])", "2")
   }
+
+  public func testFilter() {
+    let elements = ArrayElementListSyntax([
+      integerLiteralElement(0),
+      integerLiteralElement(1),
+      integerLiteralElement(2),
+    ])
+    let array = ArrayExprSyntax(elements: elements)
+
+    let filteredElements = array.elements.filter { _ in true }
+    // The filtered sequence should have the `ArrayExprSyntax` as a parent
+    XCTAssert(filteredElements.parent?.is(ArrayExprSyntax.self) ?? false)
+  }
+
+  public func testAddingSequence() {
+    let elements = ArrayElementListSyntax([
+      integerLiteralElement(0),
+      integerLiteralElement(1),
+      integerLiteralElement(2),
+    ])
+    let array = ArrayExprSyntax(elements: elements)
+
+    let addedElements: ArrayElementListSyntax = Array([integerLiteralElement(4)]) + array.elements
+    XCTAssertEqual(addedElements.count, 4)
+    // If the first collection of + doesn’t have a parent, it doesn’t really
+    // make sense to inherit the parent from the second collection.
+    XCTAssert(addedElements.parent?.is(ArrayExprSyntax.self) ?? false)
+  }
 }