-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: non-scary error handler #2043
Conversation
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 like this,
does this also print the location that paniced? because that is useful to get started when a user reports an issue?
how does color_eyre::install()
differ?
int. tests that check output fail now and need to be updated
@mattsse |
cebad3f
to
b611a72
Compare
Location: | ||
[35mcli/src/compile.rs[0m:[35m151[0m |
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.
can we keep this? this is very helpful when a user reports an issue so we know where the panic happened and can start tracking it down
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 think this makes it seem like it's always a Forge issue though - and this info can be surfaced if they run with FORGE_DEBUG=1
which is (imo) a better user experience.
So the behavior in this PR is:
- By default for errors we just print the error message (and any errors preceding that error etc.)
- If
FORGE_DEBUG=1
(or any value, really) then we display the current error format (with location, backtrace stuff etc.) - If it's a panic then it will display the error in the same manner as
FORGE_DEBUG=1
but with an added message that says that this is an error they should report (because we ideally don't want to panic)
Makes sense?
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 think this makes it seem like it's always a Forge issue though - and this info can be surfaced if they run with FORGE_DEBUG=1 which is (imo) a better user experience.
agree
this makes sense!
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.
lgtm
The errors displayed to users are good for debugging, but most of the time they are not errors in Foundry, but rather errors in their input. I think we should distinguish between these and display appropriate reports, because the current error handler really looks like something super fatal happened regardless of whether that is actually the case (e.g. by displaying what file and line it is on in Foundry itself).
For panics, the current error handler is used, and for non-panics a simpler custom error handler is used.
Because we probably still want to be able to go back to the old one, you can freely switch using
FOUNDRY_DEBUG
:We can add more stuff onto the error reporter was we want (for example, we could handle specific errors in a special way) - currently it just displays the top-level error and its cause (recursively).
Note: The "Error:" is from Rust's
Termination
trait and we can't configure it - the only way around it is basically by not returning anything frommain
and instead catching the error ourselves.Closes #2042