Convert the plumbing crates away from thiserror#2716
Convert the plumbing crates away from thiserror#2716Amey Pawar (ameyypawar) wants to merge 58 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb20cf1226
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| impl std::error::Error for Error { | ||
| fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | ||
| match self { | ||
| Error::Io(err) => err.source(), |
There was a problem hiding this comment.
When bytes_of_file() or bytes_with_hasher() returns Error::Io for an open/read/interruption failure, this now exposes err.source() instead of the wrapped std::io::Error itself. For ordinary I/O errors source() is usually None (and for Error::other it skips the io::Error layer), whereas the previous #[error(transparent)] variant made the I/O error available as the source; callers using error chains/downcasting lose the actual I/O error and its kind. Return Some(err) for this variant.
Useful? React with 👍 / 👎.
|
Thanks a lot! When reading the review notes, which I find very valuable, I'd hope that these places to watch out for are marked in code in some way. Maybe Besides that, and if you think the agent does a good job, I think this can continue to spike the whole task, the complete removal of Lastly, something to think about, is to see which other standard-errors should exist as part of Thanks again for all your work! |
fb20cf1 to
15b684b
Compare
|
Done on all counts:
I'll continue with the remaining batches toward complete Also: the earlier |
|
Fantastic, thank you! Please do feel free to put the review markers into their respective commits, and to rewrite them at will. Some future work might want you to change bigger bits, and rewriting is fine with me. I will let you know once I start working on this PR so we don't step on each other feet. One more note: If you think that converting multiple related crates at once would be beneficial, please feel free to do so. My plan is to look at the whole PR at some point, and try to find my way from there. |
|
It would also be helpful if the merge-commit could be removed for a clean patch-stack. Besides that, I would have hoped for more lines to be removed, but maybe that changes once |
|
The final result will land as a clean linear patch-stack (one commit per crate, no merge commits) — I'll flatten it as the last step before it leaves draft. |
0e60087 to
af178a1
Compare
|
The branch is now the clean patch-stack: 29 Deliberately not in this PR:
Converted files still carry the allow↔expect lint-suppression drift from your lint pass (left for one sweep rather than touching every crate); the substantive bits my stale branches had reverted (gix-merge's binary-pick refactor, gix-odb's PartialOrd + allowances) are restored. CI stays red until |
Amey Pawar (ameyypawar)
changed the title
|
For this to really come to fruition, I think it will need All other plumbing crates that weren't converted should also be converted, no need to avoid conflicts. Conflicts are easy to fix nowadays. Thanks again! |
There was a problem hiding this comment.
I took a first look because these line counts didn't go into the right direction, and have some notes.
I'd think if done right, it won't be more than +30% of lines give or take, accounting for formatting and occasional double-usage of the same error message.
Maybe in some cases, it also makes sense to keep a custom Error type, which just isn't implemented with thiserror, which is what I think you already did as well.
In any case, don't let me disturb you, I know you are planning more sweeps. And even if not, it's something I can do too once I take over.
PS: I also saw that in many places, the underlying error is still a custom one that just doesn't use thiserror. Probably fine as first step, but the idea is to only do this when it's truly required or greatly beneficial.
| repo.find_reference(reference_val) | ||
| .or_raise(|| gix_error::message("The HEAD reference could not be located"))? | ||
| .peel_to_id() | ||
| .map_err(gix_error::Error::from_error)?, |
There was a problem hiding this comment.
This explicit mapping should never be explicitly required. But maybe you plan to do more sweeps to remove these.
peel_to_id() previously returned a thiserror type, and now that would be an Exn type, for which Error.
Also also, please optimise for size, so these common error types should be imported into the module to avoid long names.
4676e97 to
66ae9a6
Compare
Converted on top of the lifetime-free parser refactor: all 13 error types -- the generic `lookup::Error<E>` with the bounds thiserror would have inferred, the span error struct, the macro-generated name and value-name errors, section header and value, the file rename and set-raw-value errors, includes, and the four init errors -- become hand-written Display/Error/From impls with texts preserved verbatim. The includes `Realpath` variant wraps gix-path's `Exn` and exposes the inner error via `&**err`.
All seven error types become hand-written Display/Error/From impls with texts preserved verbatim: the five field-query errors whose `source`-named fields surface via source() without gaining a `From`, the init error whose transparent variants forward to the converted gix-config types, and the is-active-platform error whose variants wrap gix-pathspec `Exn` types and expose the inner error via `&**err`. Also adapts gix-index's write error, which had been converted against the pre-conversion gix-lock error: `gix_lock::acquire::Error` is now `Exn<Failure>` and does not implement `std::error::Error`, so its source() exposes the inner `Failure` via `&**err`.
Converts 32 of the error types in `gix` to `pub type Error = gix_error::Error`, moving their construction to the call sites: mailmap, tag, pathspec, id-shorten, the four filter errors, diff options-init/new-rewrites/resource-cache, commit and commit-describe, the status index-worktree and into-iter errors, revision-walk, the tree-editor and tree-diff errors, config-overrides, reference-edits and reference-iter-init, worktree-proxy, repository-attributes, discover, clone-checkout, remote-save, repository-filter-pipeline and revision-spec-parse-single. Message-bearing variants keep their text verbatim by re-attaching it at the call site with `or_raise`/`and_raise`, so causes stay reachable; transparent variants convert their concrete callees with `Error::from_error`, and callees that already return an erased or `Exn` type propagate with a plain `?`. Two tests that matched removed variants now assert the message instead. This is a partial conversion: 110 `thiserror` derives remain in `gix`, so `gix` still depends on `thiserror` and CI stays red until the sweep finishes. Types whose variants are matched by callers -- notably those backing `IsSpuriousError` impls, and `open::Error` -- are deliberately left as custom enums. The explicit `Error::from_error` mappings are transitional and disappear as the remaining callees are converted.
`AsPathSpec` is a `clap` value parser whose `Value` is `BString`: it validates that the argument parses as a pathspec, then stores the *original* argument. While adapting to `gix_pathspec::parse::Error` becoming `Exn<ValidationError>`, it was rewritten to return the parsed `Pattern` and to use `Exn::into_error` as a terminal `map_err`. That does not type-check: `Exn::into_error` yields a `gix::Error`, not a `clap::error::Error`, and the closure no longer produced a `BString`. The workspace therefore failed to build with E0308. Restore the original shape, matching the sibling `CheckPathSpec` parser directly below, which was adapted correctly: annotate the closure with `gix::Error`, use `?` to discard the validated pattern, and return the argument itself.
`gix_pathspec::parse()` now returns `Exn<ValidationError>`, which does not implement `std::error::Error`, so `?` can no longer convert it into the `anyhow::Error` the fuzz harness returns (E0277). Map it through `Exn::into_error` first, matching how other call sites bridge into error-trait-based contexts. Note that the fuzz crates are not workspace members, so neither `cargo check --workspace` nor `cargo clippy --workspace` builds them; only the `Fuzzing` CI job does. All 35 of them now build.
The conversion replaced the `NotANormalComponent` variant, whose message
interpolated the path inline, with `ValidationError::new_with_input()`.
That renders as `<message>: <input>`, reordering user-visible text from
Input path "../outside" contains relative or absolute components
to
Input path contains relative or absolute components: "../outside"
The migration guide translates a formatted variant to the same wording it
had before, and the reordering broke assertions in `gix-fs`' own Windows-only
tests and in `gix-worktree-state`. Construct the message verbatim instead, so
every existing assertion holds unchanged.
Using `Path::display()` also avoids `gix_path::into_bstr()`, which is
`try_into_bstr().expect()` and panics on ill-formed UTF-16 on Windows — the
reason the input was previously routed through a fallible conversion here.
These three assertions were rewritten to match the reordered `Display` output produced by `ValidationError::new_with_input()`. Now that the message is constructed verbatim again, they match upstream unchanged. This also realigns them with the five assertions in the `#[cfg(windows)]` block of the same file, which were never updated because they do not compile on Unix — the divergence that made `test-fixtures-windows` fail.
The sibling error in this file was restored to its original wording, but this one still attached the offending bytes as `ValidationError` input, which renders as a `: "…"` suffix the previous `IllegalUtf8` variant never had. Nothing asserts that text, so it went unnoticed — but it is the same user-visible divergence, and preserving it only where a test happened to look is not a principle. Construct it verbatim as well, and note in the comment that attaching the input is the nicer shape, so it can be adopted deliberately rather than as a side effect of the conversion.
Both `and_raise` call sites live inside `#[cfg(feature = "parallel")]` blocks, so a non-parallel build imports the trait without using it and warns. `just clippy` never sees this because it passes `--workspace`, where feature unification pulls `parallel` back in; only the non-workspace `cargo check --no-default-features --features small` in `just check` resolves `gix` without it. Gate the import rather than removing it, since the parallel build needs it.
Part of converting `gix` itself to `gix::Error`. `commit_graph_if_enabled::Error` was a pure wrapper whose variants nothing matched, so it becomes an alias. The config lookup reaches a still-concrete plumbing error and converts explicitly with `Error::from_error`.
Part of converting `gix` itself to `gix::Error`. `blame_file::Error` was a pure wrapper whose variants nothing matched, so it becomes an alias. `commit_graph_if_enabled()` is already erased, so it propagates with a plain `?`; the diff-cache and diff-algorithm lookups still return concrete plumbing errors and convert explicitly with `Error::from_error`.
Part of converting `gix` itself to `gix::Error`. `diff_tree_to_tree::Error` was a pure wrapper whose variants nothing matched, so it becomes an alias. `diff_resource_cache::Error` is deliberately left concrete for now: erasing it cascades into `status::tree_index`, `status::iter::Error` and `is_dirty`, and `status::iter::Error` is discriminated in `status/index_worktree.rs`. Its call sites therefore still convert explicitly, and will collapse to a plain `?` when it converts.
Part of converting `gix` itself to `gix::Error`. `merge_base::Error`,
`merge_base_with_graph::Error`, `merge_bases_many::Error`,
`merge_base_octopus::Error` and `merge_base_octopus_with_graph::Error` all
become aliases; nothing matched their variants.
Their four message-bearing variants are reproduced verbatim at the sites that
used to construct them, so the text a caller sees is unchanged. Note this does
drop the structured `NotFound { first, second }` fields and the
`MissingCommit`/`NoMergeBase` distinction, which are now only recoverable from
the message — please say if either should stay addressable.
Replaces sixteen `thiserror` enums in `gix/src/repository/mod.rs` with
`pub type Error = gix_error::Error;`. All of them were wrappers that only
forwarded their sources, and a scan of `gix`, its tests, `gitoxide-core` and
the binaries found no code matching any of their variants — every reference
was a construction, which the call sites now perform directly.
This is the shape you asked for: erasing on the caller side removes far more
than it adds, and this file loses the bulk of it.
Left as concrete enums for now:
- `diff_resource_cache` and `index_from_tree`, because erasing either gives
`status::tree_index::Error` a second `From<gix::Error>` and cascades into
`status::iter::Error`, which is discriminated in `status/index_worktree.rs`.
- `branch_remote_ref_name`, `branch_remote_tracking_ref_name`,
`upstream_branch_and_remote_name_for_tracking_branch`,
`pathspec_defaults_ignore_case`, `index_or_load_from_head{,_or_empty}`,
`worktree_stream`, `new_commit` and `new_commit_as`, which carry
message-bearing variants whose context has to be re-attached at each call
site rather than dropped.
Follows the erasure of the merge-related error types in `repository/mod.rs`. `virtual_merge_base_with_graph`'s only message-bearing variant is reproduced verbatim where it was constructed, so the text is unchanged. Callees that are themselves erased propagate with a plain `?` (`filter::Pipeline::options`, `blob_merge_options`, `merge_resource_cache`, `commit_graph_if_enabled`); the rest still return concrete plumbing errors and convert with `Error::from_error`. Keeping those two cases apart matters: using `from_error` on an already-erased callee compiles and passes tests while quietly nesting the error inside itself.
Completes the erasure batch. `find_tree()` still returns a concrete `with_conversion::Error`, so it converts explicitly; `tree.edit()` returns the already-erased `object::tree::editor::init::Error` and keeps its plain `?`, which avoids nesting the error inside itself.
With the error types erased, the inner and outer types are identical, so clippy's `needless_question_mark` fires on the `Ok(…)`/`?` pairs the previous signatures needed. Return the expression directly.
Completes the previous commit: with the error types erased, inner and outer types match, so clippy's `needless_question_mark` fires on the remaining `Ok(…)`/`?` pairs in the merge-base, merge and tree-editing entry points.
3402595 to
25c0ee0
Compare
Part of erasing the transparent config error types. The leniency helper still yields a concrete plumbing error, so it converts explicitly. Note this commit alone does not build; it pairs with the erasure in `config/mod.rs`. Delivered separately only because the tree API rejects multi-file commits on this repository.
That change only compiles together with the erasure of `config::ssh_connect_options::Error`, which is not on this branch yet, so it left the tree unbuildable. Restore the original conversion and land both together instead.
`merge::pipeline_options`, `merge::drivers`, `diff::pipeline_options` and `ssh_connect_options` only forwarded their sources and nothing matched them, so they become aliases; their call sites convert explicitly where the callee is still a concrete plumbing error. `command_context` was tried and reverted: `checkout_options::Error` and `worktree_stream::Error` already embed the erased `filter::pipeline::options::Error`, so erasing it gives them a second `From<gix::Error>`, and `checkout_options` has to stay concrete because `config/cache/access.rs` matches its variants. The reason is recorded in a TODO next to the type.
|
It feels like this PR is a bit troubled, and something that should help reduce scope is to focus on |
8cdaac5 to
e9d1962
Compare
Per review feedback, the plumbing crates keep their expanded, hand-implemented error types rather than adopting `gix-error::Exn`, which does not implement `std::error::Error` and so would have forced out-of-tree callers to unwrap it before propagating. `thiserror` stays removed, so `Display` and `Error` are written out by hand. This covers `gix-fs`, `gix-attributes`, `gix-pathspec`, `gix-url`, `gix-path`, `gix-lock`, `gix-shallow` and `gix-prompt`. Several of them have to move together: `gix-pathspec` reads the public `attribute` field off `gix-attributes`' `name::Error`, `gix-url`'s tests propagate `gix_path::realpath::Error`, and `gix-shallow`'s `write::Error` wraps `gix_lock::commit::Error`, so neither of each pair compiles against the other's erased form. Every message is reproduced verbatim. The variants that were `#[error(transparent)]` forward both `Display` and `source()`, those that carried `#[from]` expose the error they hold and keep their conversion, and the ones that wrapped an error without an attribute keep no source at all, matching what the derive generated in each case. Because the error types implement `Error` again, the call sites that bridged through `gix-error` return to a plain `?`, `gix-ref` names `acquire::Error` directly once more, and `gix` converts explicitly at its own boundary where it erases.
…ger meets `gix::discover::Error` is erased, so `clippy::result_large_err` does not fire for `discover()` or `discover_opts()` any more. The expectation was already removed from the former when `gix` started using `gix::Error`; merging picked the latter back up from `main`, where the error type is still concrete, which left it unfulfilled and failed the lint job.
The note claimed `Io` has no `From` conversion, but a hand-written `From<std::io::Error>` sits below it and predates the `thiserror` removal: it downcasts to recover an `UploadPack` error smuggled through `io::Error` before falling back to `Io`. That is why the field was `#[source]` rather than `#[from]` — the conversion could not be derived.
ae76758 to
dabff9a
Compare
Nothing in the crate refers to it since its error types went back to hand-written impls.
2 participants
Originally the batch-1 PR; scope grew to the whole plumbing conversion as discussed — one
feat!commit per crate (29 crates, leaf-first,gix-lockat the bottom), plus a trailing chore commit for the lockfile, migration plan, and adaptations ingitoxide-core, the CLI,gix's tests andgix-testtools. No merge commits, for cherry-picking.Not in this PR (also noted in
etc/plan/gix-error.md):gix-config,gix-submodule— kept atmain; their conversions predate the lifetime-free config refactor and are best redone on top of it.gix-protocol— waiting on feat: filters and partial cloning: initial support #2375.gix-fs— no conversion yet.gix— the finale; planned as a follow-up PR against current main (where?-out-of-everything lands). Its code is untouched here — only its tests are adapted to the converted crates. Because of that, CI stays red on this PR: the workspace only compiles fully oncegixconverts.Shapes, and when each was used
Exn<Message>/Exn<ValidationError>per the plan's default wherever nothing discriminates variants programmatically; validation/parsing paths carry the offending input viaValidationError.Exn<concrete>(e.g.gix-lock'sFailure) where callers discriminate cases but the gix-error benefits are wanted.Display/Error/Fromimpls (message texts unchanged) where the type is load-bearing — consumer#[from]/#[source]embeds, variant discrimination,Eq/PartialEqin tests,From<E>trait bounds. Mirrors the manual-error entries in Reorganise Error-Handling aroundExn#2351; dropsthiserrorwithout replacement in those crates.Review notes
Exnaliases no longer implementstd::error::Error(by design); in-tree consumers keep working throughDeref, propagation intoBox<dyn Error>/anyhowuses.into_error().TODO(review)markers sit at the known shortcomings and hand-preserved subtleties throughout — the two worth reading first: the io-cause gap through still-unconverted wrappers (gix-path/src/realpath.rs) and the hand-preserved transparent-Io semantics (gix-hash/src/io.rs).gix-attributes: validation errors now render their input with debug quoting (: "foo"), and unquoting failures gain the line number with the cause chained (previously a static message) — that's what the larger test rewrite in its commit reflects.Exn-converted types now assert messages, per the plan; tests of concrete types kept their matches.mainin converted files: the allow↔expect lint-suppression drift from the lint pass (left for one sweep); the substantive changes stale conversion branches had reverted (gix-merge's binary-pick refactor, gix-odb's canonicalPartialOrd+ dropped allowances) are restored.Validated per crate during conversion (crate suites + heaviest consumers, clippy's four feature combos with warnings denied, fmt). The stack is ordered so each crate lands after its converted dependencies; full workspace green arrives with the
gixfollow-up.