-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat!: Use Grain exceptions instead of JS exceptions #565
Conversation
@@ -1969,7 +1973,7 @@ let allocate_array_n = (wasm_mod, env, num_elts, elt) => { | |||
compiled_num_elts(), | |||
Expression.const(wasm_mod, encoded_const_int32(0)), | |||
), | |||
InvalidArgument, | |||
IndexOutOfBounds, |
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 these two, it didn't seem worth it to me to set up calling an exception with arguments in the compiler, when this is just going to be rewritten into Grain soon anyway. I feel that IndexOutOfBounds is also an appropriate error here until this is moved over. If anyone really cares, I can do it sooner.
switch (partial) { | ||
| Partial => [ | ||
{ | ||
instr_desc: MError(Runtime_errors.MatchFailure, [compiled_arg]), |
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.
Like I mentioned in the description, this isn't actually the error that's happening on a match failure. I did have to do this (sensible) change though to allow the exception handler to use match
and not depend on itself.
@@ -375,13 +375,7 @@ module MatchTreeCompiler = { | |||
); | |||
| Fail => | |||
/* FIXME: We need a "throw error" node in ANF */ |
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.
Even though I made this trap instead of moving on silently, I left this note here because that's what needs to happen to throw MatchFailure
instead of just unreachable
.
@@ -502,7 +496,7 @@ module MatchTreeCompiler = { | |||
cmp_id_name, | |||
Comp.prim2( | |||
~allocation_type=StackAllocated(WasmI32), | |||
Eq, | |||
Is, |
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.
Calling Pervasives.(==)
was unnecessary here (and prevented pattern matching on variants to happen in runtime mode). This actually will speed up pattern matching by a non-trivial amount.
@@ -2182,5 +2204,4 @@ let tests = | |||
@ export_tests | |||
@ comment_tests | |||
@ number_tests | |||
@ exception_tests | |||
@ export_tests; |
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.
export_tests
was in the list twice! Sneaky sneaky.
@@ -1,7 +1,5 @@ | |||
import { managedMemory, grainModule } from "../runtime"; |
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 file isn't used, but I kept it in because it can be useful for FFI.
|
||
export let mut printers = 0n | ||
|
||
export let registerBasePrinter = (f) => { |
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.
Arguably this should be dangerouslyRegisterBasePrinter
.
|
||
exception SystemError(Number) | ||
|
||
let stringOfSystemError = (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.
This is honestly one the biggest things that I'm excited for.
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.
🔥
77ecea7
to
8ae6706
Compare
65c6e16
to
17b936b
Compare
17b936b
to
e78e811
Compare
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 PR is so spicy! 🌶️ Love it!
@@ -177,7 +177,7 @@ rule token = parse | |||
| "fail" { FAIL } | |||
| "exception" { EXCEPTION } | |||
| "try" { TRY } | |||
| "raise" { RAISE } | |||
| "throw" { THROW } |
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.
Does this lexer change make this a breaking? (I forget if this PR includes a breaking note)
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.
Mmm, yeah it does (if you used throw
as an identifier).
|
||
export * | ||
|
||
exception MalformedUtf8 |
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 this have a printer?
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 thought the default printer was fine, but I suppose I could see a printer for it.
b574318
to
cf04724
Compare
feat: Add ability to `throw` exceptions feat: Add ability to register custom exception printers feat: Add Exception stdlib with Exception.registerPrinter fix: Use Is instead of Eq for match variant comparison fix: `export *` with exceptions chore: Only emit MatchFailure exception for partial matches
cf04724
to
e646fb0
Compare
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.
Looks like the tests are passing now, so take this ⚔️ (it's dangerous to go alone)!
feat: Add ability to
throw
exceptionsfeat: Add ability to register custom exception printers
feat: Add Exception stdlib with Exception.registerPrinter
fix: Use Is instead of Eq for match variant comparison
fix:
export *
with exceptionschore: Only emit MatchFailure exception for partial matches
Closes #173, closes #301, closes #302.
Overview
This PR adds the ability to throw exceptions in Grain. (Previously, you could create exceptions, pattern match on them, use them with
Result
, etc., but you could not throw them.)The
toString
value of the exception is printed when an error is thrown, but thanks to a new standard library included in this PR, the message can be overridden:Details
Exceptions are managed by
stdlib/runtime/exception
(not to be confused withstdlib/exception
). This module maintains a pointer to the list of available error printers, defines some exceptions to be used by the runtime, and defines a printer for those exceptions. AstoString
is not available yet, it exports a method that allows Pervasives to register the "base" exception printer. If Pervasives is not used (such as via the--no-pervasives
flag), exceptions are printed as the generic "GrainException".If you want to know why the base set of exceptions are defined in that module and not built directly into the compiler, we can have a virtual drink and I'll tell you the tale of how I went down that rabbit hole.
There were a handful of bugs that I needed to fix to get this together, but I'll point them out with some comments.
If this is approved by the core team, there's a few issues that I'll want to open:
stdlib/runtime
.MatchFailure
wasn't actually being thrown on a match failure—we have a custom node in the match compiler that handles that case. That was a bigger refactor than what I thought should go in this PR, so I left it alone for now.fail
ing.