Proof of concept of dyn dispatch based report handler #97

yaahc · 2020-06-06T18:54:35Z

No description provided.

tests/test_handler.rs

dtolnay

This looks great -- I am optimistic about the approach. I think I wouldn't want to land this in anyhow right away but I would be on board with accepting something like this after iterating on the specific signatures in a separate crate and having some built out ecosystem of those handlers to ensure we've covered the use cases that this is intended to cover.

dtolnay · 2020-06-21T19:46:41Z

On the backtrace() function -- one thing I would like in std is a constructor on Backtrace to cheaply make a backtrace with Disabled status. Does that help here? That can be the backtrace if one is not available from the handler.

yaahc · 2020-06-21T20:30:07Z

On the backtrace() function -- one thing I would like in std is a constructor on Backtrace to cheaply make a backtrace with Disabled status. Does that help here? That can be the backtrace if one is not available from the handler.

That sounds like a great approach

yaahc · 2020-06-21T20:46:24Z

This looks great -- I am optimistic about the approach. I think I wouldn't want to land this in anyhow right away but I would be on board with accepting something like this after iterating on the specific signatures in a separate crate and having some built out ecosystem of those handlers to ensure we've covered the use cases that this is intended to cover.

Sounds reasonable. I'm in the process of applying this change to eyre right now eyre-rs/eyre#29, which should hopefully provide sufficient dogfooding of the design to prove that it works and iron out all of the kinks.

Once I'm done making the changes in eyre and color-eyre I'll finalize this PR and release an anyhow based version of color-eyre that provides all the same features on top of anyhow::Error.

yaahc · 2020-07-18T19:18:06Z

This has been in eyre for a few weeks now and has been working great. I'm going to move forward with cleaning up this PR and then I'll write an anyhow based version of color-eyre to leverage the new feature.

yaahc · 2020-07-18T19:33:08Z

Because there's not currently an API for capturing the disabled backtrace I'm going to go ahead and add a required fn on the ReportHandler:

    fn backtrace(&self, error: &(dyn StdError + 'static)) -> &crate::backtrace::Backtrace;

That way any customizers will just have to implement this properly or explicitly panic if they don't want to support backtraces, then in the future we can change this to a default trait fn that returns a reference to a statically captured disabled backtrace if the source error didn't capture one.

dtolnay · 2020-07-18T19:59:22Z

Sounds good to me.

Could you make sure to publish release notes or a changelog for eyre, including retroactively for the past couple releases if possible? I noticed there have been a couple breaking changes quite recently and couldn't find a writeup of what changed, which would help me evaluate how baked this is. Separately, from skimming crates.io it looks like the majority of the downstream crates not written by you are using old versions of eyre i.e. without this implementation; ideally we'd make sure they are able to upgrade and are happy with the new design.

yaahc · 2020-07-18T20:05:57Z

Could you make sure to publish release notes or a changelog for eyre, including retroactively for the past couple releases if possible?

Sure! I can tell you now what the recent breaking changes are. The most recent one was removing the generic parameter in favor of the trait object approach. The one before that was to fix a bug in the eyre! macro where I'd incorrectly implemented the kind lookup for E: Error so it was not generic and immediately resolved to the default parameter provided by eyre, which broke usage with color_eyre if you tried to do something like Err(eyre!(e)).suggestion("foo").

yaahc · 2020-07-18T20:07:39Z

Separately, from skimming crates.io it looks like the majority of the downstream crates not written by you are using old versions of eyre i.e. without this implementation; ideally we'd make sure they are able to upgrade and are happy with the new design.

I've had a couple of upgrade reports, the biggest issue so far has been people forgetting to add the color_eyre::install() and thinking the functionality broke in the new version. I'll check out the other reverse deps and see if I can't get more of them to upgrade and report back on how it went.

yaahc · 2020-07-18T20:29:46Z

src/lib.rs

@@ -584,15 +588,142 @@ pub trait Context<T, E>: context::private::Sealed {
        F: FnOnce() -> C;
 }

+static HOOK: OnceCell<ErrorHook> = OnceCell::new();


lmk if you want me to move this all to a handler.rs file or something

yaahc · 2020-07-18T20:31:24Z

src/lib.rs

+}
+
+#[cfg(not(feature = "std"))]
+trait ReportHandler: core::any::Any + Send + Sync {


I couldn't find a better way to make this trait conditionally pub, we can't export this trait under no_std because the StdError trait is private but we still need to use this trait to format the error reports via the DefaultHandler when in no_std.

yaahc · 2020-07-18T20:35:06Z

src/lib.rs

+    }
+}
+
+#[cfg(feature = "std")]


I think we might want to add some doc_cfg's for all these publicly exported std only apis.

yaahc · 2020-07-18T20:37:48Z

src/fmt.rs

