BindingAction pattern matching fails when using Binding.pullback

[lukeredpath]

Describe the bug
When using pattern matching in a reducer to layer additional behaviour on a binding action, the pattern match fails if the binding action was pulled back from a child binding action.

To Reproduce
Failing test case added to a branch on my fork here:
lukeredpath@a6472a1

Expected behavior
In the above failing test I'd expect the count to be 2 but it is not because the reducer does not match the binding action case.

Environment

• swift-composable-architecture version 0.28.1
• Xcode 12.5
• OS: Reproduced on iOS 14 and iOS 15.

[lukeredpath]

Digging into this a bit deeper, the reason this fails is because the key path given to the case pattern does not equal the key path of the binding action produced using .pullback, given the following assertions (written in the same test added above):

let keyPath: WritableKeyPath<State, BindableState<String>> = \.$value
let bindingAction: BindingAction<State> = .set(\.$value, "a")

XCTAssertNoDifference(keyPath, bindingAction.keyPath) // pass

let viewBindingAction: BindingAction<ViewState> = .set(\.$value, "a")
let bindingActionTwo: BindingAction<State> = viewBindingAction.pullback(\.viewState)

XCTAssertNoDifference(keyPath, bindingActionTwo.keyPath) // fail

I'm not sure exactly how key path equality is determined by swift - it looks like keyPath and bindingAction.keyPath are the exact same values in memory, it's just that the latter is type-erased to a PartialKeyPath when accessed through the binding action, but they are the same.

However when you use .pullback you're creating a brand new key path by taking the KeyPath<NewRoot, Root> and appending the view binding action's key path to that.

You can see this is true because I can get the above failing test to pass by adding a new case to the reducer:

let reducer = Reducer<State, Action, ()> { state, action, _ in
  switch action {
  case .binding(\.$value):
    state.count += 1
    return .none
  case .binding(\.viewState.$value): // this will match the pulled back action
    state.count += 1
    return .none
  default:
    return .none
  }
}

Unfortunately whilst this is a potential workaround for handling patterns inside your feature domain, its not helpful when you want to pattern match on a feature's action in a reducer higher up the tree if your feature is in it's own module and doesn't expose the internals of it its nested view states publicly. For now I've had to resort to using normal actions and bindings in my nested view state/actions and mapping them directly to a binding action on my feature domain.

[stephencelis]

@lukeredpath We actually had a conversation about this on Twitter shortly after the latest binding updates:

https://twitter.com/pointfreeco/status/1440421625690681358

It's a bit of a gotcha, but I think this has been the behavior from the beginning, when BindingAction.pullback was first released many months ago. We also don't really see a way around the behavior, given that the key path held in the action must match the key path in the reducer for == to work.

There are workarounds that don't involve introducing view state in the reducer, but they all have their trade-offs. For example you could use onChange(of:) in your reducer to monitor changes to a specific field, or you could forego the use of view state for views that need binding actions.

Brandon and I have even discussed dropping pullback from the API because the current behavior for sub-pattern matching is less than intuitive 😕

There might be a better formulation of binding actions + view state than the pullback we have right now. We're definitely open to ideas on how to improve this! One theoretical idea we've sketched out would allow the view to specify an explicit state key path. Something like:

public struct LoginView: View {
  let store: Store<LoginState, LoginAction>

  struct ViewState: Equatable {
    var alert: AlertState<LoginAction>?
    @BindableViewState(\LoginState.$email) var email: String

We're not sure if it's possible, though, and we probably won't find the time to experiment with it anytime soon.

If you (or another intrepid member of the community) is interested in running with this idea and seeing if there's a potential solution there, please keep us posted!

I'm also going to convert this to a discussion since it's not really a bug, just a surprising behavior that should either be documented (or removed).

[iampatbrown]

@stephencelis what were the main problems with the theoretical idea? I could see the property wrapper initialisation not being ideal at the moment (which will hopefully be improved).

A rough start could look something like this:

@dynamicMemberLookup
@propertyWrapper
public struct BindableViewState<Value, Root> {
  public var wrappedValue: Value
  public let keyPath: WritableKeyPath<Root, BindableState<Value>>

  public init(wrappedValue: Value, _ keyPath: WritableKeyPath<Root, BindableState<Value>>) {
    self.wrappedValue = wrappedValue
    self.keyPath = keyPath
  }

