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

eyre!(Into::<Box<dyn StdError + Send + Sync>>::into(report)) discards original spantrace, etc #56

Open
lilyball opened this issue Aug 3, 2021 · 2 comments

Comments

@lilyball
Copy link

lilyball commented Aug 3, 2021

It looks like the only way to convert from Box<dyn StdError + Send + Sync + 'static> into eyre::Report is via the eyre!() macro. It seems odd that there's no other way, but that aside, this means of conversion does not retain the spantrace, backtrace, or any custom color-eyre sections. It does preserve the Display impl and the source chain. I haven't tested but I assume it preserves downcast casting as well.

My guess here is that report.into() throws away that data and produces the underlying boxed error (and that Report::wrap_err() wraps the underlying error). But this is rather unfortunate.

For context, I'm trying to write library functions that take error-returning closures, need to erase the error types, and want to allow callers to use eyre::Report on both ends of the function. I was hoping that declaring the function as using Into<Box<StdError + Send + Sync + 'static>> would work for this, but this issue means callers will lose all of the custom eyre::Report info.

Here's a toy sample library function:

fn library_function<E, E2>(
    mut closure1: impl FnMut() -> Result<(), E>,
    mut closure2: impl FnMut() -> Result<(), E2>,
) -> Result<(), Box<dyn StdError + Send + Sync + 'static>>
where
    E: Into<Box<dyn StdError + Send + Sync + 'static>>,
    E2: Into<Box<dyn StdError + Send + Sync + 'static>>,
{
    closure1().map_err(Into::into)?;
    closure2().map_err(Into::into)?;
    Ok(())
}

We need to homogenize the error types, hence Box<dyn StdError + Send + Sync + 'static>. If someone calls this like eyre!(library_function(fn_that _returns report, fn_that_returns_report)) then the resulting eyre::Report declares its location to be the call to library_function() and discards color-eyre sections.

Also worth noting, the library in question doesn't know anything about eyre, it just wants to play nicely with clients that use it.

@lilyball
Copy link
Author

lilyball commented Aug 3, 2021

I'm considering now that my library function should just take Box<dyn StdError + Send + Sync + 'static>, because the use of Into here gives me ambiguity if I never constrain the error otherwise. But the issue remains, a caller should be able to say eyre!(library_function(|| Err(eyre!("foo").into()), || Err(eyre!("bar").into()))) and get back the original "foo" report.

@yaahc
Copy link
Collaborator

yaahc commented Aug 3, 2021

Yea, that's to be expected with the current design which mirrors anyhow and throws away the additional context and returns the inner error when converting to a boxed error. We could wrap the error and erase that wrapper which could then be checked for with a downcast when constructing a Report from a boxed error, but I'm worried about backwards compatibility issues with such a change.

The issues being either we change the type that Into produces when we convert into a boxed error which breaks existing downcasts or we leave the current discarding Into impl and add alternative conversion functions and it's easy to accidentally discard info still which is probably not ideal. I'm also hesitant to do a major version bump because it tends to cause breakage when multiple versions of eyre get pulled in as deps, along with the fact that the sort of runtime breakage we're considering is likely to remain unfixed after a version bump unless the users read the changelog and carefully audit their code..

Not really sure how best to resolve this.

pksunkara pushed a commit that referenced this issue Oct 11, 2023
* (cargo-release) start next development iteration 0.5.3-alpha.0

* bump version in lib.rs too
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

No branches or pull requests

2 participants