apple / swift-algorithms

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

Indexing a ChunksOfCountCollection can crash with "Index out of bounds" #178

Closed rudedogdhc closed 2 years ago

rudedogdhc commented 2 years ago

Replace this paragraph with a short description of the incorrect incorrect behavior. If this is a regression, please note the last version that the behavior was correct in addition to your current version.

Swift Algorithms version: main branch

Swift version:

swift-driver version: 1.26.21 Apple Swift version 5.5.2 (swiftlang-1300.0.47.5 clang-1300.0.29.30)
Target: arm64-apple-macosx12.0

Checklist

Steps to Reproduce

The following code crashes:

import Algorithms
import Foundation

let input = "A|1|B|2"
let keyPairs = input
  .split(separator: "|", omittingEmptySubsequences: false)
  .chunks(ofCount: 2)

let aValue = keyPairs.first(where: {$0.first == "A"})
//  .flatMap { Array($0) }
  .flatMap { $0.count == 2 ? $0[1] : nil }

let bValue = keyPairs.first(where: {$0.first == "B"})
//  .flatMap { Array($0) }  // <--- uncomment this and it no longer crashes
  .flatMap { $0.count == 2 ? $0[1] : nil }   // <--- crash here with "Fatal error: Index out of bounds"

print("A: \(aValue ?? "n/a"); B: \(bValue ?? "n/a")")

Expected behavior

It shouldn't crash. Each chunk has a count of 2 so accessing a chunk with index 1 should not crash.

Actual behavior

It crashes. If I uncomment the conversion back to Array it works fine. Based on some testing, the crash appears to happen with every sub-chunk after the first one.

xwu commented 2 years ago

This is the expected behavior, if I’m understanding the point correctly.

Not all collection indices start at 0, and indeed they don’t need to have indices of type Int. However, when you obtain a slice of a collection (as in this case for chunks), that slice shares indices with its parent. When you attempt to access the element of a slice at index 1, this is supposed to be out of bounds if the slice doesn’t include the parent collection’s element that’s at index 1.

There isn’t anything specific to ChunksOfCountCollection but rather common to all of Swift. Understanding this point is key to using indices correctly.

rudedogdhc commented 2 years ago

You are correct and I've learned something new about Swift. Using $0.count == 2 ? $0[$0.startIndex + 1] : nil is the proper way to do this.

mdznr commented 2 years ago

Using $0.count == 2 ? $0[$0.startIndex + 1] : nil is the proper way to do this.

index(_:offsetBy:) exists to do this. This would work for indices that aren’t Int and those that might not count by 1s.