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

Can't use anyhow when writing custom AssetLoaders or AssetSavers #10350

Closed
Runi-c opened this issue Nov 3, 2023 · 15 comments · Fixed by #10493
Closed

Can't use anyhow when writing custom AssetLoaders or AssetSavers #10350

Runi-c opened this issue Nov 3, 2023 · 15 comments · Fixed by #10493
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Milestone

Comments

@Runi-c
Copy link

Runi-c commented Nov 3, 2023

Bevy version

v0.12.0

What you did

Tried to use anyhow::Error as the value for the associated type Error in my implementation of an AssetLoader, as recommended in #10003.

What went wrong

error[E0277]: the trait bound `anyhow::Error: StdError` is not satisfied
   --> src\asset.rs:111:18
    |
111 |     type Error = anyhow::Error;
    |                  ^^^^^^^^^^^^^ the trait `StdError` is not implemented for `anyhow::Error`
    |
note: required by a bound in `bevy::asset::AssetLoader::Error`
   --> <snip>\bevy_asset\src\loader.rs:31:17
    |
31  |     type Error: std::error::Error + Send + Sync + 'static;
    |                 ^^^^^^^^^^^^^^^^^ required by this bound in `AssetLoader::Error`

Additional information

#10003 suggests that anyhow::Error is intended to be a drop-in replacement here, to make migrations easier. I had to add thiserror as a project dependency just for this one file.

@Runi-c Runi-c added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 3, 2023
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Nov 4, 2023
@deltanedas
Copy link

deltanedas commented Nov 4, 2023

did you do something like this?

