kotlin-result icon indicating copy to clipboard operation
kotlin-result copied to clipboard

Make BindException public

Open CharlieTap opened this issue 5 months ago • 5 comments

As BindException can be thrown its possible for a another try block to catch it so technically it's part of the API. I actually discovered this because of bug where I was accidentally catching it and then I was unable to write a condition to specifically rethrow it because its marked internal 😬

CharlieTap avatar Mar 04 '24 19:03 CharlieTap

Could you paste the example code in which you managed to catch it? It definitely shouldn't leak from the call to binding{}/.bind().

Generally I would not consider the BindException part of the public API. It is only thrown as part of the ResultBindingImpl which itself is marked as internal. with the expectation that the binding function will catch it.

michaelbull avatar Mar 04 '24 19:03 michaelbull

): Result<Unit, InvocationError> = binding {

   ...

    val result = runCatching { // runCatching will catch the inner bind as it catches throwable
        instructions.forEach { instruction ->
            instructionExecutor(instruction, store, stack).bind()
        }
    }

So this wouldn't be a problem if I could do this

  result.onFailure { err ->
        // todo rethrow bind exception
        if(err is BindException) throw err
        if (err is BreakException) {
            ...
        }
        if (err is ReturnException) {
            ...
        }

I'm mixing exceptions and errors here intentionally, I understand this isn't how you typically program with result but its essential in my use case

CharlieTap avatar Mar 04 '24 20:03 CharlieTap

So fundamentally the problem resides in the fact that all calls to .bind() in a binding expect to throw exactly one level up the chain, such that it breaks out of the immediate parent binding statement, but if a consumer wraps a call to .bind() with a try { } catch (t: Throwable) (or runCatching) they'll catch it but be unable to narrow down the type of Throwable to just the BindException.

If my explanation above is correct then I think I am generally happy with the way things are currently and wouldn't be looking to make the BindException public. This forces you to not try/catch or runCatching any calls to .bind() - and instead expects them to never be able to fail, which is intentional in design.

The fact that .bind() itself returns a Result should be an indicator to the caller that it cannot throw, and therefore a try/catch/runCatching around the call to .bind() is inappropriate.

I understand this isn't how you typically program with result but its essential in my use case

If you'd like to provide a more full-fledged example I'm sure I can help you rewrite it to avoid this case. As it stands the inclusion of a call to runCatching inside of a binding is concerning, and something I definitely would advise against.

michaelbull avatar Mar 04 '24 20:03 michaelbull

I appreciate I have a very unique scenario, I'm building an interpreter and I'm forced to use Exceptions when modelling branching (breaking and returning), so I wanted to mix and match.

If you're happy with your api design thats no problem, what I can do is just not throw bind inside the runCatching lambda and manually wire the result

CharlieTap avatar Mar 04 '24 21:03 CharlieTap

Hi @CharlieTap. I did some thinking about the use case that you've hit, and I have an idea (to make runCatching explicitly handle BindingException for you). I was trying this, but I actually can't seem to come up with a test case that fails with the behaviour you've outlined.

Please can you have a look this test case and tell me what you are doing differently?

For me the runCatching is not swallowing the inner .bind(), and thus I do not need to rethrow it. Please can you explain what you're doing that makes you hit the edge case so I can come up with a test for it and then see if my idea above will solve it for you? Thanks.

michaelbull avatar Apr 10 '24 14:04 michaelbull