Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new standard-library exceptions capability and a first-class handler to support raising and handling exceptions as an effect in DBL/Fram.
Changes:
- Introduces
Exceptioneffect capability with a polymorphicraise. - Adds
hExceptionfirst-class handler intended to invoke a provided function onraise.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/Exception.fram
Outdated
| pub let hException f = | ||
| handler | ||
| Exception { effect raise x = f x } | ||
| end |
There was a problem hiding this comment.
This PR adds a new stdlib effect/handler, but there are no accompanying stdlib tests (compare to test/stdlib/State.fram). Consider adding test/stdlib/Exception.fram covering at least: a) normal return passes through, b) raise triggers the handler function and does not resume, and c) interaction with other handlers (nesting).
There was a problem hiding this comment.
@copilot propose an implementation of such tests based on this feedback
There was a problem hiding this comment.
Nice try. Proposed code parses (not exactly as intended), but fails to type-check. However, after some simple fixes everything works correctly. I'm not perfectly happy with the proposed tests, but it is a good starting point.
|
@ppolesiuk I've opened a new pull request, #333, to work on those changes. Once the pull request is ready, I'll request review from you. |
| ##} | ||
|
|
||
| {## The effect capability for exceptions. ##} | ||
| pub data Exception E X = |
There was a problem hiding this comment.
I'm not sure if Exception is a good name. It is quite long. Do you think Exn is better?
There was a problem hiding this comment.
I like the shorter Exn. With the parameters, Exception _ _ is 13 characters.
|
|
||
| It calls given function `f` when an exception is raised. | ||
| ##} | ||
| pub let hException {X} f = |
There was a problem hiding this comment.
What is the naming convention (or what it should be) for type parameters like X. Should the name be more informative, like Payload?
There was a problem hiding this comment.
Or Info? I'm not really that bothered by X, though.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wojpok
left a comment
wojpok
left a comment
There was a problem hiding this comment.
Implementation-wise everything is fine. This effect can be used to implement canonical error handling that returns Result E X or Option X, though it would be nice to have explicit implementation of those effects for the sake of boilerplate reduction. Nonetheless, it is a nice addition to stdlib
forell
changed the title
This PR provides standard effect capability of throwing exceptions together with a simple first-class handler. It is not clear to me, if the exceptions should be named
Exceptionor justExn.