Double Nested ForEachStore removal Bug

[ryantstone]

I have bug that I'm not sure if it's my implementation or CA. In the original app I found it I have a list of textfields that can have a carousel of images attached to them, as the user adds images they are added to the carousel. But they can also be removed, when removing the last item from the carousel it seems like there is an action that is still in the system looking for a now missing object in a ForEachStore Below is a minimum reproduction repo.

https://github.com/ryantstone/LoopTest

[stephencelis]

Dang. This is related to a bug in SwiftUI that we reported awhile ago but hasn't been fixed. The short of it is if you check an observable object's substate (whether a property is nil or whether the size of an array is correct) and then force-unwrap an element that exists or index an array's element that should exist, the latter logic can evaluate for downstream subviews even when upstream state should no longer allow it...like if the non-nil state goes nil, or if the array loses its last element.

For state we worked around it in ForEachStore using this check:

swift-composable-architecture/Sources/ComposableArchitecture/SwiftUI/ForEachStore.swift

Line 44 in 1ae2d86

state: { index < $0.endIndex ? $0[index] : element },

But it looks like we have some more edge cases to work around. Will dive in a bit tomorrow to gain a better understanding. Hope Apple has some fixes in SwiftUI coming soon! 😅

[mbrandonw]

Stephen and I just discussed this, and the reason this is happening is a little simpler than the SwiftUI bug (though that bug is still out there).

It's happening because you have combined your reducers to first run the .remove logic and then run the forEach logic. This means that by the time the forEach logic runs the item has been removed, and hence the crash.

You can flip them to get it working:

let contentViewReducer = Reducer<ContentView.State, ContentView.Actions, Void>.combine(
  helloViewReducer.forEach(
    state: \ContentView.State.hellos,
    action: /ContentView.Actions.didTapMenuButton(index:word:),
    environment: { $0 }
  ),
  .init { state, actions, _ in
    switch actions {
    case .didTapMenuButton(index: let index, word: let helloState):
      state.hellos.remove(at: index)
    case .add:
      state.hellos.append(.init(message: "Hello"))
    }

    return .none
  }
)

[ryantstone]

Thank you both for helping me out, that did fix the bug. @mbrandonw am I using the foreach api incorrectly or just encounter an edge case?

[stephencelis]

@ryantstone This is a gotcha with Reducer.combine that I would consider a bug right now since we should at the very least surface a more helpful error.

[stephencelis]

Hey @ryantstone! I've PRed #25 that should hopefully prevent this kind of confusion in the future. The basics are that index-based mutations are tricky in general (not just in this library), and there are a lot of gotchas when coordinating mutations across these kinds of boundaries.

In order to provide a safer means of managing collections of state we provide IdentifiedArray, which safely manages mutations by unique id instead of index.

I've forked your example repo with the migration path: ryantstone/LoopTest#1

We also use IdentifiedArray in our Todos demo, which has things like dynamic sorting and filtering, which would be much more problematic to manage with index-based mutation.

Let us know if you have any other questions!