Skip to content
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

Cannot use trait Custom: std::error::Error as boxed source due to sealed AsDynError #212

Closed
Byron opened this issue Dec 6, 2022 · 4 comments

Comments

@Byron
Copy link

Byron commented Dec 6, 2022

Thanks for this crate :). I use it all the time and even switched from quick-error to it because it seemed to have no real disadvantages.

My problem is probably most concisely described with code (playground):

use thiserror; // 1.0.37

/// An error which can tell whether it's worth retrying to maybe succeed next time.
pub trait IsSpuriousError: std::error::Error {
    /// Return `true` if retrying might result in a different outcome due to IO working out differently.
    fn is_spurious(&self) -> bool {
        false
    }
}

#[derive(Debug, thiserror::Error)]
#[error("works as AsDynError is implemented for std::error::Error in various ways")]
pub struct WorksError {
    source: Box<dyn std::error::Error + Send + Sync>
}

#[derive(Debug, thiserror::Error)]
#[error("can't work since AsDynTrait isn't implemented for IsSpuriousError and can't be implemented by author either")]
pub struct FailsError {
    source: Box<dyn IsSpuriousError + Send + Sync>
}

fn main() { }

Due to the sealed and private trait AsDynError I am unable to have custom traits as Error which derive from std::error::Error. Probably there is a very good reason that I am missing and I'd be happy to learn.

The motivation for such trait is cargo which will retry if it thinks the encountered error is spurious. I thought it would be nice if the respective gitoxide errors could say so via is_spurious().

The workaround is boilerplate heavy and nearly undoes the benefit of such a trait as one can implement IsSpuriousError for WorksError itself, downcasting to the concrete Error types of the crates to call the is_spurious() implementations.

Could there be a better way?

Thanks for your advice 🙏

@dtolnay
Copy link
Owner

dtolnay commented Dec 6, 2022

Ignoring AsDynError for a second, what would a handwritten std::error::Error implementation for FailsError look like, in your opinion?

@Byron
Copy link
Author

Byron commented Dec 6, 2022

The error types I am dealing with look more like this and removing thiserror from the equation will definitely be more complex than doing a few downcasts.

If there is other solutions than to not use thiserror i'd be happy to try contributing. Otherwise it's certainly OK to close this issue, now that I have written the downcast-workaround I think it's OK to keep as well.

Thanks again

@Byron
Copy link
Author

Byron commented Dec 7, 2022

Another possible action would be to address the error message, here repeated directly from the playground:

