Fixes recover inconsistency with raise DSL on types other than Either

[yoxjames]

See Issue: #3051

This PR is a work in progress naive fix for the issue highlighted above. It works in the sense that tests pass but I am guessing this may have problems I am not fully aware of. The idea here was to prevent bind() inside the recover(...) block from using the IorRaise scope and instead use the Raise<Error> scope. IorRaise has it's own scope to manage combine(...) for dealing with Ior.Both

A consequence of this "fix" is that recover has no concept of combine. Personally, I don't find this to be too odd. You are essentially creating a "sub raise" and recovering from anything that goes wrong. This does mean that the left value on an Ior.Both is dropped inside the recover block. However, I didn't want to start totally changing the API and figured this PR could at least serve as a discussion point for how this could be resolved. I can see adding special versions of the recover function to allow for handling Both values and combining them but wanted to keep this simple for now.

[yoxjames]

This highlights the problem in the issue. This test fails without the change above.

[yoxjames]

I came to the idea of doing this from studying why either { recover({ ... }) { ... } } works correctly. This ensures that in the recover block we hit this bind() if we reference bind() vs the one on IorScope. This allows recover to work in the way it does for either. However, the downside of this is, we do nothing with Ior.Both's left value which may be undesirable.

Originally I tried adding a new recover(...) method to IorRaise but couldn't quite get that to work right without more major changes. However, I might have missed something there.

Furthermore, the issue identified with Ior will happen with any other data type that has its own Raise variant. So NullableRaise OptionRaise etc. I didn't want to start making changes for that as I wanted to keep the discussion focused on the core problem. However, it's worth considering on this PR for sure.

[nomisRev]

Hey @yoxjames,

First of all, sorry for the late reply!
I played around a bit with the code, and defining recover within IorRaise is also a possibility.

@RaiseDSL
public inline fun <A> recover(
  @BuilderInference block: IorRaise<Error>.() -> A,
  @BuilderInference recover: (error: Error) -> A,
): A = when (val ior = ior(combineError, block)) {
  is Ior.Both -> {
    combine(ior.leftValue)
    ior.rightValue
  }

  is Ior.Left -> recover(ior.value)
  is Ior.Right -> ior.value
}

but couldn't quite get that to work right without more major changes
This requires updating two things. Not sure if you meant that ☺️

For the above code to work we need to mark combineError and combine within IorRaise as @PublishedApi internal instead of private or you cannot define it as inline. I tested this code with the test you've written, and it still passes.

What do you think about that instead? It would fix "However, the downside of this is, we do nothing with Ior.Both's left value which may be undesirable.".

@serras @franciscodr @raulraja I know we at some point discussed Ior#handleErrorWith and that it doesn't really exists but I think this is a valid implementation 🤔 What do you think?

If we all agree I would like to add this in main before we release 1.2.0.

Thank you @yoxjames for raising this issue, and creating a PR with a proposal 🙏 Amazing first contributions 🙌

[yoxjames]

@nomisRev

This requires updating two things. Not sure if you meant that

Honestly I was just hesitant to change any scoping. I do like this solution better though.

I used this strategy and also added it (and tests) to Option, Nullable, and Result. Let me know if there are any additional questions.

I couldn't get spotless to run (I didn't see it configured anywhere but have not used it and could be missing something).

[franciscodr]

@serras @franciscodr @raulraja I know we at some point discussed Ior#handleErrorWith and that it doesn't really exist but I think this is a valid implementation 🤔 What do you think?

Hey @yoxjames, thanks for taking the time to test and fix this bug. @nomisRev, this implementation looks good to me.

[franciscodr]

I added a couple of minor comments

[franciscodr]

This import is no longer required

Suggested change

	import arrow.core.Ior
	import arrow.core.Ior

[yoxjames]

Corrected.

[franciscodr]

@yoxjames Is this pull request still a work in progress? Otherwise, I can approve it

[yoxjames]

@yoxjames Is this pull request still a work in progress? Otherwise, I can approve it

Updated the description. Yeah I think this is no longer a WIP. Let me know if I should do anything else though!

[nomisRev]

One small nit, but PR looks great. Thank you so much for both the report, and fix @yoxjames! 🙌 Very impressive first-contribution. 👏 👏 👏

[franciscodr]

Apart from Simon's comments, the proposal looks great. Thanks for contributing to the Arrow project, @yoxjames!