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

Consider relaxing the Sized requirement in impl Error for Box<T> #104485

Open
Veetaha opened this issue Nov 16, 2022 · 5 comments
Open

Consider relaxing the Sized requirement in impl Error for Box<T> #104485

Veetaha opened this issue Nov 16, 2022 · 5 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Nov 16, 2022

As for today the following blanket implementation exists:

#[stable(feature = "box_error", since = "1.8.0")]
impl<T: core::error::Error> core::error::Error for Box<T> {
#[allow(deprecated, deprecated_in_future)]
fn description(&self) -> &str {
core::error::Error::description(&**self)
}
#[allow(deprecated)]
fn cause(&self) -> Option<&dyn core::error::Error> {
core::error::Error::cause(&**self)
}
fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
core::error::Error::source(&**self)
}
}

The problem with this is that T is required to be Sized, so, for example, Box<dyn Error> doesn't implement Error. It looks like a straightforward and intuitive assumption that Box<dyn Error> should implement Error trait, but it doesn't.

Going even further, there is an inconsistency between the blanket impls for Arc as well, because there exists an analogous impl for Arc that has relaxed Sized? requirement:

rust/library/alloc/src/sync.rs

Lines 2769 to 2788 in e702534

#[stable(feature = "arc_error", since = "1.52.0")]
impl<T: core::error::Error + ?Sized> core::error::Error for Arc<T> {
#[allow(deprecated, deprecated_in_future)]
fn description(&self) -> &str {
core::error::Error::description(&**self)
}
#[allow(deprecated)]
fn cause(&self) -> Option<&dyn core::error::Error> {
core::error::Error::cause(&**self)
}
fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
core::error::Error::source(&**self)
}
fn provide<'a>(&'a self, req: &mut core::any::Demand<'a>) {
core::error::Error::provide(&**self, req);
}
}

Use Case

The use case where I stumbled with this problem is where I wanted to put Box<dyn Error> as a source error in thiserror error enum, but I couldn't because that type doesn't implement Error trait.

Workarounds

The workaround for this problem is to use Arc<T> instead of Box<T> when an Error impl is required for Sized? type.

@clubby789
Copy link
Contributor

@rustbot label +T-libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 27, 2023
@richard-uk1
Copy link
Contributor

I may be wrong, but I think this is a known issue that can't be fixed because it would break backwards compatibility.

But if I'm wrong will other people please chime in. (It's much quicker to get an answer on the internet by posting something incorrect than by asking a question 😛)

@waynr
Copy link
Contributor

waynr commented Aug 9, 2023

@derekdreery I came across this comment while working on #113464 which supports your backwards compatibility claim: https://github.com/rust-lang/rust/blob/master/library/core/src/error.rs#L409

But I'll admit I don't understand why removing the Sized bound via ?Sized is backwards incompatible and am interested to have that explained more clearly...

Like if you had bounds of MyTrait and YourTrait on a given generic type parameter and a concrete type MyConcreteType that implements both traits but remove the MyTrait bound on the type parameter in question, MyConcreteType should still be valid because it still implements YourTrait -- right? Intuitively, it seems to me that by removing the implicit Sized bound for T in Box<T> via ?Sized any concrete type loitering out there in the wild that is Sized should still be valid for impl<T: core::error::Error + ?Sized> core::error::Error for Box<T>.

I think I just realized the issue here -- it's the downstream usages of Box (ie contexts receiving and using Box such as an error-reporting library like anyhow that rely on it having a size known at compile time) that potentially rely on the Sized bound which would be broken if it's removed, not the Box itself.

@Amanieu
Copy link
Member

Amanieu commented Aug 11, 2023

I believe this is because Box is #[fundamental], which has implications for trait impls: it allows crates to implement traits on Box<MyType> directly, which is not normally allowed for a crate that doesn't own the Box type.

It might still be acceptable to relax the bounds though, if a crater run shows no breakage.

@waynr
Copy link
Contributor

waynr commented Aug 12, 2023

Okay I just tried this out:

zsh/5 10140  (git)-[relax-the-sized-requirement-for-box-dyn-error]-% git diff
diff --git a/library/alloc/src/boxed.rs b/library/alloc/src/boxed.rs
index 8697a77db3b..8ad94ad1eff 100644
--- a/library/alloc/src/boxed.rs
+++ b/library/alloc/src/boxed.rs
@@ -2430,7 +2430,7 @@ fn from(err: Cow<'a, str>) -> Box<dyn Error> {
 }

 #[stable(feature = "box_error", since = "1.8.0")]
-impl<T: core::error::Error> core::error::Error for Box<T> {
+impl<T: core::error::Error + ?Sized> core::error::Error for Box<T> {
     #[allow(deprecated, deprecated_in_future)]
     fn description(&self) -> &str {
         core::error::Error::description(&**self)

But when I try compiling I get

error[E0119]: conflicting implementations of trait `From<Box<dyn core::error::Error>>` for type `Box<dyn core::error::Error>`
    --> library/alloc/src/boxed.rs:2207:1
     |
2207 | impl<'a, E: Error + 'a> From<E> for Box<dyn Error + 'a> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: conflicting implementation in crate `core`:
             - impl<T> From<T> for T;

error[E0119]: conflicting implementations of trait `From<Box<dyn core::error::Error + Send + Sync>>` for type `Box<dyn core::error::Error + Send + Sync>`
    --> library/alloc/src/boxed.rs:2240:1
     |
2240 | impl<'a, E: Error + Send + Sync + 'a> From<E> for Box<dyn Error + Send + Sync + 'a> {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: conflicting implementation in crate `core`:
             - impl<T> From<T> for T;

So impl<T: core::error::Error + ?Sized> core::error::Error for Box<T> now means that Box<dyn Error> is an Error implementation which means that impl<T> From<T> for T overlaps with the impls at line 2240 and 2207 in library/alloc/src/boxed.rs since they are basically now converting possibly from a Box<dyn Error> to a Box<dyn Error>. I find this a little bit confusing.

Is this happening because of monomorphization? At compile time because one possible known implementation of Error is Box<Error + ?Sized> aka Box<dyn Error> which monomorphs (is that a word? is there such thing as monomorphin' time? (bad Power Rangers joke, can't help myself)) to (taking the impl at line 2207 of library/alloc/src/boxed.rs shown in the compiler error as an example) to something like:

impl From<Box<dyn Error +'a>> for Box<dyn Error + 'a> { ... }

And this would also be monomorphed from impl<T> From<T> for T, hence the conflict.

Is there a good way to work around this? Add a Sized bound to lines 2207 and 2240 in library/alloc/src/boxed.rs? When I try that, I get essentially the same error. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants