Breaking change on ifLet reducer in 0.54.0

[AndreaRomani]

Description

I found a possible regression/breaking change in release 0.54.0 when using the .ifLet operator with non PresentationState.

The new _IfLetReducer has a different implementation from before, even for non PresentationState. In particular it does access child state even when handling a non child action, whereas all the child logic used to be short-circuited in previous releases.
As far as I understood, the reason why child state is accessed (here) is to allow child effects to be cancelled. I believe this is too strong of an assumption to make: there may be cases in which we don't use the operator for, strictly speaking, presentation purposes, or we may not be interested in effects cancellation at all because the child feature doesn't even start effects, or they are just .fireAndForget.

As an example of why this is a breaking change, the following code runs just fine on previous releases (I tested it on 0.52.0), but hits a crash on 0.54.0. I would expect:

• either to keep the same behaviour before this release
• or to have an overload of .ifLet to restore the previous behaviour opting out from effect cancellation checks

I haven't found previous discussions on this matter, feel free to redirect me there in case this behaviour is intended. Maybe there's already a fix I'm not aware of 🙂

p.s. thanks for the amazing work you did with this navigation series 🚀 looking forward to unlock the full potential from these tools 🚀

struct Parent: ReducerProtocol {
  struct State: Equatable {
    var x: Int?
    var child: Child.State? {
      get { self.x! } // silly example to illustrate the problem. 
                      // In real world cases there may be some pieces of state that we assume are non-nil 
                      // only when we are handling some actions and so we `!` here, but with this change we cannot anymore
      set {}
    }
  }

  enum Action: Equatable {
    case child(Child.Action)
    case parent
  }

  var body: some ReducerProtocolOf<Self> {
    EmptyReducer()
      .ifLet(\.child, action: /Action.child) {
        Child()
      }
  }
}

struct Child: ReducerProtocol {
  typealias State = Int
  enum Action: Equatable {}
  func reduce(into state: inout State, action: Action) -> EffectTask<Action> {
    .none
  }
}

class TCANavigationTest: XCTestCase {
  func test() {
    let store = TestStore(initialState: Parent.State(), reducer: Parent())
    store.send(.parent)
  }
}

Checklist

• I have determined whether this bug is also reproducible in a vanilla SwiftUI project.
• If possible, I've reproduced the issue using the main branch of this package.
• This issue hasn't been addressed in an existing GitHub issue or discussion.

Expected behavior

The example test shouldn't crash

Actual behavior

_IfLetReducer accesses child state even for non child action, opening the possibility for new crashes

Steps to reproduce

Copy the sample code and run the test

The Composable Architecture version information

0.54

Destination operating system

iOS 16

Xcode version information

Xcode 14.3.1

Swift Compiler version information

swift-driver version: 1.75.2 Apple Swift version 5.8.1 (swiftlang-5.8.0.124.5 clang-1403.0.22.11.100)
Target: arm64-apple-macosx13.0

[mbrandonw]

Hi @AndreaRomani, while the behavior of ifLet did change slightly in 0.54.0, we don't think you should be assuming the manner in which a reducer operator accesses state. And further, I think the pattern of holding onto an optional value but force unwrapping it in certain circumstances is extremely precarious. You are avoiding all of the safety guarantees the compiler gives you, and you are seeing the consequences here in this crash, and will certainly see it more situations if you continue embracing this pattern.

Can you show us a more real world example of how this problem crops up?

It's also worth mentioning that the effect cancellation behavior of ifLet is also technically a "breaking change", but we feel it is a correctness fix. It is not legitimate for a child feature to continue running effects after it has gone away, and so we have now fixed that bug.

[AndreaRomani]

Hello Brandon, thanks for your reply! 🙂

I think the pattern of holding onto an optional value but force unwrapping it in certain circumstances is extremely precarious

I agree with this: the reason why we are doing it is to verify some preconditions that we cannot guarantee with our current modelling of state. I believe we can indeed improve it with better a modelling, but it's also true that sometimes there's a trade-off between safety and ergonomics in which the latter is preferred. I'll try to craft out an easy example of this, but meanwhile there is also another reason why I believe having more control on how child effects are cancelled is beneficial.

Consider the following example:

struct Parent: ReducerProtocol {
  struct State {
    enum Destination {
      case first(Child.State)
      case second(Child.State)
    }
    @PresentationState var destination: Destination?
  }

  enum Action {
    enum Destination {
      case first(Child.Action)
      case second(Child.Action)
    }
    case destination(PresentationAction<Destination>)
  }

  var body: some ReducerProtocolOf<Self> {
    EmptyReducer()
      .ifLet(\.$destination, action: /Action.destination) {
        Scope(state: /State.Destination.first, action: /Action.Destination.first) {
          Child()
        }

        Scope(state: /State.Destination.second, action: /Action.Destination.second) {
          Child()
        }
      }
  }
}

struct Child: ReducerProtocol {
  enum State {
    case one
    case two

    mutating func toggle() {
      switch self {
      case .one:
        self = .two
      case .two:
        self = .one
      }
    }
  }
  enum Action {
    case startEffect
    case toggle
  }

  func reduce(into state: inout State, action: Action) -> EffectTask<Action> {
    switch action {
    case .startEffect:
      return .fireAndForget { try await Task.never() }
    case .toggle:
      state.toggle()
      return .none
    }
  }
}

It models a parent feature with two child destinations of the same screen (could also be different in general), each of which has an enum State. A real world example could be a parent onboarding feature within which each page has a certain number of steps to perform.

If you take that code and run the following test it will fail because the child effect is not cancelled.

@MainActor
func testNavigationReducer() async {
  let store = TestStore(
    initialState: Parent.State(destination: .first(.one)),
    reducer: Parent()
  )
  await store.send(.destination(.presented(.first(.startEffect))))
  await store.send(.destination(.presented(.first(.toggle)))) {
    $0.destination = .first(.two)
  }
}

This could make sense: after all the destination that drives the .ifLet operator didn't change. However, if I declare an Identifiable conformance for Child.State, in particular the simplest one like this

extension Child.State: Identifiable {
  var id: Self { self }
}

in that case the effect is actually cancelled (the same test passes) and this is a bit surprising. I'm fine with the fact that the library provides a default mechanism to cancel effects - and actually I think that's great - but in this case it seems like there should also be a way to fine tune how this behaves which doesn't leverage a third party protocol.

[mbrandonw]

I'm going to convert this to a discussion since it is not something we will consider changing right now, but we would love to hear more about the problem if it is really significant.