arrow-kt / arrow

Λrrow - Functional companion to Kotlin's Standard Library
http://arrow-kt.io
Other
6.18k stars 452 forks source link

Detect Raise leaks at compile-time #3394

Closed kyay10 closed 7 months ago

kyay10 commented 7 months ago

Fixes #3391

Note that @OverloadResolutionByLambdaReturnType is currently very limited. So limited, in fact, that it doesn't support calls with multiple lambdas, and it doesn't support calls where the lambdas have type parameter usages (other than the type parameter being the return type). Also, one has to explicitly provide a type argument of Nothing now when a lambda would return such, but I think that's such a niche use case that it won't show up anywhere other than our test code. I had been pulling my hair out trying to get @OverloadResolutionByLambdaReturnType to work for this case, so fair warning for any of you that wish to explore this further. I think this change shouldn't break any code that previously worked (other than code of the form nullable { throw Exception())

github-actions[bot] commented 7 months ago

Kover Report

File Coverage [81.62%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Builders.kt 71.08%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Fold.kt 98.11%
Total Project Coverage 52.90%
serras commented 7 months ago

I think this approach is great, @kyay10, using the built-in mechanism to highlight the errors to the developer. I have some concerns because of the use of OverloadByLambdaReturnType, but maybe that feature works well enough for our use case.

kyay10 commented 7 months ago

@serras Admittedly, I am scared of it as well. While tinkering I have seen it fail in any complicated-enough situation. Seemingly, for the basic declarations in this PR, everything works fine enough, but I would sleep better at night if we had maybe more tests for wild and strange usages of nullable et al.

nomisRev commented 7 months ago

but I would sleep better at night if we had maybe more tests for wild and strange usages of nullable et al.

Yes, I can tell you you will sleep better at night after that 😂 😅

I am curios if this solves the problem seen by @Zordid, @kageru, @wfhartford.

Some more context here: https://github.com/arrow-kt/arrow/issues/3391#issuecomment-1993744364

serras commented 7 months ago

According to the other issue, it seems that there are valid use cases for returning a function from within a Raise context. This means that neither check (runtime or compile time) is valid, so I think we should just drop both and go back to the 1.2.1 situation.

@kyay10 @nomisRev WDYT?

nomisRev commented 7 months ago

Agreed @serras!

compile time

The only possibility is probably implementing a compiler plugin, but not sure if you can truly reach 100% correctness that way. It's a problem inherit to DSLs...

Thank a lot @kyay10! If you have any more feedback, or thoughts I'd love to hear them. Closing this PR for now.