  public var projectedValue: Self {
    get { self }
    set { self = newValue }
  }

  public subscript<Subject>(
    dynamicMember keyPath: WritableKeyPath<Value, Subject>
  ) -> BindableViewState<Subject, Root> {
    get {
      .init(
        wrappedValue: self.wrappedValue[keyPath: keyPath],
        self.keyPath.appending(path: \.[dynamicMember: keyPath])
      )
    }
    set { self.wrappedValue[keyPath: keyPath] = newValue.wrappedValue }
  }
}

extension BindableViewState: Equatable where Value: Equatable {}

extension ViewStore {
  public func binding<Value>(
    _ keyPath: WritableKeyPath<State, BindableViewState<Value, Action.State>>
  ) -> Binding<Value>
    where Action: BindableAction, Value: Equatable
  {
    self.binding(
      get: { $0[keyPath: keyPath].wrappedValue },
      send: { .binding(.set(self.state[keyPath: keyPath].keyPath, $0)) }
    )
  }
}

Something else I've tried before is to use an optional pullback but wasn't a massive fan of the ergonomics...

extension BindingAction {
    public func ifLet<NewRoot, Value>(
    _ keyPath: WritableKeyPath<Root, BindableState<Value>>,
    pullback newKeyPath: WritableKeyPath<NewRoot, BindableState<Value>>
  ) -> BindingAction<NewRoot>? where Value: Equatable {
    guard keyPath == self.keyPath, let value = self.value as? Value else { return nil }
    return .set(newKeyPath, value)
  }
}

extension Action {
   static func view(_ viewAction: AppView.ViewAction) -> Self {
    switch viewAction {
    case let .binding(action):
      return .binding(
        action.ifLet(\.$username, pullback: \.$username)
          ?? action.ifLet(\.$password, pullback: \.$password)
          ?? action.ifLet(\.$email, pullback: \.$email)
          ?? action.pullback(\.view) // default fallback or you could force unwrap the last ifLet
      )
    case .loginButtonTapped:
      return .loginButtonTapped
    }
  }
}

[iampatbrown]

I changed up the ifLet example. It still requires access to value on BindingAction though. BindableViewState would be my preferred option. I haven't really tested these though. @lukeredpath do you think any would work in your situation?
Here is the updated ifLet:

extension Action {
  static func view(_ viewAction: AppView.ViewAction) -> Self {
    switch viewAction {
    case let .binding(action):
      return .binding(
        action.pullback(
          .case(\.$username, to: \.$username),
          .case(\.$password, to: \.$password),
          .case(\.$email, to: \.$email),
          default: fatalError("BindingAction did not match any pullbacks")
//        default: action.pullback(\.view) // or this
        )
      )

    case .loginButtonTapped:
      return .loginButtonTapped
    }
  }
}

extension BindingAction {
  public struct CasePullback<Root, NewRoot> {
    var pullback: (BindingAction<Root>) -> BindingAction<NewRoot>?

    public static func `case`<Value>(
      _ keyPath: WritableKeyPath<Root, BindableState<Value>>,
      to newKeyPath: WritableKeyPath<NewRoot, BindableState<Value>>
    ) -> Self where Value: Equatable {
      Self { action in
        guard keyPath == action.keyPath, let value = action.value as? Value else { return nil }
        return .set(newKeyPath, value)
      }
    }
  }

  public func pullback<NewRoot>(
    _ cases: CasePullback<Root, NewRoot>...,
    default defaultAction: @autoclosure () -> BindingAction<NewRoot>
  ) -> BindingAction<NewRoot> {
    for `case` in cases {
      guard let action = `case`.pullback(self) else { continue }
      return action
    }
    return defaultAction()
  }

  public func pullback<NewRoot>(
    _ cases: CasePullback<Root, NewRoot>...,
    default failure: @autoclosure () -> Never
  ) -> BindingAction<NewRoot> {
    for `case` in cases {
      guard let action = `case`.pullback(self) else { continue }
      return action
    }
    failure()
  }
}

[yuanhang]

I recently ran into this problem as well. @stephencelis I'm interested in the idea of BindableViewState that you mentioned. I searched around and didn't see any relevant updates, so I wrote one (PR #1677 ), please take a look. Feel free to give your opinion!