Split keyed(by:) into two overloads

Author: amomchilov

Guides/Keyed.md

@@ -7,7 +7,7 @@ Stores the elements of a sequence as the values of a Dictionary, keyed by the re
 
 ```swift
 let fruits = ["Apple", "Banana", "Cherry"]
-let fruitByLetter = fruits.keyed(by: { $0.first! })
+let fruitByLetter = try! fruits.keyed(by: { $0.first! })
 // Results in:
 // [
 //     "A": "Apple",
@@ -16,7 +16,7 @@ let fruitByLetter = fruits.keyed(by: { $0.first! })
 // ]
 ```
 
-Duplicate keys will trigger a runtime error by default. To handle this, you can provide a closure which specifies which value to keep:
+Duplicate keys throw an error by default. Alternatively, you can provide a closure which specifies which value to keep:
 
 ```swift
 let fruits = ["Apricot", "Banana", "Apple", "Cherry", "Blackberry", "Avocado", "Coconut"]
@@ -34,10 +34,14 @@ let fruitsByLetter = fruits.keyed(
 
 ## Detailed Design
 
-The `keyed(by:)` method is declared as a `Sequence` extension returning `[Key: Element]`.
+The `keyed(by:)` and `keyed(by:uniquingKeysWith:)` methods are declared in an `Sequence` extension, both returning `[Key: Element]`.
 
 ```swift
 extension Sequence {
+    public func keyed<Key>(
+        by keyForValue: (Element) throws -> Key
+    ) throws -> [Key: Element]
+    
     public func keyed<Key>(
         by keyForValue: (Element) throws -> Key,
         uniquingKeysWith combine: ((Key, Element, Element) throws -> Element)? = nil

Sources/Algorithms/Keyed.swift

@@ -9,7 +9,45 @@
 //
 //===----------------------------------------------------------------------===//
 
+public struct KeysAreNotUnique<Key, Element>: Error {
+  public let key: Key
+  public let previousElement: Element
+  public let conflictingElement: Element
+
+  @inlinable
+  public init(key: Key, previousElement: Element, conflictingElement: Element) {
+    self.key = key
+    self.previousElement = previousElement
+    self.conflictingElement = conflictingElement
+  }
+}
+
 extension Sequence {
+  /// Creates a new Dictionary from the elements of `self`, keyed by the
+  /// results returned by the given `keyForValue` closure. Deriving the
+  /// same duplicate key for more than one element of `self` will cause
+  /// an error to be thrown.
+  ///
+  /// - Parameters:
+  ///   - keyForValue: A closure that returns a key for each element in `self`.
+  /// - Throws: `KeysAreNotUnique ` if two values map to the same key (via `keyForValue`).
+  @inlinable
+  public func keyed<Key>(
+    by keyForValue: (Element) throws -> Key
+  ) throws -> [Key: Element] {
+    var result = [Key: Element]()
+
+    for element in self {
+      let key = try keyForValue(element)
+
+      if let previousElement = result.updateValue(element, forKey: key) {
+        throw KeysAreNotUnique(key: key, previousElement: previousElement, conflictingElement: element)
+      }
+    }
+
+    return result
+  }
+
   /// Creates a new Dictionary from the elements of `self`, keyed by the
   /// results returned by the given `keyForValue` closure. As the dictionary is
   /// built, the initializer calls the `combine` closure with the current and
@@ -18,50 +56,30 @@ extension Sequence {
   /// choose between the two values, combine them to produce a new value, or
   /// even throw an error.
   ///
-  /// If no `combine` closure is provided, deriving the same duplicate key for
-  /// more than one element of self results in a runtime error.
-  ///
   /// - Parameters:
-  ///   - keyForValue: A closure that returns a key for each element in
-  ///     `self`.
+  ///   - keyForValue: A closure that returns a key for each element in `self`.
   ///   - combine: A closure that is called with the values for any duplicate
   ///     keys that are encountered. The closure returns the desired value for
   ///     the final dictionary.
   @inlinable
   public func keyed<Key>(
     by keyForValue: (Element) throws -> Key,
-    uniquingKeysWith combine: ((Key, Element, Element) throws -> Element)? = nil
+    uniquingKeysWith combine: (Key, Element, Element) throws -> Element
   ) rethrows -> [Key: Element] {
     var result = [Key: Element]()
 
-    if combine != nil {
-      // We have a `combine` closure. Use it to resolve duplicate keys.
-
-      for element in self {
-        let key = try keyForValue(element)
-        
-        if let oldValue = result.updateValue(element, forKey: key) {
-          // Can't use a conditional binding to unwrap this, because the newly bound variable
-          // doesn't play nice with the `rethrows` system.
-          let valueToKeep = try combine!(key, oldValue, element)
-          
-          // This causes a second look-up for the same key. The standard library can avoid that
-          // by calling `mutatingFind` to get access to the bucket where the value will end up,
-          // and updating in place.
-          // Swift Algorithms doesn't have access to that API, so we make due.
-          // When this gets merged into the standard library, we should optimize this.
-          result[key] = valueToKeep
-        }
-      }
-    } else {
-      // There's no `combine` closure. Duplicate keys are disallowed.
+    for element in self {
+      let key = try keyForValue(element)
 
-      for element in self {
-        let key = try keyForValue(element)
+      if let oldValue = result.updateValue(element, forKey: key) {
+        let valueToKeep = try combine(key, oldValue, element)
 
-        guard result.updateValue(element, forKey: key) == nil else {
-          fatalError("Duplicate values for key: '\(key)'")
-        }
+        // This causes a second look-up for the same key. The standard library can avoid that
+        // by calling `mutatingFind` to get access to the bucket where the value will end up,
+        // and updating in place.
+        // Swift Algorithms doesn't have access to that API, so we make due.
+        // When this gets merged into the standard library, we should optimize this.
+        result[key] = valueToKeep
       }
     }
 

Tests/SwiftAlgorithmsTests/KeyedTests.swift

@@ -16,7 +16,7 @@ final class KeyedTests: XCTestCase {
   private class SampleError: Error {}
 
   func testUniqueKeys() {
-    let d = ["Apple", "Banana", "Cherry"].keyed(by: { $0.first! })
+    let d = try! ["Apple", "Banana", "Cherry"].keyed(by: { $0.first! })
     XCTAssertEqual(d.count, 3)
     XCTAssertEqual(d["A"]!, "Apple")
     XCTAssertEqual(d["B"]!, "Banana")
@@ -25,16 +25,19 @@ final class KeyedTests: XCTestCase {
   }
 
   func testEmpty() {
-    let d = EmptyCollection<String>().keyed(by: { $0.first! })
+    let d = try! EmptyCollection<String>().keyed(by: { $0.first! })
     XCTAssertEqual(d.count, 0)
   }
 
   func testNonUniqueKeys() throws {
-    throw XCTSkip("""
-    TODO: What's the XCTest equivalent to `expectCrashLater()`?
-
-    https://github.com/apple/swift/blob/4d1d8a9de5ebc132a17aee9fc267461facf89bf8/validation-test/stdlib/Dictionary.swift#L1914
-    """)
+    XCTAssertThrowsError(
+      try ["Apple", "Avocado", "Banana", "Cherry"].keyed(by: { $0.first! })
+    ) { thrownError in
+      let e = thrownError as! KeysAreNotUnique<Character, String>
+      XCTAssertEqual(e.key, "A")
+      XCTAssertEqual(e.previousElement, "Apple")
+      XCTAssertEqual(e.conflictingElement, "Avocado")
+    }
   }
 
   func testNonUniqueKeysWithMergeFunction() {