-
Notifications
You must be signed in to change notification settings - Fork 18
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
Initial work on the error handling proposal #26
base: master
Are you sure you want to change the base?
Conversation
stew/results.nim
Outdated
@@ -0,0 +1,3 @@ | |||
import result |
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.
let's just do this in a commit to master and properly (ie rename the file)
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.
and stick a deprecation warning in the old one - no use in waiting
stew/errorhandling.nim
Outdated
## | ||
## ``` | ||
## let x = chk foo(): | ||
## KeyError as err: defaultValue |
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.
so this example has style issues that we probably want to forbid in our code: raising exceptions from different abstraction layers.
One of the issues we have is that the API's leak implementation details from different layers - this is a design flaw in these API because it means that error handling is no longer local, and you can no longer consider each layer separate and related only to the layer directly above and below it - it breaks the whole idea of abstraction and layering and turns the application into one giant module where all code depends on all code.
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.
You can easily remap exceptions. I was even considering short-cut syntax for this, but I haven't convinced myself it's appropriate:
let x = chk foo():
OSError: defaultValue
ValueError -> MyError(msg: "Some Message")
The existing alternative is just this:
let x = chk foo():
OSError: defaultValue
ValueError as e: raise (ref MyError)(msg: "Some Message",
parent: e)
also, let's split out the save-macro-output in a separate PR - that seems standalone from this and fairly straightforward |
incorrectChkSyntaxMsg = | ||
"The `check` handlers block should consist of `ExceptionType: Value/Block` pairs" | ||
|
||
template either*[E, R](x: Raising[E, R], otherwise: R): R = |
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 is already called get
for option and result..
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.
get
is not lazy though. Also, with either
you can have a noReturn
statement in the otherwise branch:
let x = either foo():
raise (ref MyException)(msg: "...")
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.
should we make it lazy? seems reasonable?
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.
The point is that we can't do it for the Option
type, so I decided to use a new name. either
also reads for me better in English when you consider the semantics of the operation.
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.
Line 484 in 5512e89
template valueOr*[T, E](self: Result[T, E], def: T): T = |
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.
which reminds me why I originally wrote or
to take an untyped rhs:
template `or`*[T, E](self: Result[T, E], other: untyped): untyped = ...
res or raise XXX
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.
but that doesn't work.. hmm.. not sure why, gives expression expected found raise
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.
It's the Nim parser. I wanted to use the ??
operator initially, but after realizing that the parser doesn't like return
and raise
used with infix operators, I've settled on either
. valueOr
is also OK as far as naming goes, but either
is slightly more visually pleasing perhaps.
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.
is it fixable in the language? ie for a future sugary syntax, it could be a nice-to-have (if we see that it's used a lot)
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. It's hard to tell whethere these grammar changes are possible before you try them out.
README.md
Outdated
@@ -21,13 +21,14 @@ broken out into separate repositories. | |||
Libraries are documented either in-module or on a separate README in their | |||
respective folders | |||
|
|||
- `result` - friendly, exception-free value-or-error returns, similar to `Option[T]`, from [nim-result](https://github.com/arnetheduck/nim-result/) |
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.
they're in alphabetic order
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.
ah, OK, will revert that
Compared to duffy's error handling proposal and the follow-up C++ proposal, there are several significant detractions here:
over the status quo this proposal has a few benefits that I'd use already if I had them:
In general, all in all, a way forward from here that takes incremental steps would be the following:
|
for v in mitems(values): | ||
let | ||
enumValue = parseEnum v | ||
replacement = replacements[enumValue] # Error here |
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 is the hallmark of exception-unsafe code, and a school-book example of a design issue in Try
: mid-way through the iteration on values, an exception is raised and you're left with a partially mutated values
input which the calling code cannot reason about and that is invisible when reading the code, unless you carefully analyze the control flow - there's no visual indication for the reader that this in particular is where there KeyError
happens.
The most "natural" thing to do if the compiler is pestering me about KeyError
here is to add a except KeyError
below except ValueError
which hides the actual exception safety issue with this code.
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.
notably, the raising
annotation does not suffer from this issue, as I understand it
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.
Well, this is just an illustrative example. After all, the code doesn't compile, so it being a "school-book example of a design issue" is reasonable. If you can offer a better example with exceptions that every Nim user will be familiar with, feel free to suggest it.
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.
do you need to put raising
inside Try
or not?
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.
You have to put it for APIs that use the errors
pragma.
The above example will fail to compile with an error indicating | ||
that `replacements[enumValue]` may fail with an unhandled `KeyError`. | ||
|
||
## The `either` expression |
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 seems like a small, out-of-scope utility - I wouldn't add until there's significant evidence that it's generally useful in real-world code - using a default after getting an error is a bit weird: the operation failed to calculate a value so now I use a some other value? why didn't the calculation give me that replacement value directly if that makes sense for the type? clearly, this is used when taking "shortcuts" with the type, and seems less legitimate
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.
Why would you censor a small utility like this? I find it useful, so let's see how much adoption it will get in practice. it's not just about providing a replacement value - it's also a control-flow construct.
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.
because it's meant to become a core concept in the language and imported in every module basically - you don't want cruft in there from the start - better start every feature at -100 points and work up a case for it, and in this case it's cheap and easy to do "manually"
|
||
## The `check` expression | ||
|
||
The `check` macro provides a general mechanism for handling |
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.
so we don't call it match
because... it's not general enough?
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.
check
is about unpacking the successful result while handling the errors. I think that's different enough from general pattern matching to deserve its own name.
``` | ||
|
||
When applied to a `Result` or an `Option`, `raising` will use the | ||
`tryGet` API to attempt to obtain the computed value. |
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.
for unification with Result
, this is practically the same as ?
- should ?
be smarter and convert back-and-forth between the error handling models? from a performance POV, that's a bit dumb (the visual cost is much smaller than the incurred penalty) but it's... pragmatic.
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.
I considered this as well, but it may be indeed too magical. I guess we'll know soon with more practice.
I plan to write some guidelines in the documentation. Hopefully, this weekend.
I consider the delivery mechanism of exceptions a separate implementation detail in Nim which is subject to change. The proposal will be fully compatible with an improved version of Nim where the recoverable errors are much cheaper to raise and handle.
It's arguable whether
Not true, see above.
This statement is largely untrue. Any interaction with the std lib will enforce the error handling at the boundary where
Well, this is one of the key insights I wanted to cover in more depth in the guidelines. One of the design criteria when you make an API is whether you want introducing a new error type to be a breaking change for the API. You've argued sometimes that A good example for this that will become classic I think is Chronos's
The
Both of these are crucial and thus the Raising type is part of v1. |
I mean code guidance, as in "what's the easiest thing to do" - nobody reads documentation unless they absolutely have to - reading a manual is a cost. |
where |
The commit also adds a facility for writing the generated macro output to a file. It also introduces a new module named `results` that should eventually replace all usages of `import result`.
The commit also adds a facility for writing the generated macro
output to a file. It also introduces a new module named
results
that should eventually replace all usages of
import result
.