Skip to content

Conversation

@bltavares
Copy link
Member

@bltavares bltavares commented Feb 26, 2020

Choose one: a 🙋 feature

This PR moves from failure crate into a boxed std::error::Error using the anyhow.

Needs

Checklist

  • tests pass
  • tests and/or benchmarks are included
  • documentation is changed or added

Context

Semver Changes

major/minor as a breaking changing on the API.


async fn append<T>(feed: &mut Feed<T>, content: &[u8])
where
T: RandomAccess<Error = Box<dyn std::error::Error + Send + Sync>> + Debug,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe define an alias in the crate root?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wanted to do that, but trait alias are only available on nightly:

error[E0658]: trait aliases are experimental
 --> examples\async.rs:6:1
  |
6 | trait Storage = RandomAccess<Error = Box<dyn std::error::Error + Send + Sync>> + Debug + Send;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/41517

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open an issue to track rust-lang/rust#41517 and move it on after this lands.

);

self.data.write(range.start, data)
self.data.write(range.start, data).map_err(|e| anyhow!(e))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ? will map the error for you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I remember why I've done this .map_err.

My previous attempt to use ? led me to unsized types:

error[E0277]: the size for values of type `dyn std::error::Error + std::marker::Send + std::marker::Sync` cannot be known at compilation time
  --> src\storage\mod.rs:80:59
   |
80 |         instance.bitfield.write(0, &header.to_vec()).await?;
   |                                                           ^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `dyn std::error::Error + std::marker::Send + std::marker::Sync`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required because of the requirements on the impl of `std::error::Error` for `std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>`
   = note: required because of the requirements on the impl of `std::convert::From<std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>>` for `anyhow::Error`
   = note: required by `std::convert::From::from`

It seems that anyhow::Error does not implement std::error::Error directly. This issue dtolnay/anyhow#63 led me to attempt calling .into(), which also failed:

error[E0284]: type annotations needed
  --> src\storage\mod.rs:80:60
   |
80 |         instance.bitfield.write(0, &header.to_vec()).await.into()?;
   |         ---------------------------------------------------^^^^--
   |         |                                                  |
   |         |                                                  cannot infer type
   |         this method call resolves to `T`
   |
   = note: cannot resolve `<_ as std::ops::Try>::Ok == _`

Which also didn't work even with a type specification:

error[E0277]: the trait bound `std::result::Result<(), anyhow::Error>: std::convert::From<std::result::Result<(), std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>>>` is not satisfied
  --> src\storage\mod.rs:83:93
   |
83 |         let result: anyhow::Result<()> = instance.bitfield.write(0, &header.to_vec()).await.into();
   |                                                                                             ^^^^ the trait `std::convert::From<std::result::Result<(), std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>>>` is not implemented for `std::result::Result<(), anyhow::Error>`
   |
   = note: required because of the requirements on the impl of `std::convert::Into<std::result::Result<(), anyhow::Error>>` for `std::result::Result<(), std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>>`

There is an upstream issue documenting this conversion error dtolnay/anyhow#66, but I'm not sure if it will fixed, as there is a conflict on the impl of the conversions as documented on the previous discussion.

That led me to .map_err and it compiled without needing to implement a local structure to wrap stderr.

pub fn write_data(&mut self, offset: usize, data: &[u8]) -> Result<()> {
self.data.write(offset, &data)
pub fn write_data(&mut self, offset: u64, data: &[u8]) -> Result<()> {
self.data.write(offset, &data).map_err(|e| anyhow!(e))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.data.write(offset, &data).map_err(|e| anyhow!(e))
Ok(self.data.write(offset, &data)?)

@bltavares bltavares force-pushed the failure-into-stderr branch from 9df21cb to 5c6a73e Compare March 3, 2020 01:45
@bltavares bltavares requested a review from yoshuawuyts March 3, 2020 01:45
@bltavares bltavares marked this pull request as ready for review March 3, 2020 01:45
@bltavares bltavares force-pushed the failure-into-stderr branch from 5c6a73e to c51c22f Compare March 3, 2020 02:21
@Frando Frando mentioned this pull request May 16, 2020
5 tasks
@bltavares bltavares merged commit 26dcb76 into datrs:master May 17, 2020
@bltavares bltavares deleted the failure-into-stderr branch May 17, 2020 13:40
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.

2 participants