Replace eyre with a concrete error type, and 0.6.0 - #73
Closed
pathscale wants to merge 1 commit into
Closed
Conversation
`eyre::Report` was in the return type of every fallible function here, so the
crate reached `std` through its own signatures. Nothing about page framing
needs an operating system: the layout is bytes and the checks are arithmetic.
This is the type that lets the rest of the crate say so, and it is the
prerequisite for the file access moving behind a trait - doing that first
would buy nothing while every signature still named a `std` type.
The enum is also more useful than formatted prose. A caller could not tell
"this page is full" from "these bytes are damaged", because both arrived as a
`Report` carrying a string. Six variants now carry the numbers that justify
them: LinkLengthMismatch, LinkOutOfBounds, PageOverflow, Corrupt, Encode, Io.
18 construction sites, 7 files, no .context or .wrap_err chains to unpick
Io holds raw_os_error rather than an io::Error, which is the std type this
change exists to stop depending on
Three tests asserted on eyre's message strings and now match on variants with
their fields, which is what the change is for.
cargo test 69 + 2 passed, 0 failed
clippy clean
**This breaks consumers**, so 0.6.0 rather than 0.5.8. Not every `?` converts:
a function returning `persist_page(..).await` in tail position needs `?` and
an `Ok(())`, and `error.wrap_err(..)` has to become
`eyre::Report::new(error).wrap_err(..)`. WorkTable needs exactly three such
edits, verified by building it against this branch; the patch is not applied
there because that checkout has another agent's uncommitted work in it.
Owner
Author
|
Folded into #75, which now carries this branch's commits plus the page-stride work rebased on top, linear and with no merge commit. DataBucket carries one PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The first of the two things standing between this crate and
no_std, and the one that has to go first.Why eyre and not the file access
eyre::Reportis astdtype and it was in the return type of every fallible function here, so the crate reachedstdthrough its own signatures. The file access is concentrated —page/util.rsholds most of it — buteyrewas viral: every caller of every function was tied tostdby the signature alone. Moving the I/O behind a trait first would have bought nothing while that was true.Nothing about page framing needs an operating system. The layout is bytes and the checks are arithmetic.
What replaced it
Six variants that carry the numbers justifying them, rather than prose:
Ioholdsraw_os_errorrather than anio::Error, because that is thestdtype this change exists to stop depending on. The message that comes with an OS error says nothing the code does not.This is also strictly more useful than what it replaces: a caller could not previously distinguish "this page is full" from "these bytes are damaged", because both arrived as a
Reportcarrying a string.18 construction sites across 7 files. No
.context()or.wrap_err()chains to unpick, which is what usually makes this painful.Three tests asserted on eyre's message text and now match on variants and their fields — which is the point of the change, not incidental to it.
Checks
This breaks consumers, hence 0.6.0
?converts anything implementingcore::error::Errorinto aneyre::Result, so most call sites are untouched. Two shapes are not:persist_page(..).awaitin tail position needs?and anOk(())error.wrap_err(..)becomeseyre::Report::new(error).wrap_err(..)WorkTable needs exactly three such edits. I verified that by patching WorkTable at this branch and building it — it compiles clean with them. The edits are not applied there, because that checkout currently holds ~980 lines of another agent's uncommitted work and is not mine to commit into. The three-hunk patch is ready and should land with, or just before, this.
What is left for no_std
The file access:
std::fsandtokio::fs/tokio::io, concentrated inpage/util.rsand the index page files, behind a trait the wayworktable-vecdid it withembedded-io.std::fmt,std::error,std::strandstd::memarecore::renames. That is a second PR, and this one is what makes it worth doing.