Skip to content

Add provider API to error trait #98072

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 14, 2022
Merged

Add provider API to error trait #98072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
03999c2
Add provider API to error trait
yaahc Jun 13, 2022
fb2d2e5
remove outdated references
yaahc Jun 13, 2022
e3839cc
fix broken doc comment
yaahc Jun 13, 2022
655d6e8
apply suggestions from code review
yaahc Jul 11, 2022
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
117 changes: 116 additions & 1 deletion library/std/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ use core::array;
use core::convert::Infallible;

use crate::alloc::{AllocError, LayoutError};
use crate::any::TypeId;
use crate::any::{Demand, Provider, TypeId};
use crate::backtrace::Backtrace;
use crate::borrow::Cow;
use crate::cell;
Expand Down Expand Up @@ -295,6 +295,85 @@ pub trait Error: Debug + Display {
fn cause(&self) -> Option<&dyn Error> {
self.source()
}

/// Provides type based access to context intended for error reports.
///
/// Used in conjunction with [`Demand::provide_value`] and [`Demand::provide_ref`] to extract
/// references to member variables from `dyn Error` trait objects.
///
/// # Example
///
/// ```rust
/// #![feature(provide_any)]
/// #![feature(error_generic_member_access)]
/// use core::fmt;
/// use core::any::Demand;
///
/// #[derive(Debug)]
/// struct MyBacktrace {
/// // ...
/// }
///
/// impl MyBacktrace {
/// fn new() -> MyBacktrace {
/// // ...
/// # MyBacktrace {}
/// }
/// }
///
/// #[derive(Debug)]
/// struct SourceError {
/// // ...
/// }
///
/// impl fmt::Display for SourceError {
/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
/// write!(f, "Example Source Error")
/// }
/// }
///
/// impl std::error::Error for SourceError {}
///
/// #[derive(Debug)]
/// struct Error {
/// source: SourceError,
/// backtrace: MyBacktrace,
/// }
///
/// impl fmt::Display for Error {
/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
/// write!(f, "Example Error")
/// }
/// }
///
/// impl std::error::Error for Error {
/// fn provide<'a>(&'a self, req: &mut Demand<'a>) {
/// req
/// .provide_ref::<MyBacktrace>(&self.backtrace)
Copy link
Member

Choose a reason for hiding this comment

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

This turbofish is redundant, I believe, so I'd recommend removing 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.

The examples all use turbofish unconditionally to reduce the chance of accidentally erasing the member as an unexpected type, with the turbofish you'll get a compiler error if you don't provide that exact type.

Copy link
Member

Choose a reason for hiding this comment

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

accidentally erasing the member as an unexpected type

How would that occur? My understanding is that without the turbofish, the compiler will only be able to use the concrete type of the argument for type inference.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would that occur? My understanding is that without the turbofish, the compiler will only be able to use the concrete type of the argument for type inference.

by providing a type that is different from what you intended / expected, the example I gave in the other comment on source is the best example I have off of the top of my head. If you mean to provide something as a trait object but you don't turbofish the trait object type or remember to manually coerce it you'll end up providing the original unerased type, and then any attempt to request the trait object will fail at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

you'll end up providing the original unerased type

Right, but this is "accidentally forgetting to erase" while you said (emphasis mine)

accidentally erasing the member as an unexpected type

Copy link
Member Author

@yaahc yaahc Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

aah, yea, I overused erasing. In the first comment I was referring to .provide_ref as if it erased the T that is being provided, but in reality its the Demand that is the type erased memory location where the T is stored, and provide_ref is acting more like downcast on Any.

Copy link
Member

Choose a reason for hiding this comment

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

When I've played around with this API, I've always used explicit turbofish, and think it's a good practice so that you know what you're providing.

/// .provide_ref::<dyn std::error::Error + 'static>(&self.source);
Copy link
Member

Choose a reason for hiding this comment

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

Do you foresee people doing this? I'd probably discourage it as it's replicating Error::source.

I could also see people doing .provide_ref::<SourceError> as a way of getting the source error and downcasting it in one step instead of two.

Copy link
Member Author

@yaahc yaahc Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do you foresee people doing this? I'd probably discourage it as it's replicating Error::source.

In the original PoC impl of error in core I thought it might be a good idea to deduplicate all of the provide style APIs to encourage a single point of interaction with the error trait, we could implement source in terms of provide by default, here's the old version of that:

    fn source(&self) -> Option<&(dyn Error + 'static)> {
        core::any::request_by_type_tag::<'_, core::any::tags::Ref<dyn Error + 'static>, _>(self)
    }

So you'd still default to using source as the getter but you could implement it via either method. I'm not sure if this is a good idea but I think it's worth considering. The biggest concern I can think of is that it will be easy to provide the source as the wrong type, similar to what you mentioned, someone could be trying to provide a trait object but forget to add the type parameter field and just write .provide_ref(&self.source). This could be improved a bit by adding an extra helper on Demand for .provide_source which is hard coded to dyn Error, but that seems a bit hacky given Demand will probably see usage in APIs unrelated to the error trait.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really see a problem with providing it via both, although it's somewhat odd. I think you might want to provide it as the concrete type in more cases? Regardless, I think landing this will give a chance for idioms to settle.

/// }
/// }
///
/// fn main() {
/// let backtrace = MyBacktrace::new();
/// let source = SourceError {};
/// let error = Error { source, backtrace };
/// let dyn_error = &error as &dyn std::error::Error;
Copy link
Member

Choose a reason for hiding this comment

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

Why the choice of dyn Error here? Perhaps the idea is to demonstrate that it works on concrete types and trait objects? If so, I might encourage doing that more explicitly:

let a = error.request_ref::<MyBacktrace>().unwrap();
assert!(core::ptr::eq(&error.backtrace, a));

let dyn_error = &error as &dyn std::error::Error;
let b = dyn_error request_ref::<MyBacktrace>().unwrap();
assert!(core::ptr::eq(a, b));

or can you not call request_ref on a concrete type? If so, that'd make me sad.

Copy link
Member Author

Choose a reason for hiding this comment

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

or can you not call request_ref on a concrete type? If so, that'd make me sad.

Not until we move error into core sadly, for now I had to impl the Provider trait directly on dyn Error in order to make coherence treat it as a foreign trait on a local type. impl<E: Error> Provider for E produces a compiler error complaining about a blanket impl on a foreign trait.

Copy link
Member

Choose a reason for hiding this comment

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

Not until we move error into core sadly

OK, if it can happen then, I'll be happy.

/// let backtrace_ref = dyn_error.request_ref::<MyBacktrace>().unwrap();
///
/// assert!(core::ptr::eq(&error.backtrace, backtrace_ref));
/// }
/// ```
#[unstable(feature = "error_generic_member_access", issue = "none")]
#[allow(unused_variables)]
fn provide<'a>(&'a self, req: &mut Demand<'a>) {}
}

