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

std: implement `Error` for `Box<dyn Error>` #58974

Open
wants to merge 1 commit into
base: master
from

Conversation

@seanmonstar
Copy link
Contributor

seanmonstar commented Mar 6, 2019

By using an internal auto trait, we can implement Error for Box<dyn Error>
without causing conflicts for the existing From implementations.

Found a way to get passed the previous problem.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 6, 2019

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Mar 6, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:123d1f14:start=1551896681564999974,finish=1551896755399873792,duration=73834873818
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:03:55]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:03:55]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:03:55]    Compiling rustc-demangle v0.1.10
[00:04:00]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:06] error: type does not implement `fmt::Debug`; consider adding #[derive(Debug)] or a manual implementation
[00:04:06]     |
[00:04:06]     |
[00:04:06] 239 | pub struct IsBoxDynError<T>(T);
[00:04:06]     |
[00:04:06] note: lint level defined here
[00:04:06]    --> src/libstd/lib.rs:210:9
[00:04:06]     |

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

std: implement `Error` for `Box<dyn Error>`
By using an internal auto trait, we can implement `Error` for `Box<dyn Error>`
without causing conflicts for the existing `From` implementations.

@seanmonstar seanmonstar force-pushed the seanmonstar:impl-error-for-box-dyn-error branch from 0562b85 to 79ee19b Mar 6, 2019

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Mar 7, 2019

As I mentioned in the previous PR, I'd normally add "bounds asserts" so that any future edits don't accidentally break impls. Is there a preferred pattern or place to do that in libstd?

/// it, we cannot pass `Box<dyn Error>` to anything asking for `E: Error`.
///
/// To get around the conflict, an auto trait is used, since we can have
/// negative imlementations. Then, the `From<E: Error>` implementation above

This comment has been minimized.

Copy link
@mehcode

mehcode Mar 13, 2019

Contributor

Randomly found a minor spelling error: imlementations.

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Mar 13, 2019

Ping?

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Mar 13, 2019

@seanmonstar Are there any similar uses of auto traits in stable Rust at the moment such that we do not add anything fundamentally new to the language? If not this will require sign-off from the language team and deeper consideration wrt. implications re. the type system.

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Mar 13, 2019

@Centril I'm not aware of anything else in libstd using this kind of trick. If this were merged, and something about how the trick (like auto traits) needed to be changed that would make this no longer work, it would likely require specialization to have progressed enough to allow these overlapping implementations instead.

There is desire by the libs team to have Box<dyn Error>: Error.

(And personally, having just moved several tower middlewares to return a Box<dyn Error>, it's blown up several other places that were just wanting E: Error.)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Mar 13, 2019

@Centril I'm not aware of anything else in libstd using this kind of trick. If this were merged, and something about how the trick (like auto traits) needed to be changed that would make this no longer work, it would likely require specialization to have progressed enough to allow these overlapping implementations instead.

OK, then I'm generally disinclined to allow this special case on stable until such time that we are 100% certain that it can be soundly generalized.

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Mar 13, 2019

OK, then I'm generally disinclined to allow this special case on stable until such time that we are 100% certain that it can be soundly generalized.

If neither of those could no longer satisfy this requirement, then the compiler could add custom support. That Box<dyn Error> doesn't implement Error is quite maddening, and it doesn't due to a mistake by allowing the extra From<E: Error> for Box<dyn Error> implementations before realizing Box<dyn Error> didn't implement Error.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Mar 18, 2019

If neither of those could no longer satisfy this requirement, then the compiler could add custom support.

I'm not OK with adding ad-hoc custom support for just this. Such treatment of the type system is not something I'm in favor of.

That Box<dyn Error> doesn't implement Error is quite maddening, and it doesn't due to a mistake by allowing the extra From<E: Error> for Box<dyn Error> implementations before realizing Box<dyn Error> didn't implement Error.

Maddening as it may be, we should live with the mistake until such time it can be fixed by a general mechanism.

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Mar 18, 2019

We should live with the mistake until such time it can be fixed by a general mechanism.

Seems we don't agree there. That's OK! Still, I'd welcome hearing from others that would need to comment due to the needs-fcp label.

@jonhoo jonhoo referenced this pull request Mar 19, 2019

Merged

Polish call all #198

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Mar 26, 2019

Is there a way I can ping the related teams? Or should I just ping individuals?

@mati865

This comment has been minimized.

Copy link
Contributor

mati865 commented Mar 28, 2019

You cannot ping teams unless you are member of rust-lang project.

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Apr 1, 2019

Very well.

@alexcrichton @SimonSapin @withoutboats I've seen you all to comment before about improvements to Error, or about implementing Error for Box<dyn Error>, so your thoughts would be welcome here.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Apr 2, 2019

This auto trait is not really internal - it appears in the public API. I want to see this impl exist, but I don't think it is a good idea to add these kinds of coherence hacks to std.

Specialization would allow adding this impl without any hacks, but needs more energy behind it for the feature to get implemented soundly. And FWIW, in my opinion the mistake we made was in making ? use From/Into instead of using a special trait just for error conversion and implementing the identity case in the desugaring, so that this problem would have been totally avoided.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 2, 2019

I would personally agree with @withoutboats that this is not internal due to the exact motivation of the PR, enabling something that's been long desired. Building something so fundamental on unstable features that have a questionable stabilization path and deep implications in the compiler make me pretty uneasy. I think that we'll want to hold off on this until more around specialization develops to see if that's a viable strategy, as well as more developments on the failure-in-libstd story

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Apr 5, 2019

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Apr 5, 2019

Team member @Centril has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Apr 5, 2019

This auto trait is not really internal - it appears in the public API.

I can put these pieces in an internal module so that they are truly private, if that's the concern. Or did you mean as Alex did, that this is using an auto trait to allow an addition to the "public" API?

Building something so fundamental on unstable features that have a questionable stabilization path and deep implications in the compiler make me pretty uneasy.

I can appreciate that, I was torn on whether to open this PR or not. However, I'd like to be absolutely certain as to the reason for closing this: is it specifically because this uses an unstable feature (auto traits) to allow constructing an impl that would be part of the public contract, and if something unsound were later found in auto traits, this additional impl could slow down fixing that?

There are several "stable", "public" APIs in libstd that were built off unstable features:

  • Box uses box internally.
  • Rc<str>, Arc<[u8]>, etc were implemented on top of the unstable CoerceUnsized trait.
  • std::marker::Unsized and std::marker::Freeze
  • #[fundamental]
  • It seems likely async will stabilize while using the unstable Generator.

I'm sure there are others, and while some are less relevant, I'd prefer to identify what exactly is different.


I feel like this mechanism is quite self-contained, and likely any other method of providing this impl will be much bigger. Still, if that's the case, then what can we do to implement this soon? Waiting another year or more for stabilization hurts a lot, I'm happy to implement something special in the compiler instead.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Apr 6, 2019

There are several "stable", "public" APIs in libstd that were built off unstable features:
[…]
I'd prefer to identify what exactly is different.

For at least some of those, what’s different is that the unstable features are used in implementation details and are not "visible" in the public API surface.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Apr 6, 2019

In principle I wouldn't mind landing an implementation like this using an auto trait. I consider it more valuable to have the desired impl work than to attempt to keep the long list of impls under From's documentation free of unstable details.

As specialization matures, would it be backward compatible to transition the implementation to use that instead while maintaining the same set of impls as in this PR?

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Apr 6, 2019

Be aware that this PR as written is a breaking change:

use std::error::Error;

// used to work
fn f<E: Error + 'static>(e: E) -> Box<dyn Error> {
    e.into()
}
error[E0277]: the trait bound `E: std::error::NotBoxDynError` is not satisfied in `std::error::IsBoxDynError<E>`
 --> src/main.rs:5:7
  |
5 |     e.into()
  |       ^^^^ within `std::error::IsBoxDynError<E>`, the trait `std::error::NotBoxDynError` is not implemented for `E`
  |
  = help: consider adding a `where E: std::error::NotBoxDynError` bound
  = note: required because it appears within the type `std::error::IsBoxDynError<E>`
  = note: required because of the requirements on the impl of `std::convert::From<E>` for `std::boxed::Box<dyn std::error::Error>`
  = note: required because of the requirements on the impl of `std::convert::Into<std::boxed::Box<dyn std::error::Error>>` for `E`
@seanmonstar

This comment has been minimized.

Copy link
Contributor Author

seanmonstar commented Apr 9, 2019

@dtolnay

Be aware that this PR as written is a breaking change

Wow, excellent catch! I'd been working with this playground, and the compiler was fine with Imp.into(), since it had enough knowledge to work out it implements Error while not being a Box<dyn Error>. But once we make a generic function where the only knowledge is that E: Error, the compiler can no longer be certain of all those bounds. That's disappointing, but understandable.

I'll be jumping back into the playground trying some more things, but it might be impossible with just an auto trait.


An alternative is if we could add an attribute or something to the impl<T> From<T> for T essentially saying "this is always the most specific impl". I realize that's likely controversial, but I'm deeply motivated to do all the work to get it through...

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Apr 9, 2019

@seanmonstar

An alternative is if we could add an attribute or something to the impl From for T essentially saying "this is always the most specific impl". I realize that's likely controversial, but I'm deeply motivated to do all the work to get it through...

Others might have thoughts, but so long as specializing that impl was restricted to the standard library, I'd feel a lot more comfortable with that then the current PR, as it lines up more closely with things that we'd like to have be possible using specialization. It also has other uses such as impl<T> From<!> for T.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Apr 17, 2019

I'm not inclined to support the implementation here by anything other than a stable and general mechanism. In particular, I'm not at all comfortable with the assumption that "specialization will remove the hack later when it gets stable". It feels like a high risk proposition to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.