GEN-150: ResultExt doesn't cover Report<EyreReport> for some reason

[0x009922]

Describe the bug

For a reason I haven't yet understood, ResultExt trait doesn't work on error_stack::Result (e.g. change_context is not found), although EyreReport seems to satisfy the Context trait.

However, it is possible to call Report::change_context on it without a problem.

To reproduce

use error_stack::{IntoReportCompat, ResultExt};
 
fn eyre_result() -> eyre::Result<()> {
    Err(eyre::eyre!("Something happened"))
}
 
#[derive(Debug, thiserror::Error)]
#[error("my error occurred")]
struct MyError;
 
fn main() -> error_stack::Result<(), MyError> {
    // doesn't compile
    eyre_result()
        .into_report()
        .change_context(MyError)?;
 
    // fine
    eyre_result()
        .into_report()
        .map_err(|report| report.change_context(MyError))?;
 
 
    Ok(())
}

https://github.com/0x009922/error-stack-eyre-compat-issue

Expected behavior

Expected .into_report().change_context(...) to work

Rust compiler

1.76.0 (07dca489a 2024-02-04)

Host

aarch64-apple-darwin

Target

aarch64-apple-darwin

Version

0.4.1

Features

default, eyre

Additional context

Compiler error:

error[E0599]: no method named `change_context` found for enum `Result` in the current scope
   --> src/main.rs:15:10
    |
13 | / eyre_result()
14 | | .into_report()
15 | | .change_context(MyError)?;
    | | -^^^^^^^^^^^^^^ method not found in `Result<(), Report>`
    | |_________|
    |
    |
note: the method `change_context` exists on the type `error_stack::Report`
   --> [redacted]/error-stack-0.4.1/src/report.rs:462:5
    |
462 | / pub fn change_context(mut self, context: T) -> Report
463 | | where
464 | | T: Context,
    | |___________________^
help: use the `?` operator to extract the `error_stack::Report` value, propagating a `Result::Err` value to the caller
    |
14 | .into_report()?
    | +
 
 

[TimDiekmann]

Hi @0x009922 and thanks for opening the issue.
I will look into this, it feels wrong that the map_err is working but the trait extension on Result does not.

[TimDiekmann]

I investigated this further:

• Report::new(error: E) does require E: Context. Neither eyre::Report, nor anyhow::Error implement Context (they don't implement Error). This means, it's not possible to construct Report<eyre::Report> from new.
• Over the time, the restrictions on Report shifted a little bit (we had a lot of unsafe code in error-stack and I was able to remove almost all of it (only a very few locations use unsafe, e.g. for async or repo(transparent) conversions). For that, I added an internal trait FrameImpl, which is implemented by C: Context and wrapper structs for eyre::Report and anyhow::Error
• When calling eyre_result().into_report(), a Report<eyre::Report> is created, however, without implementing C: Context for the resulting Report<C>
• Report<C>::change_context does not require C: Context, however, the associated type ResultExt::Context requires Context, so does C in impl ResultExt for Result<T, Report<C>>

I see three options here. In any case, we should change something, any of these changes is a breaking change.

• Explicitly allow C: !Context in Report. Implications:
  • Report::new would still require Context for internals
  • Remove the bound ResultExt::Context: Context and thus remove the C: Context in the ResultExt for Result<T, Report<C>>
  • Removing a trait bound on an associated type is a breaking change
  • When removing Context in favor of core::error::Error this might become an issue due to the orphan rule, need to test this
• Make IntoReportCompat::into_report() return a wrapper struct around eyre::Report which implements Context. Implications:
  • Report<eyre::Report> wouldn't be a thing anymore even though it is intuitive
  • C: Report is enforced for any Report<C>
• Drop the support for eyre and anyhow

I like the first solution the most, however, I'm not sure about the implications. @indietyp what are your thoughts on this?

[TimDiekmann]

I had a quick call with @indietyp and we agreed that the first option is sub optimal due to the implications. Being able to enforce C: Context inside of Report<C> is quite nice. We also discovered another option: Utilizing Box<dyn Error + Send + Sync>. Both, eyre::Report and anyhow::Error can be converted into that type and we could remove the feature flags if it's possible to create a blanket implementation. I'm going to discover this more and if this is not going to work as expected we'll go with the second approach.