Restrict Shared.elements to IdentifiedArray only. (#3187)

* Restrict Shared.elements to IdentifiedArray only.

* wip

* wip

* wip

* fix test

---------

Co-authored-by: Stephen Celis <stephen@stephencelis.com>

ComposableArchitecture.xcworkspace/xcshareddata/swiftpm/Package.resolved

@@ -104,8 +104,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/pointfreeco/swift-identified-collections",
       "state" : {
-        "revision" : "d533cd18b0b456b106694a9899f917ee595f2666",
-        "version" : "1.0.2"
+        "revision" : "2f5ab6e091dd032b63dacbda052405756010dc3b",
+        "version" : "1.1.0"
       }
     },
     {

ComposableArchitecture.xcworkspace/xcshareddata/xcschemes/ComposableArchitecture.xcscheme

@@ -65,7 +65,7 @@
          <EnvironmentVariable
             key = "CI"
             value = "$(CI)"
-            isEnabled = "YES">
+            isEnabled = "NO">
          </EnvironmentVariable>
       </EnvironmentVariables>
    </LaunchAction>

Makefile

@@ -6,6 +6,8 @@ PLATFORM_TVOS = tvOS Simulator,id=$(call udid_for,tvOS 17.5,TV)
 PLATFORM_VISIONOS = visionOS Simulator,id=$(call udid_for,visionOS 1.2,Vision)
 PLATFORM_WATCHOS = watchOS Simulator,id=$(call udid_for,watchOS 10.5,Watch)
 
+TEST_RUNNER_CI = $(CI)
+
 default: test-all
 
 test-all: test-examples

Package.swift

@@ -25,7 +25,7 @@ let package = Package(
     .package(url: "https://github.com/pointfreeco/swift-concurrency-extras", from: "1.1.0"),
     .package(url: "https://github.com/pointfreeco/swift-custom-dump", from: "1.3.0"),
     .package(url: "https://github.com/pointfreeco/swift-dependencies", from: "1.0.0"),
-    .package(url: "https://github.com/pointfreeco/swift-identified-collections", from: "1.0.0"),
+    .package(url: "https://github.com/pointfreeco/swift-identified-collections", from: "1.1.0"),
     .package(url: "https://github.com/pointfreeco/swiftui-navigation", from: "1.1.0"),
     .package(url: "https://github.com/pointfreeco/xctest-dynamic-overlay", from: "1.1.0"),
   ],

Package@swift-5.9.swift

@@ -27,7 +27,7 @@ let package = Package(
     .package(url: "https://github.com/pointfreeco/swift-concurrency-extras", from: "1.1.0"),
     .package(url: "https://github.com/pointfreeco/swift-custom-dump", from: "1.3.0"),
     .package(url: "https://github.com/pointfreeco/swift-dependencies", from: "1.1.0"),
-    .package(url: "https://github.com/pointfreeco/swift-identified-collections", from: "1.0.0"),
+    .package(url: "https://github.com/pointfreeco/swift-identified-collections", from: "1.1.0"),
     .package(url: "https://github.com/pointfreeco/swift-macro-testing", from: "0.2.0"),
     .package(url: "https://github.com/pointfreeco/swift-perception", from: "1.1.7"),
     .package(url: "https://github.com/pointfreeco/swiftui-navigation", from: "1.1.0"),

Sources/ComposableArchitecture/Internal/DefaultSubscript.swift

@@ -23,7 +23,8 @@ extension Optional {
 
 extension RandomAccessCollection where Self: MutableCollection {
   subscript(
-    position: Index, default defaultSubscript: DefaultSubscript<Element>
+    position: Index,
+    default defaultSubscript: DefaultSubscript<Element>
   ) -> Element {
     get { self.indices.contains(position) ? self[position] : defaultSubscript.value }
     set {
@@ -32,3 +33,16 @@ extension RandomAccessCollection where Self: MutableCollection {
     }
   }
 }
+
+extension _MutableIdentifiedCollection {
+  subscript(
+    id id: ID,
+    default defaultSubscript: DefaultSubscript<Element>
+  ) -> Element {
+    get { self[id: id] ?? defaultSubscript.value }
+    set {
+      defaultSubscript.value = newValue
+      self[id: id] = newValue
+    }
+  }
+}

Sources/ComposableArchitecture/SharedState/Shared.swift

@@ -323,7 +323,7 @@ extension Shared: _CustomDiffObject {
 }
 
 extension Shared
-where Value: RandomAccessCollection & MutableCollection, Value.Index: Hashable & Sendable {
+where Value: _MutableIdentifiedCollection {
   /// Allows a `ForEach` view to transform a shared collection into shared elements.
   ///
   /// ```swift
@@ -349,8 +349,8 @@ where Value: RandomAccessCollection & MutableCollection, Value.Index: Hashable &
   /// > you need to derive a shared element from a shared collection, use a stable lookup, instead,
   /// > like the `$array[id:]` subscript on `IdentifiedArray`.
   public var elements: some RandomAccessCollection<Shared<Value.Element>> {
-    zip(self.wrappedValue.indices, self.wrappedValue).lazy.map { index, element in
-      self[index, default: DefaultSubscript(element)]
+    zip(self.wrappedValue.ids, self.wrappedValue).lazy.map { id, element in
+      self[id: id, default: DefaultSubscript(element)]
     }
   }
 }

Tests/ComposableArchitectureTests/RuntimeWarningTests.swift

@@ -104,79 +104,76 @@
       .value
     }
 
-    #if os(macOS)
-      @MainActor
-      func testEffectEmitMainThread() async throws {
-        try XCTSkipIf(ProcessInfo.processInfo.environment["CI"] != nil)
-        XCTExpectFailure {
-          [
-            """
-            An effect completed on a non-main thread. …
-
-              Effect returned from:
-                RuntimeWarningTests.Action.response
-
-            Make sure to use ".receive(on:)" on any effects that execute on background threads to \
-            receive their output on the main thread.
-
-            The "Store" class is not thread-safe, and so all interactions with an instance of \
-            "Store" (including all of its scopes and derived view stores) must be done on the main \
-            thread.
-            """,
-            """
-            An effect completed on a non-main thread. …
-
-              Effect returned from:
-                RuntimeWarningTests.Action.tap
-
-            Make sure to use ".receive(on:)" on any effects that execute on background threads to \
-            receive their output on the main thread.
-
-            The "Store" class is not thread-safe, and so all interactions with an instance of \
-            "Store" (including all of its scopes and derived view stores) must be done on the main \
-            thread.
-            """,
-            """
-            An effect published an action on a non-main thread. …
-
-              Effect published:
-                RuntimeWarningTests.Action.response
-
-              Effect returned from:
-                RuntimeWarningTests.Action.tap
-
-            Make sure to use ".receive(on:)" on any effects that execute on background threads to \
-            receive their output on the main thread.
-
-            The "Store" class is not thread-safe, and so all interactions with an instance of \
-            "Store" (including all of its scopes and derived view stores) must be done on the main \
-            thread.
-            """,
-          ]
+    @MainActor
+    func testEffectEmitMainThread() async throws {
+      XCTExpectFailure {
+        [
+          """
+          An effect completed on a non-main thread. …
+
+            Effect returned from:
+              RuntimeWarningTests.Action.response
+
+          Make sure to use ".receive(on:)" on any effects that execute on background threads to \
+          receive their output on the main thread.
+
+          The "Store" class is not thread-safe, and so all interactions with an instance of \
+          "Store" (including all of its scopes and derived view stores) must be done on the main \
+          thread.
+          """,
+          """
+          An effect completed on a non-main thread. …
+
+            Effect returned from:
+              RuntimeWarningTests.Action.tap
+
+          Make sure to use ".receive(on:)" on any effects that execute on background threads to \
+          receive their output on the main thread.
+
+          The "Store" class is not thread-safe, and so all interactions with an instance of \
+          "Store" (including all of its scopes and derived view stores) must be done on the main \
+          thread.
+          """,
+          """
+          An effect published an action on a non-main thread. …
+
+            Effect published:
+              RuntimeWarningTests.Action.response
+
+            Effect returned from:
+              RuntimeWarningTests.Action.tap
+
+          Make sure to use ".receive(on:)" on any effects that execute on background threads to \
+          receive their output on the main thread.
+
+          The "Store" class is not thread-safe, and so all interactions with an instance of \
+          "Store" (including all of its scopes and derived view stores) must be done on the main \
+          thread.
+          """,
+        ]
           .contains($0.compactDescription)
-        }
+      }
 
-        enum Action { case tap, response }
-        let store = Store(initialState: 0) {
-          Reduce<Int, Action> { state, action in
-            switch action {
-            case .tap:
-              return .publisher {
+      enum Action { case tap, response }
+      let store = Store(initialState: 0) {
+        Reduce<Int, Action> { state, action in
+          switch action {
+          case .tap:
+            return .publisher {
+              Deferred {
                 Future { callback in
-                  Thread.detachNewThread {
-                    XCTAssertFalse(Thread.isMainThread, "Effect should send on non-main thread.")
-                    callback(.success(.response))
-                  }
+                  callback(.success(.response))
                 }
               }
-            case .response:
-              return .none
+              .receive(on: DispatchQueue(label: "background"))
             }
+          case .response:
+            return .none
           }
         }
-        await ViewStore(store, observe: { $0 }).send(.tap).finish()
       }
-    #endif
+      await ViewStore(store, observe: { $0 }).send(.tap).finish()
+    }
 
     @MainActor
     func testBindingUnhandledAction() {

Tests/ComposableArchitectureTests/SharedTests.swift

@@ -1,5 +1,6 @@
 import Combine
 @_spi(Internals) import ComposableArchitecture
+import CustomDump
 import XCTest
 
 final class SharedTests: XCTestCase {
@@ -947,6 +948,52 @@ final class SharedTests: XCTestCase {
 
     XCTAssertNil(optionalValueWithDefault)
   }
+
+  func testElements() {
+    struct User: Equatable, Identifiable {
+      let id: Int
+      var name = ""
+    }
+    let sharedCollection = Shared([User(id: 1), User(id: 2)] as IdentifiedArrayOf<User>)
+    let elements = sharedCollection.elements
+    let first = elements.first!
+    let second = elements.last!
+
+    first.wrappedValue.name = "Blob"
+    second.wrappedValue.name = "Blob Jr"
+    XCTAssertNoDifference(first.wrappedValue, User(id: 1, name: "Blob"))
+    XCTAssertNoDifference(second.wrappedValue, User(id: 2, name: "Blob Jr"))
+    XCTAssertNoDifference(
+      sharedCollection.wrappedValue,
+      [
+        User(id: 1, name: "Blob"),
+        User(id: 2, name: "Blob Jr"),
+      ]
+    )
+
+    sharedCollection.wrappedValue.swapAt(0, 1)
+    XCTAssertNoDifference(first.wrappedValue, User(id: 1, name: "Blob"))
+    XCTAssertNoDifference(second.wrappedValue, User(id: 2, name: "Blob Jr"))
+    XCTAssertNoDifference(
+      sharedCollection.wrappedValue,
+      [
+        User(id: 2, name: "Blob Jr"),
+        User(id: 1, name: "Blob"),
+      ]
+    )
+
+    first.wrappedValue.name += ", M.D."
+    second.wrappedValue.name += ", Esq."
+    XCTAssertNoDifference(first.wrappedValue, User(id: 1, name: "Blob, M.D."))
+    XCTAssertNoDifference(second.wrappedValue, User(id: 2, name: "Blob Jr, Esq."))
+    XCTAssertNoDifference(
+      sharedCollection.wrappedValue,
+      [
+        User(id: 2, name: "Blob Jr, Esq."),
+        User(id: 1, name: "Blob, M.D."),
+      ]
+    )
+  }
 }
 
 @Reducer