Skip to content

accept that box dyn error is not and will never be an error #92862

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

Closed
wants to merge 1 commit into from

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Jan 13, 2022

r? @dtolnay since this is also relevant to you and anyhow.

I'm adding this negative impl to make it so I can eventually add the following impl to eyre:

    impl StdError for Box<dyn std::error::Error + Send + Sync + 'static> {
        fn ext_report<D>(self, msg: D) -> Report
        where
            D: Display + Send + Sync + 'static,
        {
            self.wrap_err(msg)
        }
    }

Which lets me call .wrap_err on Result<T, Box<dyn Error + Send + Sync + 'static>>. This is currently impossible because it produces a future incompatibility error:

error[E0119]: conflicting implementations of trait `context::ext::StdError` for type `std::boxed::Box<(dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static)>`
  --> src/context.rs:30:5
   |
18 | /     impl<E> StdError for E
19 | |     where
20 | |         E: std::error::Error + Send + Sync + 'static,
21 | |     {
...  |
27 | |         }
28 | |     }
   | |_____- first implementation here
29 | 
30 |       impl StdError for Box<dyn std::error::Error + Send + Sync + 'static> {
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::boxed::Box<(dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static)>`
   |
   = note: upstream crates may add a new impl of trait `std::error::Error` for type `std::boxed::Box<(dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static)>` in future versions

For more information about this error, try `rustc --explain E0119`.
error: could not compile `eyre` due to previous error

But this error is incorrect because we cannot add the proper Error impl to Box<dyn Error> without lattice specialization, which is not happening any time soon, if ever. Adding this negative impl promises to never impl Error for box dyn error in the future, so this will need an FCP.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2022
@yaahc
Copy link
Member Author

yaahc commented Jan 13, 2022

Hmm, for some reason it did not work, I tried testing the change in eyre with a locally built toolchain and it just slightly changed the error msg instead of allowing the impl...

eyre on  helpful [$!] is 📦 v0.6.5 via 🦀 v1.60.0-dev on ☁️  [email protected] 
 cargo build
   Compiling eyre v0.6.5 (/home/jlusby/git/yaahc/eyre)
   Compiling once_cell v1.8.0
   Compiling indenter v0.3.3
error[E0119]: conflicting implementations of trait `context::ext::StdError` for type `std::boxed::Box<(dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static)>`
  --> src/context.rs:30:5
   |
18 | /     impl<E> StdError for E
19 | |     where
20 | |         E: std::error::Error + Send + Sync + 'static,
21 | |     {
...  |
27 | |         }
28 | |     }
   | |_____- first implementation here
29 | 
30 |       impl StdError for Box<dyn std::error::Error + Send + Sync + 'static> {
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::boxed::Box<(dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static)>`

For more information about this error, try `rustc --explain E0119`.
error: could not compile `eyre` due to previous error

cc @spastorino, can you check if this is a bug in the current impl of rust-lang/negative-impls-initiative#1 and if so use it as a testcase?

@m-ou-se m-ou-se added needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 13, 2022
@yaahc
Copy link
Member Author

yaahc commented Jan 19, 2022

As a minor update, I talked to niko today and he mentioned that he doesn't think that lattice specialization is likely to never happen. Given that I don't think we should be making this commitment, at least not until we've completely ruled out lattice specialization as a possible feature. I'm going to close this for now though I still would like to see if this can be made to work with negative-impls incase we ever decide to add it again in the future.

@yaahc yaahc closed this Jan 19, 2022
@yaahc yaahc deleted the box-dyn-error-acceptance branch January 19, 2022 02:15
@yaahc yaahc restored the box-dyn-error-acceptance branch January 26, 2022 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants