-
Notifications
You must be signed in to change notification settings - Fork 97
H-4708: error-stack
: Extract owned types from Report
#7324
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
Conversation
@indietyp I'll reply to your comments relating to this over here:
The times this has been a noticeable issue are usually some variation of this: use error_stack::Report;
mod foo {
use super::*;
#[derive(Debug)]
pub struct MyError {
private: (),
}
impl core::fmt::Display for MyError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
write!(f, "MyError occurred")
}
}
impl core::error::Error for MyError {}
pub fn foo() -> Report<MyError> {
Report::new(MyError { private: () })
}
}
// Note no Report<> wrapping here
fn bar() -> foo::MyError {
let mut report = foo::foo();
// - The foo module cannot be modified
// - MyError cannot be instanstiated
// - MyError does not have a default
// - MyError must be returned unwrapped and this API cannot change
todo!()
} Existing systems that cannot have interfaces changed and limited control over the error type itself.
The current implementation just replaces one I have no special love for this PR's implementation! This was just an initial attempt at getting those new tests written without touching the frame structure. I'd just love something that makes an external API like this work :). |
As I said, afaik this PR specifically avoids changing any structure, or am I missing something?
They don't act any differently, a Edit: actually, |
Thanks for the detailed example with There are, however, alternative ways to achieve similar outcomes – for example, by using the You also emphasized that your main concern is having some API to extract context-like data, and that the exact mechanism is less important to you. I understand and sympathize with that perspective. But that brings us to what I’d call the “plot twist” in this discussion: @indietyp's critique. He’s absolutely right to point out that this change fundamentally alters what a I still think the general direction of enabling richer introspection or mutation makes sense. In fact, I’ve wanted something like this myself in the past (and ended up working around it using This connects to a larger point: our internal model of |
Thanks @TimDiekmann! Unfortunately the provider api (afaik?!) is nightly only, which makes it a no go for me.
I am still somewhat missing exactly what safety guarantees have changed. At a basic level calling
Would it be possible to give me a concrete example that wouldn't apply to |
Report
.error-stack
: Extract owned types from Report
The problem is that this proposal changes a To illustrate, I'll use lowercase letters (
On Stack Structure:graph TD;
a --> b --> A --> c --> d --> B --> e --> f --> A1[A]
A -.-> g["β(g)"]
A -.-> h["β(h)"]
This structure includes:
After calling graph TD;
a --> b --> A0["α(A)"] --> c --> d --> B --> e --> f --> A1[A]
This undermines the user's entire mental model of what a Report is. Attachments and contexts don't just format into a string - they compose behavior and meaning. The moment we allow replacement (even with a dummy), we introduce holes in type-driven reasoning. This is also exacerbated by the fact that it allows for conditional replacement, code might rely on On Tree Structure:This is especially problematic in non-linear (tree-shaped) reports: graph TD;
a --> b --> A --> c
c --> d --> B
c --> e --> f --> A1[A]
c --> g --> A2[A]
If we pop On Ergonomics and IntentI appreciate the motivation - it's a pain I've encountered too. But in practice, I've found that this isn't usually the right solution. Turning an error into a context changes its role: it's no longer "the thing being propagated," it's part of the story - one of many layers of context and attachments building toward a final cause. If you pull it back out as if it were the primary error, you risk discarding important framing (attachments, causality, or even the root issue). That's why I've never reached for an API like this: I want to propagate the whole report, not just a single layer. Anything else strips away what makes error-stack so valuable. On InteropMost Rust libraries that expect a specific error type also provide ergonomic interop layers through conversion or formatting- not mutation. This is why I've never personally felt the need to destructure a Report. Instead, the idiomatic approach is to convert or wrap, preserving the full context chain:
This pattern is consistent across the ecosystem: conversion is the escape hatch, and it maintains all invariants of the original Report. By contrast, mutating a report in-place via context removal breaks assumptions these systems rely on, even if the change is invisible at the formatting layer. |
…ts, into_frame_contents
Thanks for the detailed walkthrough @indietyp! Totally agree with how you outline the change in behaviour after calling pop/take in each of your steps in your walkthrough, I think we just differ in that I see those changes as expected by the user and reasonable, who knowingly called pop or take. I completely get the tradeoffs you're concerned about, I think we just value the tradeoffs slightly differently and it's your project, you'll correctly have the final say :) I think the api hole can still be filled with a watered down version of this PR, that focuses on consuming the entire report, removing the I've updated the PR to be more targeted to not include methods that return a modified report, only a report consuming api. The new api consists of: impl<C> Report<C> {
pub fn into_current_context(self) -> C
where
C: Send + Sync + 'static
}
impl<C: ?Sized> Report<C> {
pub fn into_frame_contents(self) -> impl IntoIterator<Item = Box<dyn Any + Send + Sync>>
pub fn downcast<T: Send + Sync + 'static>(self) -> Result<T, Self>
}
impl<C> Report<[C]> {
pub fn into_current_contexts(self) -> impl Iterator<Item = C>
where
C: Send + Sync + 'static
} What do you think? This does not provide ownership over anything unless the report is consumed. This API would cover all the issues I've encountered, without introducing the concerning soundness holes of modifying a report. I'm not sure on the best name for |
Closing this because the parent ticket is closed as well. Also, as @indietyp pointed out, moving out owned types from a |
🌟 What is the purpose of this PR?
Derived from #7274
Some error types are not
Clone
, or are expensive to clone, but need to be owned to handle them. Not being able to extract/take owned types from aReport
has been one of the trickiest aspects of a migration to ES I've experienced.The current implementation just replaces the types internally with an internal type that will have the same printable format. Methods return an untyped
Report<dyn Error>
because this meansC: Unsized
and methods such ascurrent_context
or further calls topop_current_context
cannot be used,downcast
etc must be used instead as type safety onC
has been lost.downcast_take<T>()
must also removeC
fromReport<C>
and return an untyped (Report<dyn Error>
), becauseT
may equalC
and act likepop_current_context()
.I'm not tied to the implementation, but the api hole is:
There needs to be a way to pull owned types out of an existing report, for both the top level
C
and arbitrary nested contexts + attachments, without consuming the report, or affecting the printable representation.This implementation seemed to solve that goal with the most concise change.