Remove thiserror from every crate but gix, erasing error types at the gix boundary - #2716
Remove thiserror from every crate but gix, erasing error types at the gix boundary#2716Amey Pawar (ameyypawar) wants to merge 73 commits into
thiserror from every crate but gix, erasing error types at the gix boundary#2716Conversation
![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.
3402595 to
25c0ee0
Compare
|
It feels like this PR is a bit troubled, and something that should help reduce scope is to focus on |
ae76758 to
dabff9a
Compare
|
Thanks a lot! From what I can tell, there is only one manifest still needing For Is there anything in the way of finishing this? If not, I will be looking forward to giving it the last push, I will do that with priority. Thanks again for all the time you put into this! |
|
Nothing blocks you — my local commits sit on the PR tip as a clean fast-forward, and On scope, so I don't oversell it: I've converted 17 of gix's ~90 error types to Happy to push it as a head start, or to leave |
|
I am not sure we talk about entirely the same thing, but from what I see, it should be possible to convert as much of When done, and you think that's it reasonably, the PR can come out of draft which is my cue. |
5bfea5f to
bfda230
Compare
Amey Pawar (ameyypawar)
changed the title
thiserror from every crate but gix, erasing error types at the gix boundary
|
Ready for review.
On the line count you raised: the branch adds roughly 6,000 net, so it went up rather than down. Nearly all of that is the plumbing crates, where dropping The 42 isn't a floor. The E0119 blocker depends on order: erasing a hub enum frees everything it pinned, which is how nine types that looked blocked turned into ten erasures. Merge commit is gone — the history is linear now, and the tree hash is unchanged from before the rebase. Two things worth flagging:
The branch is behind I'm around and happy to pick up whatever's useful — more of |
|
Thanks a lot! A rebase would be appreciated, but is no must. |
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.
`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.
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.
Nothing in the crate refers to it since its error types went back to hand-written impls.
Continue converting `gix` to its erased boundary type: thirteen error types in `repository/` and `reference/` become `pub type Error = gix_error::Error`, so callers `?` through them without matching variants. Erased in `repository/mod.rs`: `new_commit`, `new_commit_as`, `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`. Erased in `reference/errors.rs`: `peel::to_kind`, `follow::to_object`, `head_id`, `head_commit`, `head_tree`, `find`. Four types that were targeted stay concrete because erasing them would embed a second `gix_error::Error` in a parent enum and collide on `From` (E0119): `diff_resource_cache`, `index_from_tree`, `index_or_load_from_head_or_empty` (through the `status`/`worktree_stream` chains) and `head_tree_id` (through `status::is_dirty`). Each carries a `TODO(review)` naming the collision. Call sites that bridged through these types go to a plain `?` where the callee is now erased, and to `.map_err(gix_error::Error::from_error)` where it stays concrete. Every message on an erased variant is preserved verbatim at its construction site via `or_raise`/`message!`. `head_tree_id_or_empty` recovered its unborn-HEAD case by matching the nested error structurally; since `head_commit`'s error is now erased, it intercepts `peel_to_commit`'s still-concrete error and recovers `Unborn` via `downcast_any_ref`, which works regardless of the `tree-error` feature.
Continue converting `gix` to its erased boundary type: four error types in `submodule/errors.rs` become `pub type Error = gix_error::Error`, so callers `?` through them: `open_modules_file`, `modules`, `is_active`, `fetch_recurse`. `index_id` and `head_id` were targeted too but stay concrete: `submodule::status::Error` already embeds the erased `status::into_iter::Error` (via `StatusIter`), and it also has `#[from]` variants for both of these, so erasing either would give it a second `From<gix::Error>` and collide (E0119). Each carries a `TODO(review)`. The two custom messages on the erased variants — "Could not read '.gitmodules' file" and "Could not find the .gitmodules file by id in the object database" — are preserved verbatim at their construction sites via `or_raise`.
All three error types here go to `gix::Error`, which the crate reaches through `gix`'s own re-export, so no new dependency is needed. `repository::credential`'s and `pack::explode`'s types were private and are gone entirely — their functions now produce `gix::Error` directly. `pack::input_iteration::Error` is public and becomes an alias, which is breaking for anyone matching its variants; nothing in the workspace does. Of the ten messages these types carried, five are reproduced verbatim at the sites that produce them. The other five had no producer: `pack::explode`'s `Io` and `OdbWrite` could only be reached through `From` conversions that no code path took, because `gix_object::write::Error` boxes those errors long before they arrive, and `input_iteration`'s three belong to a type nothing references. `gix` is now the only crate in the workspace that still depends on `thiserror`.
`diff::algorithm`, `diff::drivers`, `encoding`, `abbrev`, `transport` and `transport::http` become `gix::Error`, so callers `?` through them. Four more were targeted but stay concrete. Three because a parent enum already embeds an erased error and a second one would collide on `From` (E0119): `exclude_stack` through `dirwalk`, `protocol::allow` through `remote::connect` and `commit_signature` through `clone::fetch`. `attribute_stack` is blocked the same way, through `checkout_options`. `stat_options` is a different case: nothing embeds it, but `checkout_options()` maps its two variants across by hand, which an erased type cannot support. Each carries a `TODO(review)` saying which of the two it is. The top-level `config::Error` gained a note as well: erasing `abbrev` spent the one slot it has, so no other `#[from]` member of that enum can be erased without colliding. `key::Error` is deliberately untouched. It is generic over its source and two const parameters, and fourteen aliases are built on it, so it has no erased form. Every message on an erased type is reproduced verbatim where it is produced. That includes the context `transport::Error` used to add when an `http::Error` crossed into it, which the erasure turned into an identity conversion; it is restored at all twelve crossings. `diff_algorithm()` could no longer match the `Unimplemented` variant to decide whether a lenient configuration should fall back, so it re-derives the same condition from the input instead, and a test now covers both outcomes.
`object::conversion`, `object::find::existing::with_conversion`, `object::write` and `object::peel::to_kind` become `gix::Error`. Four others were targeted and stay concrete. Erasing `find` or `commit` would give a parent enum a second `From<gix::Error>` and collide: `update::Error` already embeds the erased `reference::find::Error`, and `submodule::head_id::Error` the erased `reference::head_commit::Error`. `try_into` is destructured by two callers that read its fields, in `object::commit` and in the revision-spec delegate. `blob::diff::lines::Error` is generic over the caller's own error and hands it back through `ProcessHunk`, which a type alias cannot express. Each carries a `TODO(review)`. Every message on an erased type is reproduced verbatim where it is produced. One test changes, and it is worth saying why. `peel::to_kind::Error::NotFound` carried the offending object id and both kinds as fields, and a revision-spec test asserts on that frame's `Debug` inside the error tree. The text it prints is unchanged — the assertion covering the printed form passes untouched — but the typed frame is gone, so the tree now shows the same sentence as a `Message`. Keeping it typed would mean holding a private concrete payload behind the erased type; the snapshot follows the erasure instead, matching what the migration notes prescribe for formatted variants.
`worktree::excludes`, `worktree::attributes` and `worktree::pathspec` become `gix::Error`. All of their variants were transparent, so no message moves; the one message in the file, on the case-sensitivity lookup in `pathspec()`, is reproduced verbatim and still built only when that lookup fails. `worktree::open_index` stays concrete. It is embedded by two enums that already embed an erased error, so erasing it would give each of them a second `From<gix::Error>` and collide: `repository::index_or_load_from_head_or_empty::Error` through `object::peel::to_kind::Error`, and `status::is_dirty::Error` through `status::into_iter::Error`. A `TODO(review)` names both. That second one is worth noting for whoever continues this: `is_dirty::Error` now pins three types concrete on its own — `reference::head_tree_id`, `status::tree_index` and `worktree::open_index` — because one of its five members is erased and only one can be.
`remote::find`, `remote::connection::fetch::update_refs::update` and `object::find` become `gix::Error`. All twelve messages the first two carried are reproduced verbatim where they are produced. `object::find` is here rather than in its own batch because erasing `update::Error` dissolved the reason it had been kept concrete: the collision was with `update::Error`'s variants, and those no longer exist. Its `TODO(review)` said otherwise and has been removed. Five types under `remote/` stay concrete, and for two different reasons. `remote::init` and `remote::name` would each give a parent enum a second `From<gix::Error>`: `clone::fetch::Error` and `remote::save::AsError` respectively. `ref_map`, `fetch` and `fetch::prepare` are blocked more firmly — each implements `gix_protocol::transport::IsSpuriousError`, which would become a foreign trait on a foreign type once the error is an alias, and that trait is how a transient network failure is told apart from a permanent one. Erasing them would cost retry behaviour, not just a trait impl. Callers also match their variants in `env.rs` and `clone::fetch`. `object::find` wrapped a boxed error, and `thiserror`'s derive handled that for its two producers. `gix_error::Error::from_error` does not, so `try_find_header` and `try_find_object` now bridge through `io::Error::other`, as this crate already does elsewhere.
…ked others An inventory of what remained turned up twelve types with no recorded reason for staying concrete. Three of them were only ever mentioned as the reason *other* types were blocked, and nobody had tried them: `status::is_dirty`, `repository::worktree_stream` and `config::checkout_options`. None had a parent embedding them, so all three erase. Doing so freed three more, which are erased here too: `reference::head_tree_id`, `config::command_context` and `config::stat_options`. `checkout_options()` no longer needs to re-map `stat_options`' two variants by hand, since both sides are the same type now. Four types stay concrete, and their notes were wrong. `index_from_tree` and `attribute_stack` blamed hubs that this commit removes; `open_index` named one real blocker and one that no longer exists; `tree_index` had no note at all despite being blocked by `status::iter::Error`. All four now name blockers that were confirmed by compiling with the type erased and reading the error. Only `worktree_stream::Error::NotATree` carried a message; it is reproduced where it is produced. Nine call sites that wrapped these errors no longer need to and were collapsed to `?`.
Every `thiserror` error type left in `gix` now carries a `TODO(review)` note recording why it could not be converted to `gix-error`, so what remains is legible rather than implicit. Each note names one of four structural blockers found during the conversion: callers matching variants or reading fields, a parent enum whose single `From<gix_error::Error>` slot is already spent (E0119), a type parameter that a `pub type` alias cannot carry, or a local implementation of a foreign trait (E0117). Blockers inside `gix/src` are cited by symbol name rather than by line number, so those references survive later edits to the files they point into. Citations into `gix/tests` keep their line numbers: they point at caller match sites that have no unique symbol to name, in a tree this campaign does not modify.
…ack collision Erase three error types proven safe to collapse to `gix_error::Error`: - `dirwalk::Error` (`gix/src/dirwalk/mod.rs`) — the hub. Its two message variants (`MissingWorkDir`, `ListWorktrees`) are re-raised via `or_raise`/ `ok_or_raise` at their construction sites in `gix/src/repository/dirwalk.rs`. - `dirwalk::iter::Error` (`gix/src/dirwalk/iter.rs`) — cfg-split on the `parallel` feature; repaired in both the threaded producer path and the serial fallback path, including its `SpawnThread` message. - `config::exclude_stack::Error` (`gix/src/config/mod.rs`) — only possible now that `dirwalk::Error` no longer collides with it via two `From<gix_error::Error>` impls (the E0119 the surviving note used to cite). Both its messages (`Io`, `ExcludesFilePathInterpolation`) are re-raised at their construction site in `Cache::assemble_exclude_globals()` (`gix/src/config/cache/access.rs`). Beyond the audited call sites, this also fixes two double-wraps that the erasure of `exclude_stack::Error` would otherwise silently introduce — a callee returning `gix_error::Error` wrapped again via `.map_err(gix_error::Error::from_error)`, which compiles but nests erased-in-erased: - `Repository::excludes()`'s call into `assemble_exclude_globals()` (`gix/src/repository/attributes.rs`), collapsed to a plain `?`. - `Worktree::excludes()`'s call into `Repository::excludes()` (`gix/src/worktree/mod.rs`), where the trailing `.map_err(...)` is now dropped entirely since both sides are `gix_error::Error`. Deletes the `TODO(review)` note on each of the three erased types.
Erase two error types proven safe to collapse to `gix_error::Error`,
both in `gix/src/head/peel.rs`:
- `head::peel::Error` — its two variants were `#[error(transparent)]`
(`FindExistingObject`, `PeelReference`), so no message needed re-raising;
every internal call site (`id.header()`, `id.object()`,
`peel_tags_to_end()`, `nr.peel_to_id()`) is now a plain
`.map_err(gix_error::Error::from_error)`, and the one manual construction
in `peel_to_object()` (`Error::FindExistingObject(err)`, wrapped inside
the still-concrete `to_object::Error::Peel`) becomes
`gix_error::Error::from_error(err)`.
- `head::peel::into_id::Error` — its `Unborn { name }` message is re-raised
via `gix_error::Error::from_error(gix_error::message!("Branch '{name}' does
not have any commits"))` at its construction site in `into_peeled_id()`.
Beyond the audited call sites, this also fixes two double-wraps that these
erasures would otherwise silently introduce — a callee returning
`gix_error::Error` wrapped again via `.map_err(gix_error::Error::from_error)`:
- `Repository::head_id()` (`gix/src/repository/reference.rs`): its call into
`Head::into_peeled_id()` had a trailing `.map_err(...)` that is now dropped,
since both sides are `gix_error::Error`.
- `Repository::modules()` (`gix/src/repository/submodule.rs`): its call into
`Head::try_peel_to_id()` is now a plain `?`, for the same reason.
Also fixes a stale note: `reference::peel::Error`'s `TODO(review)`
(`gix/src/reference/errors.rs`) cited `head::peel::Error::PeelReference` as
a collision-free parent; that variant no longer exists once `head::peel::Error`
is a type alias, so the now-inaccurate clause is dropped. The note's actual
justification (callers match `reference::peel::Error::ToId(...)` directly)
is unaffected and unchanged.
Deletes the `TODO(review)` note on each of the two erased types.
Erase five more error types proven safe to collapse to `gix_error::Error`:
- `config::set_value::Error` (`gix/src/config/mod.rs`) — its `SetRaw`
transparent variant becomes a plain `.map_err(gix_error::Error::from_error)`
at the three `set_raw_value_by()` call sites in
`gix/src/config/snapshot/access.rs`. Its `Validate` variant needs no
wrapper: `validate::Error` is erased in this same commit, so those call
sites remain a plain `?`. Its two message variants (`SubSectionRequired`,
`SubSectionForbidden`) are re-raised via
`gix_error::Error::from_error(gix_error::message(...))` at their
construction sites in the same file.
- `config::tree::key::validate::Error` (`gix/src/config/tree/mod.rs`) — its
sole field was `Box<dyn std::error::Error + Send + Sync + 'static>`, which
`gix_error::Error::from_error()` can't accept directly (the std blanket
impl needs `Sized`); bridged via `std::io::Error::other(err)` at the one
call site in `Any::<T>::validate()` (`gix/src/config/tree/keys.rs`),
matching the idiom already shipped in `gix/src/pathspec.rs`.
- `config::tree::key::validate_assignment::Error` (same file) — its
`Validate` message is re-raised via `.or_raise(...)`, and its `Name`
message via `gix_error::Error::from_error(gix_error::message!("{message}"))`,
at both call sites in `gix/src/config/tree/traits.rs`
(`validated_assignment()`/`validated_assignment_with_subsection()`).
- `config::snapshot::credential_helpers::Error`
(`gix/src/config/snapshot/credential_helpers.rs`) — its `InvalidUseHttpPath`
and `CoreAskpass` messages are re-raised via `or_raise`/`message!`, and its
transparent `BooleanConfig` variant (plus the untouched `protect_protocol`
and two `HELPER_STDERR`/`TERMINAL_PROMPT` sites) become plain
`.map_err(gix_error::Error::from_error)`.
- `status::index_worktree::submodule_status::Error`
(`gix/src/status/index_worktree.rs`) — all three transparent variants
become plain `.map_err(gix_error::Error::from_error)` at their call sites
in `BuiltinSubmoduleStatus::status()`.
`gix::config::tree::Key::validate()` is a public trait method; erasing
`validate::Error` changes its signature to return `gix_error::Error`, which
downstream implementors of `Key` must now satisfy. This is expected and
consistent with the `feat!` breaking-change marker used throughout this
campaign.
Also fixes a stale note: `submodule::status::Error`'s `TODO(review)`
(`gix/src/submodule/mod.rs`) cited
`status::index_worktree::submodule_status::Error::SubmoduleStatus` as its
sole collision-free `#[from]` parent; that variant no longer exists once
`submodule_status::Error` is a type alias, so the now-inaccurate clause is
dropped. The note's actual justification (callers match `Error::State(...)`
directly) is unaffected and unchanged.
Deletes the `TODO(review)` note on each of the five erased types.
Wrapping an error that is already `gix_error::Error` in `.map_err(gix_error::Error::from_error)` nests an erased error inside another erased error. It compiles and passes the test suite, so the compiler cannot catch it; these were found by resolving each call site's callee through its alias chain. Collapse eight such sites to a plain `?`: - `status::iter` -> `BuiltinSubmoduleStatus::new()` (`submodule::modules::Error`) - `repository::merge` -> `merge_pipeline_options()`, `merge_drivers()` and `diff_algorithm()` (three sites) - `repository::blame` -> `diff_algorithm()` - `commit` -> `commit_graph_if_enabled()` - `pathspec` -> `pathspec_defaults_inherit_ignore_case()` - `filter` -> `Core::CHECK_ROUND_TRIP_ENCODING::try_into_encodings()` (`config::encoding::Error`) Three of these live in `gix/src/repository/merge.rs`, which a bare `cargo check -p gix` does not compile at all; they are only visible with `--features merge`. Also corrects the `TODO(review)` note on `remote::find::existing::Error` (`gix/src/remote/errors.rs`), which claimed both its `#[from]` parents had no other erased member. Erasing `config::snapshot::credential_helpers::Error` spends the `From<gix_error::Error>` slot of `env::collate::fetch::Error` via `CredentialHelperConfig`, so that type is now blocked by E0119 there in addition to its existing caller-match blocker. The claim about the other parent, `remote::find::for_fetch::Error`, still holds and is unchanged.
The plan had gone stale enough to mislead: it reported 33 crates pending when only `gix` remains, listed finished crates as blocked on a refactor, and prescribed an idiom that review had since overruled. Record what the migration actually became — a two-tier strategy. Plumbing crates drop `thiserror` for hand-written `Display`/`Error` impls on concrete enums and take no `gix-error` dependency; the `gix` boundary erases to `pub type Error = gix_error::Error;` where callers do not need to match variants. Add the two findings that cost the most to discover and appear nowhere in the original plan: the four structural blockers that keep a type concrete, and the double-wrap trap, where wrapping an already-erased error compiles and passes the suite while nesting an error inside itself. Note that the E0119 blocker is order-dependent rather than permanent — erasing a hub enum deletes it and frees everything it pinned.
The doc-comment on the erased `config::transport::Error` alias explained which of its former variants had been dropped and why — reasoning aimed at reviewers of this change, not at callers, and it would have shipped to docs.rs. Move it to a line comment, leaving the doc-comment to describe only what the type is. Every other note of this kind in the conversion already uses a line comment.
The plan claimed every crate that dropped `thiserror` moved to hand-written concrete errors rather than to `gix-error`. That was generalised from the plumbing crates and is not true: 16 crates take a `gix-error` dependency and use its types directly, `gix-date` most completely, whose entire public error is a `ValidationError` re-export. Those crates also had no place in the migration's stated two-tier model, which described only the plumbing crates and the `gix` boundary. Name them as a tier of their own so the model matches what was actually built.
Three claims in the plan did not survive checking. The count of crates adopting `gix-error` was a dependency-key count, which also caught `gix` itself and `gix-error`'s own dev-dependency. Fourteen crates actually use its types in their public error surface, and two of them were missing from the `ValidationError` list. The taxonomy said every remaining type falls into exactly one of four blockers. Thirteen of the forty-two cite more than one, five of them three. The plan also recorded nothing about the size of the change, which is the question asked most often about it. Note the figures and that the `TODO(review)` blocks account for much of the growth.
The note blamed the branch's line growth on the `TODO(review)` comments. They are about 4% of it. The growth is in the plumbing crates, where dropping `thiserror` meant writing out by hand the `Display` and `Error` impls it used to generate. Give the per-crate figures and drop the suggestion that moving the notes into this file would recover the difference; it would move some 260 lines against a total near 6,000. State the totals approximately, since the commit that records an exact figure is the one that invalidates it.
1796893 to
47d414f
Compare
|
Thanks for giving it a shot! I am closing it in favor of #2847 as it already shows diff-lines that are much closer to what I'd expect, so I will be iterating there, soon hopefully to get rid of these huge PRs. Thanks for your understanding. |
2 participants
Removes
thiserrorfrom every crate butgix. 67 of 68 crates are free of it;gixstill has 42 error types concrete and keeps the dependency.Per review feedback here, the conversion is three-tier rather than a uniform erasure:
thiserrorand keep concrete enums with hand-writtenDisplay/Errorimpls, nogix-errordependency. Eight crates already converted toExnwere reverted to this shape.gix-erroradopters — 14 crates use its types directly in their own error surface.gixboundary erases topub type Error = gix_error::Error;where callers don't need to match variants. 101 types.The 42 that remain each carry a
TODO(review)naming the blockers that apply — callers matching variants, an E0119 spent slot on a parent enum, a generic parameter an alias can't express, or an E0117 orphan impl. Some are blocked several ways at once.etc/plan/gix-error.mdhas the detail, including a double-wrap trap wherefrom_erroron an already-erased callee nests one error inside another and still compiles and passes the suite.gix::config::tree::Key::validate()is a public trait method, so erasingvalidate::Errorchanges whatKeyimplementors have to satisfy — the only trait affected.On size: the branch adds roughly 6,000 lines net, so it went up rather than down. That sits almost entirely in the plumbing crates, where dropping
thiserrormeant hand-writing theDisplayandErrorimpls it used to generate —gix-pack+770,gix-ref+669,gix-filter+494 and so on.gixandgitoxide-coresources together come to -318.Linear history, no merge commit.