-
Notifications
You must be signed in to change notification settings - Fork 1.6k
RFC: Generic member access for dyn Error trait objects #2895
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
39fc23e
63d0539
6d9a212
c1fd1f6
f344bd9
9726cfe
9dd4113
49fc1d0
f2073ac
1fc15e9
460c523
c8b80ed
f16045d
e12ba35
9581f78
6143f6d
1bb8ca7
e82ac82
53431f5
6451b1c
f74f64f
75ef121
ac0f94e
8d55678
7f87544
bcb4823
2ea013d
248e4ca
ef2e47e
d055bbb
ac79814
41b589b
defeafe
48adce6
0d441ac
7b98760
407ce78
84c8bf7
fb02f91
61be66b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,335 @@ | ||
- Feature Name: Add fns for generic member access to dyn Error and the Error trait | ||
- Start Date: 2020-04-01 | ||
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/2895) | ||
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
This RFC proposes a pair of additions to the `Error` trait to support accessing | ||
generic forms of context from `dyn Error` trait objects, one method on the | ||
`Error` trait itself for returning references to members based on a given type | ||
id, and another fn implemented for `dyn Error` that uses a generic return type | ||
to get the type id to pass into the trait object's fn. These functions will act | ||
as a generalized version of `backtrace` and `source`, and would primarily be | ||
used during error reporting when rendering a chain of opaque errors. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Today, there are a number of forms of context that are traditionally gathered | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
when creating errors. These members are gathered so that a final error | ||
reporting type or function can access them and render them independently of the | ||
`Display` implementation for each specific error type. This allows for | ||
consistently formatted and flexible error reports. Today, there are 2 such | ||
forms of context that are traditionally gathered, `backtrace` and `source`. | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
However, the current approach of promoting each form of context to a fn on the | ||
`Error` trait doesn't leave room for forms of context that are not commonly | ||
used, or forms of context that are defined outside of the standard library. | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Example use cases this enables | ||
|
||
* using `backtrace::Backtrace` instead of `std::backtrace::Backtrace` | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* zig-like Error Return Traces by extracting `Location` types from errors | ||
gathered via `#[track_caller]` or some similar mechanism. | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* error source trees instead of chains by accessing the source of an error as a | ||
slice of errors rather than as a single error, such as a set of errors caused | ||
when parsing a file | ||
* `SpanTrace` a backtrace like type from the `tracing-error` library | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be better to cite the mechanism this uses in addition to/instead of the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that might be more detail than is necessary for the RFC, I've merged it with the "alternatives to std::backtrace" bullet and it has a docs.rs link |
||
* Help text such as suggestions or warnings attached to an error report | ||
|
||
By adding a generic form of these functions that works around the restriction | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
on generics in vtables we could support a greater diversity of error handling | ||
needs and make room for experimentation with new forms of context in error | ||
reports. | ||
|
||
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
Error handling in rust consists mainly of two steps, creation/propogation and | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
reporting. The `std::error::Error` trait exists to bridge the gap between these | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a strong and attractive claim but to me needs expansion/defense. i.e. are there really no other reasons to have it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I can think of, the only other purpose it has is downcasting, so if you wanted a type that could contain an open set of errors where the only thing you do with them is downcast them back to concrete types you'd just use |
||
two steps. It does so by acting as a consistent interface that all error types | ||
can implement to allow error reporting types to handle them in a consistent | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
manner when constructing reports for end users. | ||
|
||
The error trait accomplishes this by providing a set of methods for accessing | ||
members of `dyn Error` trait objects. The main member, the error message | ||
itself, is handled by the `Display` trait which is a requirement for | ||
implementing the Error trait. For accessing `dyn Error` members it provides the | ||
`source` function, which conventionally represents the lower level error that | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
caused the current error. And for accessing a `Backtrace` of the state of the | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
stack when an error was created it provides the `backtrace` function. For all | ||
other forms of context relevant to an Error Report the error trait provides the | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
`context`/`provide_context` functions. | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
As an example of how to use these types to construct an error report lets | ||
explore how one could implement an error reporting type that retrieves the | ||
Location where each error in the chain was created, if it exists, and renders | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
it as part of the chain of errors. Our end goal is to get an error report that | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd start the walkthrough with the motivating output, much better headline There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean move this to the top of the guide level explanation? or to motivation / summary? |
||
looks something like this: | ||
|
||
``` | ||
Error: | ||
0: Failed to read instrs from ./path/to/instrs.json | ||
at instrs.rs:42 | ||
1: No such file or directory (os error 2) | ||
``` | ||
|
||
The first step is to define or use a Location type. In this example we will | ||
define our own but we could use also use `std::panic::Location` for example. | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```rust | ||
struct Location { | ||
file: &'static str, | ||
line: usize, | ||
} | ||
``` | ||
|
||
Next we need to gather the location when creating our error types. | ||
|
||
```rust | ||
struct ExampleError { | ||
source: std::io::Error, | ||
location: Location, | ||
path: PathBuf, | ||
} | ||
|
||
impl fmt::Display for ExampleError { | ||
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!(fmt, "Failed to read instrs from {}", path.display()) | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
fn read_instrs(path: &Path) -> Result<String, ExampleError> { | ||
std::fs::read_to_string(path).map_err(|source| { | ||
ExampleError { | ||
source, | ||
path: path.to_owned(), | ||
location: Location { | ||
file: file!(), | ||
line: line!(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since these two lines don't really do anything, I assume they're stand-ins for "extracting Location types from errors gathered via #[track_caller] or similar" mentioned previously? Is there a link to a proposal for how that might be done? Maybe it's already possible and I just haven't been watching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct. It is already possible to do this with |
||
}, | ||
} | ||
}) | ||
} | ||
``` | ||
|
||
Next we need to implement the `Error` trait to expose these members to the | ||
Error Reporter. | ||
|
||
```rust | ||
impl std::error::Error for ExampleError { | ||
fn source(&self) -> Option<&(dyn Error + 'static)> { | ||
Some(&self.source) | ||
} | ||
|
||
fn provide_context(&self, type_id: TypeId) -> Option<&dyn Any> { | ||
if id == TypeId::of::<Location>() { | ||
Some(&self.location) | ||
} else { | ||
None | ||
} | ||
} | ||
} | ||
``` | ||
|
||
And finally, we create an error reporter that prints the error and its source | ||
recursively along with the location data if it was gathered. | ||
|
||
```rust | ||
struct ErrorReporter(Box<dyn Error + Send + Sync + 'static>); | ||
|
||
impl fmt::Debug for ErrorReporter { | ||
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
let mut current_error = Some(self.0.as_ref()); | ||
let mut ind = 0; | ||
|
||
while let Some(error) = current_error { | ||
writeln!(fmt, " {}: {}", ind, error)?; | ||
|
||
if let Some(location) = error.context::<Location>() { | ||
writeln!(fmt, " at {}:{}", location.file, location.line)?; | ||
} | ||
|
||
ind += 1; | ||
current_error = error.source(); | ||
} | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Ok(()) | ||
} | ||
} | ||
``` | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
There are two additions necessary to the standard library to implement this | ||
proposal: | ||
|
||
Add a function for dyn Error trait objects that will be used by error | ||
reporters to access members given a generic type. This function circumvents | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
restrictions on generics in trait functions by being implemented for trait | ||
objects only, rather than as a member of the trait itself. | ||
|
||
```rust | ||
impl dyn Error { | ||
pub fn context<T: Any>(&self) -> Option<&T> { | ||
self.provide_context(TypeId::of::<T>())?.downcast_ref::<T>() | ||
} | ||
} | ||
``` | ||
|
||
With the expected usage: | ||
|
||
```rust | ||
// With explicit parameter passing | ||
let spantrace = error.context::<SpanTrace>(); | ||
|
||
// With a type inference | ||
fn get_spantrace(error: &(dyn Error + 'static)) -> Option<&SpanTrace> { | ||
error.context() | ||
} | ||
``` | ||
|
||
Add a member to the `Error` trait to provide the `&dyn Any` trait objects to | ||
the `context` fn for each member based on the type_id. | ||
|
||
```rust | ||
trait Error { | ||
/// ... | ||
|
||
fn provide_context(&self, id: TypeId) -> Option<&dyn Any> { | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
None | ||
} | ||
} | ||
``` | ||
|
||
With the expected usage: | ||
|
||
```rust | ||
fn provide_context(&self, type_id: TypeId) -> Option<&dyn Any> { | ||
if id == TypeId::of::<Location>() { | ||
Some(&self.location) | ||
} else { | ||
None | ||
} | ||
} | ||
``` | ||
|
||
# Drawbacks | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A potential additional drawback is that requesting types dynamically like this can lead to surprising changes in runtime behavior when changing versions of (transitive) dependencies. Given:
Then a situation is created in which This could even happen in a semver-compatible release of (I've been playing with an alternate design based on an associated type rather than generic member access which causes this situation to be a compile time error instead of a runtime behavior change, but that design requires traits with lifetime GATs to be dyn safe to be actually good, and associated type defaults for it to maybe be possible to retrofit that design onto the standard library's existing error trait.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree this is a footgun I'd prefer to avoid but as far as I can tell this is not a problem being uniquely introduced by generic member access. This is a problem with type erasure in general and the same situation could be encountered by having foo return I avoid this in my crates currently by re-exporting types from crates that are part of my public API (https://docs.rs/color-eyre/latest/color_eyre/#reexports). Applying this to your example you could avoid this footgun if you downcast/request That said, I was never able to figure out how to implement generic member access w/o type erasure and runtime checks. I'm curious to understand more about your potential alternative design, because I would definitely love to see a better solution but I don't see how it is possible given the constraints of the error trait and the goals of generic member access. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
True.
However, it seems to me that this kind of thing is relatively rare currently, both in the standard library and ecosystem crates. The only standard library exception I can think of is So, I think making type erasure a central part of the error handling API of the standard library would probably be the main place this kind of problem would come up. I'm not sure the existing occasional cases of downcasting in public APIs can be used to justify a standard library API built entirely on downcasting.
While true, I imagine another place this might become a problem is in a library whose purpose is to use generic member access to generate pretty error reports. This hypothetical library might hypothetically pull in the Your next point about requesting the type through each path (including crate version) does provide a solution there, but also as you point out, would not be the most fun to maintain.
I can't promise that this particular idea is any better, but for what it's worth, it goes like this: trait Error {
/// Type that contains details about this error.
///
/// For example, this could be a struct containing a String with help text and a Backtrace.
type Details<'a> = ()
where Self: 'a;
/// Get the details about this error.
fn details(&self) -> Self::Details<'_>;
} As written, this requires some language features that aren't implemented yet:
Another part of this design is a To approximate generic member access to make it possible to implement a hypothetical library for pretty-printing errors like described above, I imagine a crate could define some traits like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't that still have a similar problem? To adapt your example from before:
baz still can't get the the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct, but the difference is how it can't get the details. With the generic member access design, this would fail at run time: the generic member access functions would return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can currently have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess a really important question is how many notable occurrences of
I suspect these cases could be covered for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, I think returning a And you have libraries like anyhow that make heavy use of
But that means you have to know what the concrete type is. And There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The return type of
I don't think this really counts towards what I was asking either. For
Worth noting that this is not a regression from the status quo.
You would use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I've just published |
||
[drawbacks]: #drawbacks | ||
|
||
* The API for defining how to return types is cumbersome and possibly not | ||
accessible for new rust users. | ||
* If the type is stored in an Option getting it converted to an `&Any` will | ||
probably challenge new devs, this can be made easier with documented | ||
examples covering common use cases and macros like `thiserror`. | ||
```rust | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this misplaced? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, but I wasn't really sure how to put a code snippet in a bullet list |
||
} else if typeid == TypeId::of::<SpanTrace>() { | ||
self.span_trace.as_ref().map(|s| s as &dyn Any) | ||
} | ||
``` | ||
* When you return the wrong type and the downcast fails you get `None` rather | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
than a compiler error guiding you to the right return type, which can make it | ||
challenging to debug mismatches between the type you return and the type you | ||
use to check against the type_id | ||
* The downcast could be changed to panic when it fails | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* There is an alternative implementation that mostly avoids this issue | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* This approach cannot return slices or trait objects because of restrictions | ||
on `Any` | ||
* The alternative implementation avoids this issue | ||
* The `context` function name is currently widely used throughout the rust | ||
error handling ecosystem in libraries like `anyhow` and `snafu` as an | ||
ergonomic version of `map_err`. If we settle on `context` as the final name | ||
it will possibly break existing libraries. | ||
|
||
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives | ||
|
||
The two alternatives I can think of are: | ||
|
||
## Do Nothing | ||
|
||
We could not do this, and continue to add accessor functions to the `Error` | ||
trait whenever a new type reaches critical levels of popularity in error | ||
reporting. | ||
|
||
If we choose to do nothing we will continue to see hacks around the current | ||
limitations on the error trait such as the `Fail` trait, which added the | ||
missing function access methods that didn't previously exist on the `Error` | ||
trait and type erasure / unnecessary boxing of errors to enable downcasting to | ||
extract members. | ||
[[1]](https://docs.rs/tracing-error/0.1.2/src/tracing_error/error.rs.html#269-274). | ||
|
||
## Use an alternative to Any for passing generic types across the trait boundary | ||
|
||
Nika Layzell has proposed an alternative implementation using a `Provider` type | ||
which avoids using `&dyn Any`. I do not necessarily think that the main | ||
suggestion is necessarily better, but it is much simpler. | ||
|
||
* https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0af9dbf0cd20fa0bea6cff16a419916b | ||
* https://github.com/mystor/object-provider | ||
|
||
With this design an implementation of the `provide_context` fn might instead look like: | ||
|
||
```rust | ||
fn provide_context<'r, 'a>(&'a self, request: Request<'r, 'a>) -> ProvideResult<'r, 'a> { | ||
request | ||
.provide::<PathBuf>(&self.path)? | ||
.provide::<Path>(&self.path)? | ||
.provide::<dyn Debug>(&self.path) | ||
} | ||
``` | ||
|
||
The advantages of this design are that: | ||
|
||
1. It supports accessing trait objects and slices | ||
2. If the user specifies the type they are trying to pass in explicitly they | ||
will get compiler errors when the type doesn't match. | ||
3. Takes advantage of deref sugar to help with conversions from wrapper types | ||
to inner types. | ||
4. Less verbose implementation | ||
|
||
The disadvatages are: | ||
|
||
1. More verbose function signature, very lifetime heavy | ||
2. The Request type uses unsafe code which needs to be verified | ||
3. could encourage implementations where they pass the provider to | ||
`source.provide` first which would prevent the error reporter from knowing | ||
which error in the chain gathered each piece of context and might cause | ||
context to show up multiple times in a report. | ||
|
||
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
I do not know of any other languages whose error handling has similar | ||
facilities for accessing members when reporting errors. For the most part prior | ||
art exists within rust itself in the form of previous additions to the `Error` | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
trait. | ||
|
||
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
- What should the names of these functions be? | ||
- `context`/`context_ref`/`provide_context` | ||
- `member`/`member_ref` | ||
- `provide`/`request` | ||
- Should we go with the implementation that uses `Any` or the one that supports | ||
accessing dynamically sized types like traits and slices? | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Should there be a by value version for accessing temporaries? | ||
- I bring this up specifically for the case where you want to use this | ||
function to get an `Option<&[&dyn Error]>` out of an error, in this case | ||
its unlikely that the error behind the trait object is actually storing | ||
the errors as `dyn Errors`, and theres no easy way to allocate storage to | ||
store the trait objects. | ||
|
||
# Future possibilities | ||
[future-possibilities]: #future-possibilities | ||
|
||
I'd love to see the various error creating libraries like `thiserror` adding | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
support for making members exportable as context for reporters. | ||
|
||
Also, I'm interested in adding support for `Error Return Traces`, similar to | ||
zigs, and I think that this accessor function might act as a critical piece of | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
that implementation. |
Uh oh!
There was an error while loading. Please reload this page.