#[unstable(feature = "error_generic_member_access", issue = "none")]
impl Provider for dyn Error + 'static {
fn provide<'a>(&'a self, req: &mut Demand<'a>) {
self.provide(req)
}
}

mod private {
Expand Down Expand Up @@ -831,6 +910,18 @@ impl dyn Error + 'static {
None
}
}

/// Request a reference of type `T` as context about this error.
#[unstable(feature = "error_generic_member_access", issue = "none")]
pub fn request_ref<T: ?Sized + 'static>(&self) -> Option<&T> {
core::any::request_ref(self)
}

/// Request a value of type `T` as context about this error.
#[unstable(feature = "error_generic_member_access", issue = "none")]
pub fn request_value<T: 'static>(&self) -> Option<T> {
core::any::request_value(self)
}
}

impl dyn Error + 'static + Send {
Expand All @@ -854,6 +945,18 @@ impl dyn Error + 'static + Send {
pub fn downcast_mut<T: Error + 'static>(&mut self) -> Option<&mut T> {
<dyn Error + 'static>::downcast_mut::<T>(self)
}

/// Request a reference of type `T` as context about this error.
#[unstable(feature = "error_generic_member_access", issue = "none")]
pub fn request_ref<T: ?Sized + 'static>(&self) -> Option<&T> {
<dyn Error + 'static>::request_ref(self)
}

/// Request a value of type `T` as context about this error.
#[unstable(feature = "error_generic_member_access", issue = "none")]
pub fn request_value<T: 'static>(&self) -> Option<T> {
<dyn Error + 'static>::request_value(self)
}
}

impl dyn Error + 'static + Send + Sync {
Expand All @@ -877,6 +980,18 @@ impl dyn Error + 'static + Send + Sync {
pub fn downcast_mut<T: Error + 'static>(&mut self) -> Option<&mut T> {
<dyn Error + 'static>::downcast_mut::<T>(self)
}

/// Request a reference of type `T` as context about this error.
#[unstable(feature = "error_generic_member_access", issue = "none")]
pub fn request_ref<T: ?Sized + 'static>(&self) -> Option<&T> {
<dyn Error + 'static>::request_ref(self)
}

/// Request a value of type `T` as context about this error.
#[unstable(feature = "error_generic_member_access", issue = "none")]
pub fn request_value<T: 'static>(&self) -> Option<T> {
<dyn Error + 'static>::request_value(self)
}
}

impl dyn Error {
Expand Down
1 change: 1 addition & 0 deletions library/std/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@
#![feature(panic_internals)]
#![feature(portable_simd)]
#![feature(prelude_2024)]
#![feature(provide_any)]
#![feature(ptr_as_uninit)]
#![feature(raw_os_nonzero)]
#![feature(slice_internals)]
Expand Down