Add helper function for getting the source edits of two strings

[StevenWong12]

This change should make it easier to test two different source by getting SourceEdit with the helper function

[ahoppen]

Thanks, this seems like a good helper function to write incremental parsing tests. I’ve got a few comments inline.

[ahoppen]

One thing you could also test here is that applyEdits(diffs, concurrent: false, s1) is something close to s2 but with a few questions marks of where characters are inserted.

[StevenWong12]

I think applyEdits is written in SwiftSyntaxTest and we're in SwiftParserTest here so I could not directly call it. If that's necessary I could move it to elsewhere.

[ahoppen]

Actually, now that I think about it, it would make sense to move getConcurrentEdits to _SwiftSyntaxTestSupport and move all of your new tests in Tests/SwiftSyntaxTestSupportTests because they are really testing that this helper function actually works. You can then also move applyEdits to _SwiftSyntaxTestSupport and use it from your new tests.

[ahoppen]

I’d make this function fileprivate because usually I don’t like it if test cases suddenly start relying on helper functions from other files. If we need this in multiple test files (which we don’t right now), I usually prefer to move it to a standalone file.

[StevenWong12]

Since I moved getConcurrentEdits and applyEdits to a standalone file, I made both of them public to be called in other test files.

[ahoppen]

The tests in lit_tests/incrParse are still using these python files, so we can’t delete them just yet. If you want to run them locally, you can do so as described here. You just need to replace build by test (and I think you might need to build the compiler to get the FileCheck executable to verify the files, but maybe there’s some other way of getting it, eg. through Homebrew, I’m not sure)

https://github.com/apple/swift-syntax/blob/94959f0693610c3f51fc8823c2f38bb00b064c26/CONTRIBUTING.md?plain=1#L10-L21

[StevenWong12]

Oh, I see. Maybe I could translate those tests in IncrementalParsingTests later when I added some functionality related to them.

[ahoppen]

Actually, now that I think about it, it would make sense to move getConcurrentEdits to _SwiftSyntaxTestSupport and move all of your new tests in Tests/SwiftSyntaxTestSupportTests because they are really testing that this helper function actually works. You can then also move applyEdits to _SwiftSyntaxTestSupport and use it from your new tests.

[ahoppen]

Could you add one more test case that has an insertion at the start and an insertion at the end. That would make sure that Collection.difference actually contains sequential edits instead of concurrent edits.

let base = "0123"
let new = "a0123b"

[ahoppen]

Sorry if I wasn’t clear here. What I was looking for is a test case just like the one you have in testDiffOfTwoStrings (which I think has a very good test structure), just with the inputs I gave you. So basically:

  public func testStringDifferenceReturnSequentialEdits() throws {
    let base = "0123"
    let new = "a0123bc"

    let diffs = getConcurrentEdits(from: base, to: new)
    XCTAssertEqual(diffs.edits, [
        SourceEdit(offset: 0, length: 0, replacementLength: 1),
        SourceEdit(offset: 4, length: 0, replacementLength: 2)
      ])

    let appliedDiffsBase = applyEdits(diffs.edits, concurrent: true, to: base)

    XCTAssertEqual(appliedDiffsBase, "?0123??")
  }

Which seems to pass, so that’s good ✅

[StevenWong12]

Great. Already added this.

[ahoppen]

I just tried running some random tests on the getConcurrentEdits and it looks like there are still some bugs in it.

For example base = "0a1ab" and new = "bb01d" seems to produce incorrect edits. The following test fails

let base = "0a1ab"
let new =  "bb01d"

let diffs = getConcurrentEdits(from: base, to: new)

let appliedDiffsBase = applyEdits(diffs.edits, concurrent: true, to: base)

XCTAssertEqual(appliedDiffsBase, "??aa?")

---

The automated test that I used to find this failure was the following (very hacky, the code itself is not ready to be submitted to the repo)

func randomString(length: Int) -> String {
  let letters = "abcd"
  return String((0..<length).map{ _ in letters.randomElement()! })
}

public func testFoo() {
  for i in 0..<100_000 {
    let startLength = Int.random(in: 0..<6)
    let endLength = Int.random(in: 0..<6)

    let base = randomString(length: startLength)
    let new = randomString(length: endLength)

    let edits = getConcurrentEdits(from: base, to: new)
    let editsApplied = applyEdits(edits.edits, concurrent: true, to: base)

    assert(new.count == editsApplied.count)
    for (n, e) in zip(new, editsApplied) {
      assert(e == "?" || n == e)
    }
  }
}

[StevenWong12]

I found String.Difference sometimes produces some unexpected outputs. e.g.

"class".difference(from: "struct")

The function seems to center on the c in "struct" and view other characters as deletions and "lass" as insertions, which is not we expect.

I opened another draft pr to use markers to mark source edits and generate original and edited source (which is a bit like the previous one in the old parser), I think this should be easier to mark and test, also avoid being trapped in the underlying logic of String.Difference. Maybe we could close this pr and turn to that one. What do you think?

[ahoppen]

Thanks for catching that. With that explanation, I agree that being explicit about the edits makes a lot of sense. 👍🏽 So, I prefer #1684

[StevenWong12]

Cool. Let's just close this and continue in the other pr.