error[[E0599]](https://doc.rust-lang.org/stable/error-index.html#E0599): the method `as_dyn_error` exists for struct `Box<(dyn IsSpuriousError + Send + Sync + 'static)>`, but its trait bounds were not satisfied
  --> src/main.rs:20:5
   |
4  |   pub trait IsSpuriousError: std::error::Error {
   |   --------------------------------------------
   |   |
   |   doesn't satisfy `dyn IsSpuriousError + Send + Sync: AsDynError`
   |   doesn't satisfy `dyn IsSpuriousError + Send + Sync: Sized`
...
20 |       source: Box<dyn IsSpuriousError + Send + Sync>
   |       ^^^^^^ method cannot be called on `Box<(dyn IsSpuriousError + Send + Sync + 'static)>` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `Box<dyn IsSpuriousError + Send + Sync>: std::error::Error`
           which is required by `Box<dyn IsSpuriousError + Send + Sync>: AsDynError`
           `dyn IsSpuriousError + Send + Sync: Sized`
           which is required by `dyn IsSpuriousError + Send + Sync: AsDynError`

Rust as of 1.65 won't make it clear where the trait AsDynError is defined. Due to its official name I assumed it's in the standard library, and failing to find it there led to searching crates.io and lib.rs, to no avail. Only at last I thought that it then must be originating in thiserror (maybe something more experienced users will know right away) and found it here.

I don't know if this can reasonably be addressed in the Rust compiler, but since it is an implementation detail of thiserror it might be easiest to rename the trait so that it's clear what the users is or isn't supposed to do.

So far thiserror was absolutely wonderful and just worked with whatever I threw at it, which is why I'd love to smooth out this interaction with the library as well.

bors added a commit to rust-lang/cargo that referenced this issue Mar 2, 2023
gitoxide integration: fetch

This PR is the first step towards resolving #1171.

In order to get there, we integrate `gitoxide` into `cargo` in such a way that one can control its usage in nightly via `-Zgitoxide` or `Zgitoxide=<feature>[,featureN]`.

Planned features are:

* **fetch** - all fetches are done with `gitxide` (this PR)
* **shallow_index** - the crates index will be a shallow clone (_planned_)
* **shallow_deps** - git dependencies will be a shallow clone (_planned_)
* **checkout** - plain checkouts with `gitoxide` (_planned_)

The above list is a prediction and might change as we understand the requirements better.

### Testing and Transitioning

By default, everything stays as is. However, relevant tests can be re-runwith `gitoxide` using

```
RUSTFLAGS='--cfg always_test_gitoxide' cargo test git
```

There are about 200 tests with 'git' in their name and I plan to enable them one by one. That way the costs for CI stay managable (my first measurement with one test was 2min 30s), while allowing to take one step at a time.

Custom tests shall be added once we realize that more coverage is needed.

That way we should be able to maintain running `git2` and `gitoxide` side by side until we are willing to switch over to `gitoxide` entirely on stable cargo. Then turning on `git2` might be a feature toggle for a while until we finally remove it from the codebase.

_Please see the above paragraph as invitation for discussion, it's merely a basis to explore from and improve upon._

### Tasks

* [x] add feature toggle
* [x] setup test system with one currently successful test
* [x] implement fetch with `gitoxide` (MVP)
* [x] fetch progress
* [x] detect spurious errors
* [x] enable as many git tests as possible (and ignore what's not possible)
* [x] fix all git-related test failures (except for 1: built-in upload-pack, skipped for now)
* [x] validate that all HTTP handle options that come from `cargo` specific values are passed to `gitoxide`
* [x] a test to validate `git2` code can handle crates-index clones created with `gitoxide` and vice-versa
* [x] remove patches that enabled `gitoxide` enabled testing - it's not used anymore
* [x] ~~remove all TODOs and use crates-index version of `git-repository`~~ The remaining 2 TODO's are more like questions for the reviewer.
* [x] run all tests with gitoxide on the fastest platform as another parallel task
* [x] switch to released version
* [x] [Tasks from first review round](#11448 (comment))
* [x] create a new `gitoxide` release and refer to the latest version from crates.io (instead of git-dependency)
* [x] [address 2nd review round comments](#11448 (comment))

### Postponed Tasks

I suggest to go breadth-first and implement the most valuable features first, and then aim for a broad replacement of `git2`. What's left is details and improved compatibility with the `git2` implementation that will be required once `gitoxide` should become the default implementation on stable to complete the transition.

* **built-in support for serving the `file` protocol** (i.e. without using `git`). Simple cases like `clone` can probably be supported quickly, `fetch` needs more work though due to negotiation.
* SSH name fallbacks via a native (probably ~~libssh~~ (avoid LGPL) `libssh2` based) transport. Look at [this issue](#2399) for some history.
* additional tasks from [this tracking issue](GitoxideLabs/gitoxide#450 (comment))

### Proposed Workflow

I am now using [stacked git](https://stacked-git.github.io) to keep commits meaningful during development. This will also mean that before reviews I will force-push a lot as changes will be bucketed into their respective commits.

Once review officially begins I will stop force-pushing and create small commits to address review comments. That way it should be easier to understand how things change over time.

Those review-comments can certainly be squashed into one commit before merging.

_Please let me know if this is feasible or if there are other ways of working you prefer._

### Development notes

* unrelated: [this line](https://github.com/rust-lang/cargo/blob/9827412fee4f5a88ac85e013edd954b2b63f399b/src/cargo/ops/registry.rs#L620) refers to an issue that has since been resolved in `curl`.
* Additional tasks related to a correct fetch implementation are collected in this [tracking issue](GitoxideLabs/gitoxide#450). **These affect how well the HTTP transport can be configured, needs work**
* _authentication_ [is quite complex](https://github.com/rust-lang/cargo/blob/37cad5bd7f7dcd2f6d3e45312a99a9d3eec1e2a0/src/cargo/sources/git/utils.rs#L490) and centred around making SSH connections work. This feature is currently the weakest in `gitoxide` as it simply uses `ssh` (the program) and calls it a day.  No authentication flows are supported there yet and the goal would be to match `git` there at least (which it might already do by just calling `ssh`). Needs investigation. Once en-par with `git` I think `cargo` can restart the whole fetch operation to try different user names like before.
   - the built-in `ssh`-program based transport can now understand permission-denied errors, but the capability isn't used after all since a builtin ssh transport is required.
* It would be possible to implement `git::Progress` and just ignore most of the calls, but that's known to be too slow as the implementation assumes a `Progress::inc()` call is as fast as an atomic increment and makes no attempt to reduce its calls to it.
* learning about [a way to get custom traits in `thiserror`](dtolnay/thiserror#212) could make spurious error checks nicer and less error prone during maintenance. It's not a problem though.
* I am using `RUSTFLAGS=--cfg` to influence the entire build and unit-tests as environment variables didn't get through to the binary built and run for tests.

### Questions

* The way `gitoxide` is configured the user has the opportunity to override these values using more specific git options, for example using url specific http settings. This looks like a feature to me, but if it's not `gitoxide` needs to provide a way to disable applying these overrides. Please let me know what's desired here - my preference is to allow overrides.
* `gitoxide` currently opens repositories similar to how `git` does which respects git specific environment variables. This might be a deviation from how it was before and can be turned off. My preference is to see it as a feature.

### Prerequisite PRs

* #11602
@dtolnay
Copy link
Owner

dtolnay commented Nov 10, 2024

It does not seem that this is a scenario different users are commonly encountering. I think it's fine to leave impl Error for FailsError as something that needs to be handwritten rather than using the derive for it.

@dtolnay dtolnay closed this as completed Nov 10, 2024
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

No branches or pull requests

2 participants