Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make BindException public #96

Closed
CharlieTap opened this issue Mar 4, 2024 · 5 comments
Closed

Make BindException public #96

CharlieTap opened this issue Mar 4, 2024 · 5 comments

Comments

@CharlieTap
Copy link

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 😬

@michaelbull
Copy link
Owner

michaelbull commented Mar 4, 2024

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.

@CharlieTap
Copy link
Author

): 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

@michaelbull
Copy link
Owner

michaelbull commented Mar 4, 2024

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.

@CharlieTap
Copy link
Author

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

michaelbull added a commit that referenced this issue Apr 10, 2024
@michaelbull
Copy link
Owner

michaelbull commented Apr 10, 2024

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 michaelbull reopened this Apr 10, 2024
@michaelbull michaelbull closed this as not planned Won't fix, can't repro, duplicate, stale Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants