Use opaque reducers in 5.7 to fix builder inference

[stephencelis]

Turns out a lot of the bugs we were working around with CombineReducers inference, as well as buildFinalResult warnings (previously: #1467) go away if we use some ReducerProtocol<State, Action> on Swift 5.7 instead of the concrete alternative.

@slavapestov Not sure if you're aware of these functional differences between inference of generics vs. opaque constrained types or would like some fresh bug reports. It can take some time to file them, but if they're not being tracked yet we can try to file down a repro.

[stephencelis]

I've tested this in isowords and things still build OK...hopefully CI here is also agreeable.

[stephencelis]

buildEither was the one spot I couldn't get some ReducerProtocol to work...so there may be some inference issues when it comes to if-else...but that's rare and I guess we can revisit if/when it comes up.

[iampatbrown]

@stephencelis were you using _Conditional<R0, R1>.first(reducer)?

[stephencelis]

I was trying to, but couldn't figure out how to get R0 to be a some and R1 to be a generic that propagates.

[iampatbrown]

Oh I get you.
The only way I can think of at the moment would be to use custom inits. eg: _Conditional(first: reducer, secondType: R1.self) but not sure if that's any better than just passing the two generics.

[stephencelis]

Fun idea! Seems to work and I imagine is better on the type checker than the alternative, so pushed, thanks! 😄

[iampatbrown]

I hadn't thought to include the type in the case. Did you try a static func as well?

@inlinable
static func first(_ reducer: First, _ secondType: Second.Type) -> Self {
  .first(reducer)
}

Is there a benefit including it as an associated value?

[stephencelis]

@iampatbrown I spoke too soon, using it as either associated value or a static func doesn't appear to compile, unfortunately. Definitely down to merge something that compiles, but both my commit and your code above crashes the compiler 😞

[iampatbrown]

@stephencelis So close.. yet so far...
buildEither probably wants a concrete type in order to match first: and second:.
I think this PR is great without it anyway.

[tgrapperon]

SwiftUI makes Optional<some View> a View itself. Was making Optional<some ReducerProtocol> a ReducerProtocol instead of using this _Optional<some Reducer> considered at some point?

EDIT: I made the comment in the 5.6 branch, but it's only because diffing is all messed up. My remark applies to both branches.

[tgrapperon]

For example, with

extension Optional: ReducerProtocol where Wrapped: ReducerProtocol {
  public func reduce(into state: inout Wrapped.State, action: Wrapped.Action)
    -> EffectTask<Wrapped.Action>
  {
    switch self {
    case .none:
      return .none
    case .some(let wrapped):
      return wrapped.reduce(into: &state, action: action)
    }
  }
}

We can return directly wrapped in buildOptional, and Optional.some(wrapped) in buildLimitedAvailability, while using opaque reducers.

BTW, I've just spotted this thing while checking in SwiftUI interface:

@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
@inlinable @inline(__always) internal func asOptional<T>(_ value: T) -> T? {
    func unwrap<T>() -> T { value as! T }
    let optionalValue: T? = unwrap()
    return optionalValue
}

This is some weird gymnastic and I'm not getting why they don't use Optional.some instead. But this is mostly trivia and unrelated to the PR.

EDIT: I think this is to make sure they don't create T??, etc. So we can probably use this too for reducers.

EDIT2: Well, it still creates T?? as I'm getting it, so I don't know. I'm not 100% on nested optionals, as I think there is some automatic coalescing in some places.

[stephencelis]

@tgrapperon Good find! I'm not sure why we didn't conform optional directly, maybe because of a bug with type aliases in pre-5.7. Added support and removed _Optional, thanks!

We also realized our buildLimitedAvailability wasn't right. This function needs to erase the reducer somehow. I think we snuck a fix into this PR, though.

[tgrapperon]

Do you think we still need the buildOptional block? As I'm seeing it, it should be handled by the buildBlock(reducer: R?) now that R? is a reducer, isn't it?

[tgrapperon]

Nevermind, it is required to work with buildLimitedAvailability at least.

[stephencelis]

@tgrapperon Just merged and missed this. What hit main should at least be an improvement, but if you find other tweaks that can be made def PR!

[tgrapperon]

No, I think we need this. As I'm getting it now:

if #available(iOS 16, *) {
  R()
}

will buildLimitedAvailability R into AnyReducer and then buildOptional it to produce AnyReducer?. If you remove buildOptional, the test for buildLimitedAvailability doesn't pass anymore, so I guess this is what happens (and it is maybe involved with other blocks/situations).

[tgrapperon]

@stephencelis buildOptional is involved as soon as we have unbalanced if checks of some sort, so it is required for sure.