#[derive(Error)]
pub enum LoaderError {
    Error(#[from] anyhow::Error)
}

@Runi-c
Copy link
Author

Runi-c commented Nov 4, 2023

@deltanedas Naw I just enumerated the error types since I figured if I'm being made to pull in thiserror I might as well use it as intended.

However it doesn't seem intended that I had to pull in thiserror just to interact with this system in an ergonomic way, and the intention appears to have been for anyhow::Error to be a drop-in replacement, hence it seems to be a bug.

@deltanedas
Copy link

i think its an issue on anyhows end since youd expect to be able to use it for something that wants std error trait

@rparrett
Copy link
Contributor

rparrett commented Nov 6, 2023

@bushrat011899 any idea what's going on here? It's not clear to me whether this is a trait bound issue or if the migration guide suggesting anyhow should be updated.

@deltanedas
Copy link

i looked at anyhow and couldnt see any Error trait implemention, bounds or not.

@Testare
Copy link
Contributor

Testare commented Nov 6, 2023

There's a github issue describing why anyhow::Error doesn't implement std::error::Error: dtolnay/anyhow#25

Looking at the doc, anyhow doesn't implment Error but it has AsRef, Deref, and From implementation (From to StdError) to allow conversion.

@bushrat011899
Copy link
Contributor

There's a github issue describing why anyhow::Error doesn't implement std::error::Error: dtolnay/anyhow#25

Looking at the doc, anyhow doesn't implment Error but it has AsRef, Deref, and From implementation (From to StdError) to allow conversion.

That's a mistake on my part then! When I made that PR, I assumed anyhow::Error would implement Error. I think the simplest option here would be to update the migration guide to suggest using Box<dyn Error + Send + Sync>, or a new type around anyhow::Error.

@Runi-c
Copy link
Author

Runi-c commented Nov 6, 2023

I think the simplest option here would be to update the migration guide to suggest using Box<dyn Error + Send + Sync>, or a new type around anyhow::Error.

@bushrat011899 I did try the first one before I made the issue, but unfortunately the way the trait bounds are set up it seems to want the boxed type to be Sized as well...

error[E0277]: the size for values of type `(dyn StdError + Send + Sync + 'static)` cannot be known at compilation time
   --> src\asset.rs:119:18
    |
119 |     type Error = Box<dyn std::error::Error + Send + Sync>;
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: the trait `Sized` is not implemented for `(dyn StdError + Send + Sync + 'static)`
    = help: the trait `StdError` is implemented for `std::boxed::Box<T>`
    = note: required for `std::boxed::Box<(dyn StdError + Send + Sync + 'static)>` to implement `StdError`
note: required by a bound in `bevy::asset::AssetLoader::Error`
   --> <snip>\bevy_asset\src\loader.rs:31:17
    |
31  |     type Error: std::error::Error + Send + Sync + 'static;
    |                 ^^^^^^^^^^^^^^^^^ required by this bound in `AssetLoader::Error`

For more information about this error, try `rustc --explain E0277`.

I'm not sure how to get a newtype around anyhow::Error to work either without explicitly implementing std::fmt::Display and std::error::Error for the newtype, which is much more tedious than just pulling in thiserror.

@bushrat011899
Copy link
Contributor

I'm not sure how to get a newtype around anyhow::Error to work either without explicitly implementing std::fmt::Display and std::error::Error for the newtype, which is much more tedious than just pulling in thiserror.

Unfortunately I think that's exactly correct. I do apologise for my mistake regarding the migration guide for those relying on anyhow for this associated type. During development I had obviously removed the dependency, and had (falsely) assumed that anyhow::Error was indeed an Error type (which in my defence, I think is a reasonable assumption to make). If it's any consolation, thiserror was already a part of Bevy and thus in your dependency graph from well before this change was made.

@Testare
Copy link
Contributor

Testare commented Nov 7, 2023

I still think we should try to make the associated Error type able to support anyhow::Error in some way, or enable some sort of generic Error handling. As far as I'm aware there's not really any sort of error handling that is type specific for AssetReader, just using the Error trait. Meanwhile, I have at least 4 different AssetReaders in my code currently across different crates, and now I have to create different error types for different ones and make sure that they convert all the errors to the appropriate type.

It might not make it into one of the 0.12.x patches, but supporting anyhow in 0.13 would be ideal

@rlidwka
Copy link
Contributor

rlidwka commented Nov 8, 2023

Meanwhile, I have at least 4 different AssetReaders in my code currently across different crates, and now I have to create different error types for different ones

If you don't care about strong error typing, you can use Box<dyn Error + Send + Sync> as mentioned above.

@Runi-c
Copy link
Author

Runi-c commented Nov 8, 2023

@rlidwka As also mentioned above, that produces an error message currently - the compiler wants the boxed type to be Sized.

@rlidwka
Copy link
Contributor

rlidwka commented Nov 8, 2023

@Roryl-c, okay, sounds like a bug then.

Do you have a minimal example (of a code that worked with anyhow before) that we can try out?

@Runi-c
Copy link
Author

Runi-c commented Nov 8, 2023

@rlidwka My code was originally based on the examples/asset/custom_asset.rs example, which is fairly minimal and used anyhow::Error before #10003.

rlidwka added a commit to rlidwka/bevy that referenced this issue Nov 8, 2023
this fixes bevyengine#10350

Previous change made in bevyengine#10003
resulted in inability for users to have ?Sized errors, which caused
usability issues with crates like anyhow.
rlidwka added a commit to rlidwka/bevy that referenced this issue Nov 8, 2023
this fixes bevyengine#10350

Previous change made in bevyengine#10003
resulted in inability for users to have ?Sized errors, which caused
usability issues with crates like anyhow.
@rafalh
Copy link
Contributor

rafalh commented Nov 10, 2023

I also stepped onto this problem. I wanted to use Box<dyn std::error::Error + Send + Sync> as mentioned above but it does not work because T must be Sized for Box<T> to implement std::error::Error which is a little non-intuitive. It looks like a Rust bug that cannot be solved because of backward compatibility reasons. See: rust-lang/rust#104485
Did anybody found a workaround that does not require to defining Error type for every loader?

rafalh added a commit to rafalh/bevy that referenced this issue Nov 10, 2023
anyhow::Error does not implement std::error::Error but converting to boxed
std::error::Error is possible. At the same time this bound should work fine
with custom error types like ones generated by thiserror.
Note: after this change Bevy 0.12 migration guide claim that anyhow::Error
works in this context becomes true.
Fixes bevyengine#10350
rafalh added a commit to rafalh/bevy that referenced this issue Nov 10, 2023
anyhow::Error does not implement std::error::Error but converting to boxed
std::error::Error is possible. At the same time this bound should work fine
with custom error types like ones generated by thiserror.
Note: after this change Bevy 0.12 migration guide claim that anyhow::Error
works in this context becomes true.
Fixes bevyengine#10350
@nicopap nicopap added this to the 0.12.1 milestone Nov 10, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2023
#10493)

# Objective

* In Bevy 0.11 asset loaders used `anyhow::Error` for returning errors.
In Bevy 0.12 `AssetLoader` (and `AssetSaver`) have associated `Error`
type. Unfortunately it's type bounds does not allow `anyhow::Error` to
be used despite migration guide claiming otherwise. This makes migration
to 0.12 more challenging. Solve this by changing type bounds for
associated `Error` type.
* Fix #10350

## Solution

Change associated `Error` type bounds to require `Into<Box<dyn
std::error::Error + Send + Sync + 'static>>` to be implemented instead
of `std::error::Error + Send + Sync + 'static`. Both `anyhow::Error` and
errors generated by `thiserror` seems to be fine with such type bound.

---

## Changelog

### Fixed
* Fixed compatibility with `anyhow::Error` in `AssetLoader` and
`AssetSaver` associated `Error` type
Shatur pushed a commit to projectharmonia/bevy that referenced this issue Nov 19, 2023
bevyengine#10493)

# Objective

* In Bevy 0.11 asset loaders used `anyhow::Error` for returning errors.
In Bevy 0.12 `AssetLoader` (and `AssetSaver`) have associated `Error`
type. Unfortunately it's type bounds does not allow `anyhow::Error` to
be used despite migration guide claiming otherwise. This makes migration
to 0.12 more challenging. Solve this by changing type bounds for
associated `Error` type.
* Fix bevyengine#10350

## Solution

Change associated `Error` type bounds to require `Into<Box<dyn
std::error::Error + Send + Sync + 'static>>` to be implemented instead
of `std::error::Error + Send + Sync + 'static`. Both `anyhow::Error` and
errors generated by `thiserror` seems to be fine with such type bound.

---

## Changelog

### Fixed
* Fixed compatibility with `anyhow::Error` in `AssetLoader` and
`AssetSaver` associated `Error` type
cart pushed a commit that referenced this issue Nov 30, 2023
#10493)

# Objective

* In Bevy 0.11 asset loaders used `anyhow::Error` for returning errors.
In Bevy 0.12 `AssetLoader` (and `AssetSaver`) have associated `Error`
type. Unfortunately it's type bounds does not allow `anyhow::Error` to
be used despite migration guide claiming otherwise. This makes migration
to 0.12 more challenging. Solve this by changing type bounds for
associated `Error` type.
* Fix #10350

## Solution

Change associated `Error` type bounds to require `Into<Box<dyn
std::error::Error + Send + Sync + 'static>>` to be implemented instead
of `std::error::Error + Send + Sync + 'static`. Both `anyhow::Error` and
errors generated by `thiserror` seems to be fine with such type bound.

---

## Changelog

### Fixed
* Fixed compatibility with `anyhow::Error` in `AssetLoader` and
`AssetSaver` associated `Error` type
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
bevyengine#10493)

# Objective

* In Bevy 0.11 asset loaders used `anyhow::Error` for returning errors.
In Bevy 0.12 `AssetLoader` (and `AssetSaver`) have associated `Error`
type. Unfortunately it's type bounds does not allow `anyhow::Error` to
be used despite migration guide claiming otherwise. This makes migration
to 0.12 more challenging. Solve this by changing type bounds for
associated `Error` type.
* Fix bevyengine#10350

## Solution

Change associated `Error` type bounds to require `Into<Box<dyn
std::error::Error + Send + Sync + 'static>>` to be implemented instead
of `std::error::Error + Send + Sync + 'static`. Both `anyhow::Error` and
errors generated by `thiserror` seems to be fine with such type bound.

---

## Changelog

### Fixed
* Fixed compatibility with `anyhow::Error` in `AssetLoader` and
`AssetSaver` associated `Error` type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Projects
None yet
9 participants