Skip to content

Replace eyre with a concrete error type, and 0.6.0 - #73

Closed
pathscale wants to merge 1 commit into
masterfrom
refactor/concrete-errors
Closed

Replace eyre with a concrete error type, and 0.6.0#73
pathscale wants to merge 1 commit into
masterfrom
refactor/concrete-errors

Conversation

@pathscale

Copy link
Copy Markdown
Owner

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::Report is a std type and it was in the return type of every fallible function here, so the crate reached std through its own signatures. The file access is concentrated — page/util.rs holds most of it — but eyre was viral: every caller of every function was tied to std by 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:

LinkLengthMismatch { expected, found }
LinkOutOfBounds    { offset, length, capacity }
PageOverflow       { page, needed, capacity }
Corrupt            { what: &'static str }
Encode
Io                 { code: Option<i32> }

Io holds raw_os_error rather than an io::Error, because that is the std type 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 Report carrying 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

cargo test                 69 + 2 passed, 0 failed
cargo clippy --all-targets clean
cargo fmt --check          clean
eyre in src/               gone; dependency removed from Cargo.toml

This breaks consumers, hence 0.6.0

? converts anything implementing core::error::Error into an eyre::Result, so most call sites are untouched. Two shapes are not:

  • a function returning persist_page(..).await in tail position needs ? and an Ok(())
  • error.wrap_err(..) becomes eyre::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::fs and tokio::fs/tokio::io, concentrated in page/util.rs and the index page files, behind a trait the way worktable-vec did it with embedded-io. std::fmt, std::error, std::str and std::mem are core:: renames. That is a second PR, and this one is what makes it worth doing.

`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.
@pathscale

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant