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

RFC: Generic member access for dyn Error trait objects #2895

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open
Changes from 5 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
39fc23e
Initial commit for first rfc
yaahc Apr 1, 2020
63d0539
First wave of edits
yaahc Apr 2, 2020
6d9a212
more edits
yaahc Apr 2, 2020
c1fd1f6
more edits
yaahc Apr 2, 2020
f344bd9
maybe time to start showing ppl this
yaahc Apr 2, 2020
9726cfe
simplify summary
yaahc Apr 2, 2020
9dd4113
oops, didnt realize this was a suggested edit
yaahc Apr 2, 2020
49fc1d0
post eliza review
yaahc Apr 2, 2020
f2073ac
Adams comments
yaahc Apr 6, 2020
1fc15e9
rewrite to focus on nika's version
yaahc May 4, 2020
460c523
proof reading
yaahc May 4, 2020
c8b80ed
boop
yaahc May 4, 2020
f16045d
boop
yaahc May 4, 2020
e12ba35
boop
yaahc May 4, 2020
9581f78
boop
yaahc May 4, 2020
6143f6d
boop
yaahc May 4, 2020
1bb8ca7
boop
yaahc May 4, 2020
e82ac82
boop
yaahc May 4, 2020
53431f5
boop
yaahc May 4, 2020
6451b1c
boop
yaahc May 4, 2020
f74f64f
boop
yaahc May 5, 2020
75ef121
fix attribution to be less confusing and mention source
yaahc May 5, 2020
ac0f94e
nikanit
yaahc May 5, 2020
8d55678
Update text/0000-dyn-error-generic-member-access.md
yaahc May 5, 2020
7f87544
rename to provide_context
yaahc May 5, 2020
bcb4823
Update based on kennys changes
yaahc May 5, 2020
2ea013d
Document divergence from object-provider crate
yaahc May 5, 2020
248e4ca
Add example to code snippet
yaahc May 5, 2020
ef2e47e
update to include nikas updates
yaahc May 6, 2020
d055bbb
Update text/0000-dyn-error-generic-member-access.md
yaahc May 7, 2020
ac79814
Apply suggestions from code review
yaahc May 7, 2020
41b589b
reply to adams coments
yaahc May 11, 2020
defeafe
make type_id fn in Request private
yaahc May 11, 2020
48adce6
remove type_id fn
yaahc May 11, 2020
0d441ac
update example to use successors
yaahc Jul 28, 2020
7b98760
add back missing write
yaahc Jul 28, 2020
407ce78
update rfc to include new object provider API
yaahc Dec 4, 2020
84c8bf7
add examples for by value
yaahc Dec 4, 2020
fb02f91
Apply suggestions from code review
yaahc Feb 26, 2021
61be66b
update RFC to be based on dyno design
yaahc Apr 12, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
335 changes: 335 additions & 0 deletions text/0000-dyn-error-generic-member-access.md
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.
yaahc marked this conversation as resolved.
Show resolved Hide resolved

# 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
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 dyn Any.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd start the walkthrough with the motivating output, much better headline

Copy link
Member Author

Choose a reason for hiding this comment

The 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!(),
Copy link
Contributor

Choose a reason for hiding this comment

The 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 track_caller close enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. It is already possible to do this with track_caller, so we could update this to include track_caller. I don't recall if there was a specific reason I avoided using track_caller here but I don't mind updating this example.

},
}
})
}
```

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
[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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this misplaced?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.