+
+impl crate::ReportHandler for crate::DefaultHandler {
+    #[cfg(backtrace)]
+    fn backtrace<'a>(


I'm not sure we can have a publicly exported method that depends on a non feature cfg, I'll make sure to test this when I port color-eyre to depend on this branch

yaahc · 2020-07-18T20:56:13Z

https://github.com/yaahc/color-anyhow/actions/runs/174054634 seems to be working ^_^

Edit: JK, it doesn't work for the fn backtrace on nightly, as I feared. It looks like the cfg(backtrace) doesn't transitively apply to color-anyhow just by depending on anyhow, sad day. I'm pretty sure I can solve this by vendoring the build.rs logic from anyhow and applying the same approach it uses to feature gate that fn, but I'm not confident this is a good way to solve this issue.

yaahc · 2020-07-18T21:54:09Z

I've run into another snag with implenting backtrace support in color-anyhow. by default color-anyhow always captures a backtrace, even on stable via backtrace-rs, where it then uses the frame api provided in that crate to iterate over the frames and produce the nice colorful backtrace. I wanted to maintain that support so I figured I'd actually use backtrace only on stable and use std::backtrace::Backtrace and btparse on nightly to get the frames from the debug impl and maintain the nice color backtrace output.

Here's the problem though...

Apparently you can't do optional deps based on cfg values, only on features 😢.

I can work around this by copying the backtrace build.rs file once again into btparse to make it conditionally compile the entire API, but I'll still end up with an unused backtrace dep on nightly :/

yaahc · 2020-07-18T22:19:37Z

Well, with sufficient application of build.rs files I got it all working, I now have a full version of color-eyre working ontop of anyhow that even uses std::backtrace::Backtrace on nightly and gets colorized backtraces! This is extremely cool.

https://github.com/yaahc/color-anyhow/actions/runs/174105901

yaahc · 2020-07-18T22:43:02Z

@mystor raised some pretty good concerns about the #[cfg(backtrace)] public trait fn and requiring implementors of ReportHandler to vendor the build.rs logic to correctly implement the trait being fragile and probably not good, so I'm going to go ahead and open a PR to libstd to add a Backtrace::disabled() fn

yaahc · 2020-07-22T19:28:39Z

Regarding the earlier question about how "Baked" the design is. One area where I don't think its fully baked yet is how it interacts with #[track_caller]. I recently tried using it on beta and ended up finding a miscompilation bug when using track caller on function calls through trait objects. Ideally I'd like to just support #[track_caller] on custom handlers, where they will grab the callsite on their own and we never have to interact with it other than attributing the relevant fns in anyhow to get it to forward the information along. That way customized error reports can be more consistent with panic reports which already capture the location they were constructed by default.

My worry is that if it isn't possible to just pass the location all the way into the custom handler via #[track_caller] attributes we may need to manually pass across the trait boundary which would require some tweaks to the API. This can be done backwards compatibly but since we've not yet committed to an API for anyhow I think we might want to consider API changes from eyre if it means we get a better interface.

My existing ideas for working around the issue (if it indeed ends up being an issue) include either:

(breaking change) changing the Hook signature from Fn(&self, error: &(dyn Error + 'static)) -> Box<dyn ReportHandler> too Fn(&self, error(&dyn Error + 'static), location: &Location<'static>) -> Box<dyn ReportHandler>
(backwards compatible) Adding a fn track_caller(&mut self, location: &Location<'static>) { } fn to the ReportHandler trait that is defaulted to just discarding the location, where the implementors can customize the impl to save the location, then anyhow would first construct the context then try passing in the location every time it constructs a new anyhow::Error.
(backwards compatible) Add a second set_hook fn that takes a closure with the second signature from the first bullet instead of the original signature, so downstream crates could opt into the new construction API but the old API is still supported.

@dtolnay

add a Backtrace::disabled function Based upon @dtolnay's suggestion here: dtolnay/anyhow#97 (comment)

yaahc · 2020-11-17T19:26:59Z

Status update

I've had track_caller in eyre for a while now, been working great no issues there, and a breaking change was not necessary. Other than that for a while now I've been thinking about how this design should integrate with libraries that want to use eyre::Report or anyhow::Error as their error type, such as tide/http-types, and was worried some more changes might be necessary to better support these libraries. However recently I came to the conclusion that we don't need to add special support for these kinds of use cases, since they could be built on top of the existing abstractions, and nobody is actually asking for such support as far as I can tell.

More details can be found here eyre-rs/eyre#35 (comment)

All in all I'm confident in the current design and would like to move forward with finalizing this PR, getting it up to date and consistent with eyre, and addressing any final blockers.

Ecosystem Compatibility

That said, I do have one extra goal that I want to aim for. Ideally, I'd like custom hook libraries like color-eyre to be compatible with both eyre and anyhow. Would you be open to creating another dependency just for the trait interface, set_hook and get_hook logic so hooks and handlers defined by any crate work with both reporting types?

vultix · 2021-07-26T01:24:44Z

@yaahc @dtolnay What's the status on this PR? There’s a bit of an ecosystem split between anyhow and eyre and this seems like a great way to help unify the two

yaahc · 2021-07-26T18:29:38Z

@yaahc @dtolnay What's the status on this PR? There’s a bit of an ecosystem split between anyhow and eyre and this seems like a great way to help unify the two

Status is more or less that we need to resolve the following blockers:

Figure out if we're going to create a shared dependency to define the Handler trait
- Assuming yes, and reading backscroll it looks like David already suggested doing so, update eyre to use this shared handler dep
Update PR to resolve merge conflicts
Come to a consensus that anyhow would like to merge this

rongcuid · 2023-07-10T19:26:17Z

Is this still possible?

Proof of concept of dyn dispatch based report handler

125142e

yaahc mentioned this pull request Jun 6, 2020

RFC: Add support for customizable error handlers based on eyre #96

Open

try to setup proof of concept of downcast stuff

bdb37be

yaahc commented Jun 6, 2020

View reviewed changes

tests/test_handler.rs Outdated Show resolved Hide resolved

insert real syntax that was expected to work

9d734ff

yaahc commented Jun 6, 2020

View reviewed changes

tests/test_handler.rs Outdated Show resolved Hide resolved

yaahc added 2 commits June 6, 2020 12:25

fix downcasting

24ee496

add comments

89cb798

yaahc mentioned this pull request Jun 21, 2020

remove generic parameter for handling and replace with a global handler hook eyre-rs/eyre#28

Closed

dtolnay reviewed Jun 21, 2020

View reviewed changes

refactor backtrace logic

5b36a0d

yaahc added 4 commits July 18, 2020 13:17

hmmmmmm

e7919fb

back to defaults

612e25e

a little closer

3489703

cleanup warnings

e65b873

yaahc commented Jul 18, 2020

View reviewed changes

yaahc requested a review from dtolnay July 18, 2020 20:36

yaahc commented Jul 18, 2020

View reviewed changes

yaahc mentioned this pull request Jul 18, 2020

add a Backtrace::disabled function rust-lang/rust#74490

Merged

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 23, 2020

Rollup merge of rust-lang#74490 - yaahc:disabled-bt, r=dtolnay

f98c77c

add a Backtrace::disabled function Based upon @dtolnay's suggestion here: dtolnay/anyhow#97 (comment)

yaahc mentioned this pull request Sep 29, 2020

Implement track caller support for eyre::Report eyre-rs/eyre#36

Merged

dan-da mentioned this pull request Dec 6, 2022

crates.io? yaahc/color-anyhow#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept of dyn dispatch based report handler #97

Proof of concept of dyn dispatch based report handler #97

yaahc commented Jun 6, 2020

dtolnay left a comment

dtolnay commented Jun 21, 2020

yaahc commented Jun 21, 2020

yaahc commented Jun 21, 2020

yaahc commented Jul 18, 2020

yaahc commented Jul 18, 2020 •

edited

dtolnay commented Jul 18, 2020

yaahc commented Jul 18, 2020

yaahc commented Jul 18, 2020

yaahc Jul 18, 2020

yaahc Jul 18, 2020

yaahc Jul 18, 2020

yaahc Jul 18, 2020

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 18, 2020

yaahc commented Jul 22, 2020 •

edited

yaahc commented Nov 17, 2020 •

edited

vultix commented Jul 26, 2021

yaahc commented Jul 26, 2021 •

edited

rongcuid commented Jul 10, 2023

Proof of concept of dyn dispatch based report handler #97

Are you sure you want to change the base?

Proof of concept of dyn dispatch based report handler #97

Conversation

yaahc commented Jun 6, 2020

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jun 21, 2020

yaahc commented Jun 21, 2020

yaahc commented Jun 21, 2020

yaahc commented Jul 18, 2020

yaahc commented Jul 18, 2020 • edited

dtolnay commented Jul 18, 2020

yaahc commented Jul 18, 2020

yaahc commented Jul 18, 2020

yaahc Jul 18, 2020

Choose a reason for hiding this comment

yaahc Jul 18, 2020

Choose a reason for hiding this comment

yaahc Jul 18, 2020

Choose a reason for hiding this comment

yaahc Jul 18, 2020

Choose a reason for hiding this comment

yaahc commented Jul 18, 2020 • edited

yaahc commented Jul 18, 2020 • edited

yaahc commented Jul 18, 2020 • edited

yaahc commented Jul 18, 2020

yaahc commented Jul 22, 2020 • edited

yaahc commented Nov 17, 2020 • edited

Status update

Ecosystem Compatibility

vultix commented Jul 26, 2021

yaahc commented Jul 26, 2021 • edited

rongcuid commented Jul 10, 2023

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 18, 2020 •

edited

yaahc commented Jul 22, 2020 •

edited

yaahc commented Nov 17, 2020 •

edited

yaahc commented Jul 26, 2021 •

edited