Add methods from `RangeReplaceableCollection` to `SyntaxCollection` and remove `add*` methods on syntax nodes that have a collection as child #1958

ahoppen · 2023-07-27T17:06:09Z

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.

With those operators and methods, we no longer need the add* methods on syntax nodes that have a syntax collection as their child.

ahoppen · 2023-07-27T17:06:14Z

@swift-ci Please test

With swiftlang/swift-syntax#1958, `node.filter` will return a new `SyntaxCollection` that has the filtered elements removed (instead of the array it’s currently returning). Since that node has elements removed, it will have a new parent and thus all the nodes inside of it have new IDs. Use `where` after `for` to get the elements with the same IDs and just don’t iterate the elements that don’t satisfy the condition. This is also more performant because it doesn’t create an intermediate array.

ahoppen · 2023-07-27T21:15:42Z

swiftlang/swift-format#575

@swift-ci Please test

ahoppen · 2023-07-27T23:45:33Z

swiftlang/swift-format#575

@swift-ci Please test

ahoppen · 2023-07-28T00:04:04Z

swiftlang/swift-format#575

@swift-ci Please test Windows

With swiftlang/swift-syntax#1958, `node.filter` will return a new `SyntaxCollection` that has the filtered elements removed (instead of the array it’s currently returning). Since that node has elements removed, it will have a new parent and thus all the nodes inside of it have new IDs. Use `where` after `for` to get the elements with the same IDs and just don’t iterate the elements that don’t satisfy the condition. This is also more performant because it doesn’t create an intermediate array.

ahoppen · 2023-07-28T15:06:28Z

@swift-ci Please test

ahoppen · 2023-07-28T15:08:57Z

@swift-ci Please test Windows

bnbarham

My main concern here is that many of the methods list a Big O that isn't true for our implementation. Eg. even something like append ends up doing much more work than you'd otherwise expect (since it is constructing all parents + has to copy the elements into a new raw syntax node).

That's what all the add* are doing today though, so while I don't think this is blocking, perhaps we should add some extra documentation in. There's also the filter case, which we should probably make a custom implementation for.

bnbarham · 2023-07-27T21:38:27Z

Sources/SwiftSyntax/SyntaxCollection.swift

    self.init(Syntax(data))!
  }

+  /// Create an empty ``SyntaCollection`` with no children.


Suggested change

/// Create an empty ``SyntaCollection`` with no children.

/// Create an empty ``SyntaxCollection`` with no children.

ahoppen · 2023-07-28T21:37:52Z

I added an overload of filter and two more to override the methods in RangeReplacableCollection that were calling Self() and thus dropped the parent. Could you take another quick look at them @bnbarham?

ahoppen · 2023-07-28T21:38:06Z

swiftlang/swift-format#575

@swift-ci Please test

bnbarham · 2023-07-28T21:57:39Z

Sources/SwiftSyntax/SyntaxCollection.swift


+  /// Create an empty ``SyntaxCollection`` with no children.
+  public init() {
+    self.init([])


I guess this is the other downside of using RangeReplaceableCollection. This constructor should really never be used and makes it super easy to drop the parent accidentally.

kimdv · 2023-07-29T07:39:31Z

swiftlang/swift-format#575

@swift-ci Please test Windows

ahoppen · 2023-08-01T23:18:15Z

@swift-ci Please test

bnbarham · 2023-08-02T00:38:39Z

Tests/SwiftSyntaxTest/SyntaxCollectionsTests.swift

+    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)


I wouldn't expect this. This is basically prepending and I think I'd want to keep the parent in that case?

testPrependingElement tests this case FWIW

But that’s what it’s testing. after adding the elements, the parent should still be an ArrayExprSyntax. Or am I misunderstanding you?

That's... true 🤔 The comment seems to be suggesting the opposite doesn't it? And then I read the assert as an XCTAssertFalse.

Oh, the comment was outdated…

bnbarham · 2023-08-02T00:41:05Z

Tests/SwiftSyntaxTest/SyntaxCollectionsTests.swift

+    // 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)
+  }


Would be good to test the added functions too (append, insert, replace). Or update the existing ones - they all manipulate the underlying list for the most part.

