-
Notifications
You must be signed in to change notification settings - Fork 30
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
Prevent either block nesting to avoid type/try-catch mismatches #148
Prevent either block nesting to avoid type/try-catch mismatches #148
Conversation
In my experience, once one is aware that this can happen, it's actually a nice feature. Normally, yes, you jump to the closest block. But you can get out farther if you need to. It's unusual--at least for me--to be so confused that I have both the type wrong and where to jump to wrong, but they accidentally align and work despite not being desired behavior. The problem comes when you try to jump thread boundaries. So, from my experience at least, I think this avoidance of nesting is not desirable. |
In simplistic examples, sure. The problem I encountered was in a block in which tapir endpoints were also declared, either block was opened on top for either:
val t = Thread(() => e.ok())
t.start() but I still feel it would prevent bugs coming from refactoring that are possible due to the automatic application of implicits. |
core/src/main/scala/ox/either.scala
Outdated
inline def apply[E, A](inline body: Label[Either[E, A]] ?=> A): Either[E, A] = boundary(Right(body)) | ||
inline def apply[E, A](inline body: Label[Either[E, A]] ?=> A)(using | ||
@implicitNotFound( | ||
"Nesting of either blocks is not allowed as it's bug prone with type inference. Consider extracting the nested either block to a separate function." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe reformulate a bit? Sth like:
Nesting of either blocks is not allowed as it's error prone, due to type inference. Consider extracting the nested either block to a separate function.
Looks good - let's try and see when we get complaints :) Do you think we should mention this in the docs? |
I guess we should with an example on how to refactor. |
@lbialy Would you be able to add the docs as well? :) |
Yeah. Btw: I was thinking whether there should be a |
Let's be lazy here and wiat for the first user complaint that they would like such behavior ;) |
Ok, I did add docs and changed the error message to your suggestion. |
def returnsEither: Either[Exception, Int] = ??? | ||
|
||
def inner(): Either[String, Int] = either: | ||
val i = returnsEither.ok() // this can only jump to either on the opening of this function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not compile (as indicated by the CI failure) - you can't .ok()
here as the boundary is String
-error-typed, and returnsEither
is Exception
-error-typed.
Ok, one merged, couple more to go :) Thanks :) |
Either blocks, based on boundaries, provide implicit evidence for safe jumps. One problematic case is when
.ok()
combinator is called on a fork (or inside of exception capturing scope that can swallow a Break), another is when nested boundaries provide evidences for mismatched cases:Notice that first
Either
value evaluated with.ok()
matches outereither
block while being evaluated in the inner block. This might very well be considered a feature - types allow us to decide where to jump - but in my short but stressful experience with this kind of combinators - when this is used with type inference, results can be surprising. In my case the inner block was the body of Tapir handler while outer either block was in main function. The result was, unsurprisingly, a thread killed withBreak
. I would get a compilation error if nested either blocks were not allowed because without the outer block I'd be informed that it's impossible to use.ok()
aseither
block in tapir handler is inferred to beEither[Unit, String]
. I know this is a rather heavy-handed change and there are possible use cases for nested either blocks (tagged jumps for example) but this is confusing enough imho to warrant such a change. I can easily imagine such a change to occur during refactoring of code where inner either block stops matching one of the eithers with.ok()
in it's scope and outer either providing evidence where it's no longer handled correctly.