Fold restriction not allowing any Function<*> to be returned is too strict about arrow HOT 20 CLOSED

This thread definitely gives enough reasons to revert the change. I think we should do a 1.2.4 bug fix release, given that we haven’t completely moved main to 2.0 yet.

from arrow.

I see your point! Would you be satisfied if it was changed to an assert call so that it wouldn't trigger in production? Also, I think there could be some way to make this error static by using @OverloadResolutionByLambdaReturnType, but that'll need further investigation. In fact, with this type of solution, we can have it marked with an OptIn annotation so that you have a simple "I know what I'm doing" switch off

from arrow.

Would you be satisfied if it was changed to an assert call so that it wouldn't trigger in production?

Please don’t do that. You’re still replacing a system that threw runtime exceptions when used improperly with a system that throws runtime exceptions even when used properly, just that this alternative will “only” break unit tests. I agree with Zordid. If you can make it work correctly and at compile time, that’s good, but if that’s not possible, I’d rather have no checks than bad checks.

And in the future, I’d appreciate you not making breaking changes that produce runtime exceptions in patch releases. 😅

from arrow.

Has this really been an issue for anyone? I mean, yes, it breaks at runtime when you use the Raise context during the computation at a later time - but hey, yes, that hurts. If we cannot tell this at compile time though, we should not make good code break to protect people who did not understand.
I would revert to what we had.

from arrow.

@nomisRev just adding you to see this issue quickly... ;-)

from arrow.

To make the same code work with Arrow 1.2.3 it is necessary to give up the typealias for the validator and create your own interface, which must not inherit the functional type, but can only mimic the old interface by an operator fun invoke like this:

fun interface NumValidator {
    operator fun invoke(int: Int): Either<String, Int>
}

context(Raise<String>)
fun createLessThanNumValidator(lt: Int): NumValidator {
    ensure(lt > 0) { "lt must be a positive number" }
    return NumValidator { i ->
        either {
            ensure(i < lt) { "$i is not less than $lt" }
            i
        }
    }
}

This will circumvent the error thrown by either (a.k.a. fold) while doing the exact same thing.

from arrow.

Use foldUnsafe instead, which bypasses those checks. Maybe a comment should be added about it in the docs

from arrow.

Hm. I really do not like to use functions called ...unsafe at all. It is like using !! everywhere, basically stating "I know what I am doing"... :(
But yes, this hint needs to be added not only to the docs, but as well to the error thrown! It was really, really baffling to me.

The problem with the error is:
I am doing nothing wrong in my code. Not breaking any boundaries at all. Just using the tools from Arrow in a correct way. If it weren't for my unit tests that run this code, I could have easily been deploying a new version of our product without catching this problem until it's too late. That's why I am so hard against introducing runtime exceptions that feel more like they should be compile time checks.

from arrow.

Another catch people will stumble upon is: fold is used as a basis in many other higher level functions, like recover in my case. It even took me a while to realize what is going on here, as I did not do any calls to fold at all. So, the exception thrown here inside fold is very user unfriendly...

from arrow.

...just realizing, in my case, as I am using recover and not fold itself, I would need to call an recoverUnsafe, right? Does that exist...? ;-)

from arrow.

I played around with it and I do understand what you wanted to achieve with this error. Alas, I am not convinced this is the right way to go forward, as this change breaks compatibility for users like me in an unintended way, whereas the old error was clearly an error provoked by abuse of the nested raises.

This code failed at runtime with Arrow 1.2.1 - and it should. The inner function returned has nothing to do with the outer scope at its creation time and as such must not call raise.

fun main() {
    val i = 5
    val r = recover({
        ensure(i < 10) { "not less than 10" };
        { i: Int ->
            if (i == 3) raise("BOOM!")
            i * 2
        }
    }) { error(it) }
    println(r(3))
}

In Arrow 1.2.3 this correctly throws an error - sadly at runtime, which is not good enough as this is a static problem. Because of today's limitations you cannot tackle this correctly at compile time, and as such the addition to disallow any functional or closure type as a return type was added. But this is, IMHO, not a good idea, it even looks awkward as you have to specify explicitly some types you had thought of today:
if (it is Function<*> || it is Lazy<*> || it is Sequence<*>)
But a) is this all? and b) will you add more types that potentially could leak the Raise context?