Added tests

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`.

ahoppen · 2023-08-02T16:38:51Z

@swift-ci Please test

ahoppen · 2023-08-02T17:20:28Z

@swift-ci Please test Windows

bnbarham · 2023-08-02T17:35:40Z

Tests/SwiftSyntaxTest/SyntaxCollectionsTests.swift

-    XCTAssertNotNil(newArrayElementList.child(at: 1))
-    XCTAssert(!newArrayElementList.child(at: 1)!.kind.isSyntaxCollection)
-    XCTAssertEqual("\(newArrayElementList.child(at: 1)!)", "1")
+fileprivate func assertSyntaxCollectionManipulation(


Would it be worth adding a parent in here and then checking we still have a parent in the transformed? Or are there cases where that's not always true?

That’s why I am performing all the transformations on an ArrayExprSyntax instead of the actual collection type ArrayElementListSyntax. That way the substructure test checks that we still have the collection anchored in a parent.

With swiftlang/swift-syntax#1958, `node.filter` will return a new `SyntaxCollection` that has the filtered elements removed (instead of the array it’s currently returning). Since that node has elements removed, it will have a new parent and thus all the nodes inside of it have new IDs. Use `where` after `for` to get the elements with the same IDs and just don’t iterate the elements that don’t satisfy the condition. This is also more performant because it doesn’t create an intermediate array.

ahoppen requested a review from bnbarham July 27, 2023 17:06

kimdv approved these changes Jul 27, 2023

View reviewed changes

ahoppen mentioned this pull request Jul 27, 2023

Use where instead of filter for for loops swiftlang/swift-format#575

Merged

ahoppen force-pushed the ahoppen/syntax-collection-range-replaceable branch from 4635597 to b690ece Compare July 27, 2023 23:45

ahoppen force-pushed the ahoppen/syntax-collection-range-replaceable branch from b690ece to d069a9f Compare July 28, 2023 14:46

bnbarham approved these changes Jul 28, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/syntax-collection-range-replaceable branch from d069a9f to fc54536 Compare July 28, 2023 21:36

bnbarham reviewed Jul 28, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/syntax-collection-range-replaceable branch from fc54536 to e07c1bf Compare August 1, 2023 23:12

ahoppen requested a review from bnbarham August 1, 2023 23:14

Deprecate add* methods on syntax nodes that have a collection as child

e7c7f98

ahoppen force-pushed the ahoppen/syntax-collection-range-replaceable branch from e07c1bf to c827d90 Compare August 1, 2023 23:18

bnbarham reviewed Aug 2, 2023

View reviewed changes

ahoppen force-pushed the ahoppen/syntax-collection-range-replaceable branch from c827d90 to 744275d Compare August 2, 2023 16:38

bnbarham reviewed Aug 2, 2023

View reviewed changes

ahoppen merged commit 250b431 into swiftlang:main Aug 3, 2023

ahoppen deleted the ahoppen/syntax-collection-range-replaceable branch August 3, 2023 00:21

ahoppen mentioned this pull request Aug 7, 2023

FunctionCallExprSyntax.addArgument does not add separating comma between arguments #1937

Closed

allevato mentioned this pull request Aug 9, 2023

Make all syntax collections non-optional #2040

Closed

	/// Create an empty ``SyntaCollection`` with no children.
	/// Create an empty ``SyntaxCollection`` with no children.

Add methods from RangeReplaceableCollection to SyntaxCollection and remove add* methods on syntax nodes that have a collection as child #1958

Add methods from RangeReplaceableCollection to SyntaxCollection and remove add* methods on syntax nodes that have a collection as child #1958

Uh oh!

Conversation

ahoppen commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Jul 27, 2023

Uh oh!

ahoppen commented Jul 27, 2023

Uh oh!

ahoppen commented Jul 27, 2023

Uh oh!

ahoppen commented Jul 28, 2023

Uh oh!

ahoppen commented Jul 28, 2023

Uh oh!

ahoppen commented Jul 28, 2023

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jul 28, 2023

Uh oh!

ahoppen commented Jul 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimdv commented Jul 29, 2023

Uh oh!

ahoppen commented Aug 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Aug 2, 2023

Uh oh!

ahoppen commented Aug 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add methods from `RangeReplaceableCollection` to `SyntaxCollection` and remove `add*` methods on syntax nodes that have a collection as child #1958

Add methods from `RangeReplaceableCollection` to `SyntaxCollection` and remove `add*` methods on syntax nodes that have a collection as child #1958

ahoppen commented Jul 27, 2023 •

edited

Loading