Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Error refactoring and
thiserror
2.0 #1588Error refactoring and
thiserror
2.0 #1588Changes from 11 commits
8c9bb6e
d3878b3
2292155
c80f5c7
3c22c58
cdb324c
f5f4ca0
8236817
c59a171
b5fd67f
4001a38
da41c02
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Hmmm, so is miette's
Report
incompatible withError::source
? Or is this a by-product of themiden-miette
situation described in the issue somehow?Also
MastForestError
currently doesn't use any#[source]
- but if we add one, then it won't get displayed byReport
, right? Isn't this a larger issue?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.
Yeah,
Report
doesn't implementcore::error::Error
(zkat/miette#366), so anything that is not part of theDisplay
impl of an error would be silently swallowed.This makes me think that we might not want to return
Report
from library functions likeassemble_*
, but the underlyingAssemblyError
instead. On the other hand, this will usually mean that no Report is constructed for such errors and so the error is worse than it could be.This is why for now, I'm including the printed report in the display impl of an error that wraps a
Report
, e.g.: https://github.com/0xPolygonMiden/miden-base/blob/67272131f02ed73b5b919223610e97afa7bdc8db/objects/src/errors.rs#L29(The other reason being that
Report
doesn't implementcore::error::Error
).I'm not sure if I'm missing something here, since the interop between
core::error::Error
andReport
feels unsatisfying.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.
@bitwalker IIRC you'd like us to return
Report
much more, right? From previous conversations, I think you even suggested we convertAssemblyError
to just useReport
s everywhere?I haven't played with
miette
enough to have a good intuition here, but typically when all your error are user-facing/to be printed only, you'd want to useanyhow
/eyre
/miette
(instead of anenum
that you end up printing out anyways). We could do that, but then I'm still not sure how we would "bridge properly" between e.g. errors coming frommiden-processor
which would all useError::source
.TLDR this PR is already a big upgrade from the current state of things, but I wonder what "optimal strategy" we should be striving for 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.
Yeah - we want to remove
AssemblyError
. There is an #1431 specifically for this. I think we can address this right after this PR.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 idea of
Report
(or alternatively, something likeanyhow::Error
if there isn't any associated source code, which is whereReport
really shines) is to allow ad-hoc diagnostics to be constructed/created, while handling all of the annoying/boilerplatey work of conversion/rendering/etc. The use of ad-hoc diagnostics ends up producing much more useful errors in practice IMO.The downside of using
Report
(oranyhow::Error
) is that you can't easily obtain the concrete underlying error type (if there is one), in order to say, assert that it is of a specific type. I don't find this particularly important IMO, and there are better ways to test errors (e.g. how we test assembler errors), and downcasting can be used in many situations if really needed.You can wrap
Report
in a type that allows#[from]
or#[source]
conversion, IIRC that is what theRelatedError
type exists for, but it is probably better to not have a variant that is just a transparent wrapper aroundReport
.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.
Shouldn't we use
#[source]
here? We should also add#[from]
to remove the need formap_err()
(see next comment)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.
Usually, yes, but
LibraryError
is sometimes also converted toReport
(inassemble_library
for example). This particular variantKernelConversion
is only used inTryFrom
which is user-called, so probably not converted to aReport
, but that is just far too subtle and wouldn't be noticed if we change this in the future, so I chose the safe route by including it in the display impl so we don't swallow the source error.I should've added a comment here, similar to the
Forest
variant inAssemblyError
. Will do that, thanks for pointing it out.