Prevent error accumulation in `withNel` when RCE caught

[kyay10]

This is a precedent set by ior, which was carried over into iorAccumulate and by extension accumulate itself. This further extends this rule to withNel. The previous behaviour was surprising since a user might expect ra.withNel { raise(foo.nel()) } to be equivalent to ra.raise(foo) (where ra: RaiseAccumulate), but that wasn't true in the presence of try-catch.

[github-actions[bot]]

Kover Report

File	Coverage [33.28%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Builders.kt	96.47%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/RaiseAccumulate.kt	24.24%

Total Project Coverage	47.12%

[serras]

@kyay10 Could you maybe give an example of where you'll see the change of behavior? I'm having a hard time understanding the whole implications of this change.

[kyay10]

The tests prefixed with tryCatchRecoverRaiseWithNel are the ones where behaviour changes. I'll focus on one of them:

  @Test fun tryCatchRecoverRaiseWithNel() = runTest {
    accumulate(::merge) {
      try {
        withNel {
          accumulate(1)
          raise(nonEmptyListOf(2, 4))
        }
      } catch (_: Throwable) { }
      raise(3)
    } shouldBe nonEmptyListOf(1, 3)
  }

The previous behaviour is that withNel gave you a RaiseNel<Error>: Raise<NonEmptyList<Error>, whose raise function is defined as accumulateAll(r).value This means that the call to raise(nonEmptyListOf(2, 4)) would immediately accumulate 2 and 4, adding them to the accumulated errors permanently. Now, withNel gives you access to a custom Raise<NonEmptyList<Error>>. The behaviour of it is that it will never accumulate errors directly, instead it will raise somehow. For this particular case, it will ultimatey translate to raise(nonEmptyListOf(1, 2, 4)) targeted at the Raise provided by merge. It will then be stopped by the catch, so those errors will (intentionally) disappear forever. Hence, the final list of accumulated errors will just be 1, 3, since the info about 2, 4 was caught and intentionally discarded We already have this behaviour in tests for iorAccumulate because we had it for ior. In particular, this was already true (although we didn't have a test for it, which I added here):

  @Test fun tryCatchRecoverRaise() = runTest {
     accumulate(::merge) {
      try {
        accumulate(1)
        raise(2)
      } catch (_: Throwable) { }
      raise(3)
    } shouldBe nonEmptyListOf(1, 3)
  }

With the same logic applying: raise(2) ends up desugaring to a raise(nonEmptyListOf(1, 2)) targeted at merge, but it gets stopped and discarded by the catch

This PR effectively makes RaiseAccumulate<Error> an intersection of Accumulate<Error>, Raise<Error>, and Raise<NonEmptyList<Error>>, although it hides away the latter and only exposes it through withNel.

This reasoning here is why we don't have Accumulate<Error>: Raise<Error>, even though we easily could. Not only are they different concerns, accumulate(x).value is inheritly different from raise(x), since the former guarantees that x is accumulated, while the latter allows for the RaiseCancellationException to be caught. I personally like that raise only has one (side) effect, namely throwing an exception. accumulate also has one (side) effect, which is adding the error to the accumulator. Value<T>.value has only one (side) effect, which is throwing if the value doesn't exist.

At the same time, you can argue that this is pointless. We have a precedent in ior and iorAccumulate, yes, but perhaps we're over-constraining our implementations. I'd love to hear more if you think we shouldn't extend that precedent to this API