I would argue this change is a little too far fetching and does not help as abuse has also been detected at runtime before, so there is no difference at all - other than punishing the correct use of fold and all derived methods.

from arrow.

If there's any way to make it more subtle, that would be a good way. I know it's a very tricky situation to work around, as basically you need the compiler to "know" not to make the raise context part of the closure created in the returned function. If that can be done with the said annotation, fine. Otherwise, I personally would go back to 1.2.1 behaviour. If a user really does that, it breaks, but it should. Then, telling in the exception what was done wrong is best you can do.

from arrow.

And in the future, I’d appreciate you not making breaking changes that produce runtime exceptions in patch releases. 😅

@kageru I'm so sorry about that 😓 😭That was never our intention!
This problem has kept me up at night for more problems than I care to admit in the last 2 years 😅

I think part of our rationale was here that we imagined almost no1 doing this, and we were afraid people were having subtle bugs right now. Clearly some of you really now what you're doing here 😁
Since for example in @Zordid example, it was perfectly valid. Since he nested a secondary either inside his Function. The problem actually lies in raise being leaked, but we have another "fix" for that.

So this is what is actually problematic:

val x = either<String, () -> Int> {
   { raise("Hello") }
}

x.getOrElse { fail("Boom!") }
  .invoke() // RaiseLeakedException

That's happening here:

So I guess what I am looking for is feedback, it seems it's hindering at least a couple of people and perhaps we can/should figure out a better default for 2.0.0.

cc\ @wfhartford you also raised this topic on Slack so including you here 😉

from arrow.

I second @kageru's opinion. Don't exchange a system that breaks when not used correctly with a system that breaks when used correctly. ;-) I now had to work my way around a correct solution by either either replacing the use of the recover function which internally uses fold with my own handcrafted recoverUnsafe which uses foldUnsafe instead or (this is what I did) not using a functional type at all, but my own interface that does is not considered "unsafe" by fold, but in effect could equally be doing the wrong thing which you cannot catch by the explicit check against Function, Lazy or Sequence at all.

So, this should be enough to not leave the if check in fold. Cause it does not do what it should do in too many cases and punishes the correct use of the tools.

from arrow.

second @kageru's opinion. Don't exchange a system that breaks when not used correctly with a system that breaks when used correctly. ;-)

I totally agree. Shouldn't have happened.

So, this should be enough to not leave the if check in fold. Cause it does not do what it should do in too many cases and punishes the correct use of the tools.

Very good point. I'm open to revert it, and publish another patch or do it directly in 2.x.x.

from arrow.

Thanks for copying me here Simon. I don't have much to add that hasn't been brought up by others. My use case was validating user input to produce an instance of a sealed class extending Predicate (typealias Predicate<T> = (T) -> Boolean). I don't know what the correct solution here is, but I don't think that an exception at runtime is it. Would it be possible to use something like detekt to identify the problem more precisely?

from arrow.

Would it be possible to use something like detekt to identify the problem more precisely?

I think so, but only using a compiler plugin I am afraid. So I guess we cannot count that as a solution.

@kyay10 experimented with special overloading syntax, to detect when a DSL returns Function1 as value but as discussed in this issue it doesn't really solve the problem for other lazy computation types. So I am afraid that both the overload solution, and runtime solution is not a good option...

from arrow.

I agree @Zordid. I am okay with reverting. Does anyone need a patch on the latest version, or are you okay with using a workaround/older version until 2.0(-RC) lands?

Any other thoughts @kyay10 @serras

from arrow.

Yes, I was thinking the same @serras 👍

If we want to switch 2.0 to main we can, we can just make a new branch from the latest commit before doing that and release 1.2.4 from there. So doesn't have to be blocking, but let's plan some dates anyway!

from arrow.

This problem is fixed as per this commit. Version 1.2.4 is on its way to Maven Central.

from arrow.