apple / swift-algorithms

Commonly used sequence and collection algorithms for Swift
Apache License 2.0
5.81k stars 425 forks source link

Array.adjacentPairs() is incorrect #216

Closed domkm closed 7 months ago

domkm commented 8 months ago

Swift Algorithms version: 1.2.0 Swift version: swift-driver version: 1.87.1 Apple Swift version 5.9 (swiftlang-5.9.0.128.108 clang-1500.0.40.1) Target: arm64-apple-macosx14.0

Checklist

Steps to Reproduce

let pairs = [0, 1, 2].adjacentPairs().map{ $0 }
XCTAssertEqualSequences(pairs, [(0, 1), (1, 2)], by: ==) //failed - element (0, 2) on first sequence does not match element (1, 2) on second sequence at position 1

Expected behavior

[0, 1, 2].adjacentPairs() should return [(0, 1), (1,2)]

Actual behavior

[0, 1, 2].adjacentPairs() actually returns [(0, 1), (0, 2), (1, 2)]

LucianoPAlmeida commented 8 months ago

Couldn't repro this, did this have another detail for the repro?

domkm commented 8 months ago

Odd. It's still happening for me. I copied AdjacentPairsTests.swift and TestUtilities.swift into my project and updated the testThreeElementSequence() test.

  func testThreeElementSequence() {
    let pairs = (0...).prefix(3).adjacentPairs()
    XCTAssertEqualSequences(pairs, [(0, 1), (1, 2)], by: ==)
+   let pairs2 = [0, 1, 2].adjacentPairs().map {x in x}
+   XCTAssertEqualSequences(pairs2, [(0, 1), (1, 2)], by: ==)
  }

The second assertion fails:

Screenshot 2023-11-18 at 7 53 24 PM

Perhaps related, [0, 1, 2].adjacentPairs() without the .map {x in x} fails to type check with Ambiguous use of 'adjacentPairs()'.

Any suggestions?

LucianoPAlmeida commented 7 months ago

Wait, do the "ambiguos" diagnostic notes point to the candidates it found, it looks like maybe you would have to versions of "adjacentPairs" and the one being pick for array is incorrect. You can also click in Xcode and see where it takes you.

domkm commented 7 months ago

You're right! I'm new to Swift but should have checked this first anyway, sorry. It looks like SwifterSwift defines a different version of adjacentPairs. I haven't yet narrowed down how the two implementations are causing incorrect results though.

LucianoPAlmeida commented 7 months ago

Coincidently I happen to be a maintainer of SwifterSwift so I can take a look on the implementation there and see if this is indeed a bug there. Thank you for the report :)

jshier commented 7 months ago

Seems to be the intended behavior:

/// SwifterSwift: Unique pair of elements in a collection.
///
///        let array = [1, 2, 3]
///        for (first, second) in array.adjacentPairs() {
///            print(first, second) // print: (1, 2) (1, 3) (2, 3)
///        }
///
///

Seems to be what swift-algorithms calls permutations.

LucianoPAlmeida commented 7 months ago

Seems to be what swift-algorithms calls permutations.

Yeah, this is actually something I've missed on review