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

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Apr 1, 2020

Rendered

Also, I've mocked up the proposal in this repository - https://github.com/yaahc/nostd-error-poc/

Copy link
Member

@anp anp left a comment

Choose a reason for hiding this comment

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

(didn't make it very far before my day filled up again, will return shortly)

text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
@burdges
Copy link

burdges commented Apr 2, 2020

One provide_context implementation supports multiple TypeIds, yes?

pub struct MyError<'a> {
    some_ref: &'a ..,
    /// All the rest are static
    something_any: ..,
    something_error: ..,
    something_read: ..,
}
impl<'a> Error for MyError<'a> {
    ...
    fn provide_context(&self, ty: TypeId) -> Option<&dyn Any> {
        if ty == TypeId::of::<dyn Any>() {
            &self.something_any as &dyn Any
        } else if ty == TypeId::of::<dyn Error>() {
            &self.something_error as &dyn Any
        } else if ty == TypeId::of::<dyn Read>() {
            &self.something_read as &dyn Any
        } else { None }
    }
}

You cannot make the default provide_context return Some(self as &dyn Any) whenever TypeId::of::<T>() == TypeId::of::<Self>() because doing so requires a Self: 'static bound.

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Apr 4, 2020
@yaahc
Copy link
Member Author

yaahc commented Apr 5, 2020

@burdges this is a general restriction on Any and downcast on Error types, I don't think it's currently possible to downcast types that aren't : 'static

The second design with the a Provider miiight work with this, not sure.

Copy link
Member

@anp anp left a comment

Choose a reason for hiding this comment

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

I focused my feedback more on "technical writing" as requested, but in general I'm quite excited to see this proposal move forward! There are a number of cases at work where it would be nice to specialize error presentation in this way. At a high level I think this proposal would benefit from a "why not more general, i.e. all trait objects?" section. I also recommend putting more examples closer to the top of the "error reports" that you'd like to enhance -- they're quite motivating IMO.

text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
text/0000-dyn-error-generic-member-access.md Outdated Show resolved Hide resolved
@Plecra
Copy link

Plecra commented Apr 13, 2021

I wonder if it is possible to offload some of the complexity of this API to crates.io, similar to how proc_macro only exposes a basic interface (just a token stream) while most of the functionality for building proc macros is located in the syn and quote crates which are not part of the standard library and can evolve independently of it.

The new API is a fairly fundamental extension to the Any trait. The bulk of the API is to do with enabling dynamic downcasts including lifetimes.

There are some parts of the API that could be trimmed at the cost of ergonomics, but I think the new API should be thought of similarly to the Pin API. It's being added to enable an std API we want, and will likely be useful in implementing other crates in the ecosystem (more safe pinning/downcasts). We'd include the simpler provide_... methods for the same reason we have the Deref implementations on Pin and the debug_struct APIs (and on and on): It makes it easier for users to do the right thing

@bstrie
Copy link
Contributor

bstrie commented Apr 13, 2021

WRT "update RFC to be based on dyno design", can someone give a brief summary of the advantages of this new design over the old one?

@yaahc
Copy link
Member Author

yaahc commented Apr 14, 2021

WRT "update RFC to be based on dyno design", can someone give a brief summary of the advantages of this new design over the old one?

I think this reply summarizes it best but the main difference is dyno uses a proxy type that itself only has a phantom data, and so it is 'static and gets a TypeID. That Tag type has a trait with an associated type that is the actual type being erased, where as the old design used the type being erased's TypeID directly, requiring that it is 'static. The new design allows for closures containing mutable references to be requested as shown in this earlier comment, which could be used on a type erased executor with this same generic member access API to request a callback that then modifies state in the executor on the other side of the trait object.

@yaahc
Copy link
Member Author

yaahc commented Apr 14, 2021

This RFC feels very complex and I am a bit uncomfortable with adding so much API surface to std just for the Error trait.

I wonder if it is possible to offload some of the complexity of this API to crates.io, similar to how proc_macro only exposes a basic interface (just a token stream) while most of the functionality for building proc macros is located in the syn and quote crates which are not part of the standard library and can evolve independently of it.

@Amanieu fwiw I'm already working on extracting the type tagged downcasting part of this RFC into it's own independent RFC, which should hopefully make both RFCs much easier to review. I suppose I should update the todolist above to reflect that 😅 .

@CleanCut
Copy link

This looks fantastic! ❤️

What is the likely timeline for this to reach stable? Given that it hasn't hit nightly yet, I'm guessing this will be gated behind a Rust 2024 edition?

@yaahc
Copy link
Member Author

yaahc commented May 17, 2021

This looks fantastic! ❤️

What is the likely timeline for this to reach stable? Given that it hasn't hit nightly yet, I'm guessing this will be gated behind a Rust 2024 edition?

There's no timeline but it won't be gated behind any edition. This RFC doesn't include any breaking changes. The current next step is to break out the type tagged downcasting logic into it's own RFC which is being done by @Plecra.

@yaahc
Copy link
Member Author

yaahc commented May 12, 2022

Reminder for myself: Once this lands I would like to update the example in the docs added in rust-lang/rust#95356 to instead use generic member access to grab the ExitCode from the error.

`Display` trait, which acts as the interface to the main member, the error
message itself. It provides the `source` function for accessing `dyn Error`
members, which typically represent the current error's cause. Via
`#![feature(backtrace)]` it provides the `backtrace` function, for accessing a
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems outdated. In master branch I don't see any backtrace function on the Error trait with or without that feature. Maybe the backtrace feature was already merged but left out the backtrace method on Error. This RFC makes it unnecessary anyway so I'll look into updating this section here.

Copy link
Member

Choose a reason for hiding this comment

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

There used to be an unstable backtrace function on Error, but it was removed in favor for the generic member access proposed in this RFC.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2023
core/any: remove Provider trait, rename Demand to Request

This touches on two WIP features:

* `error_generic_member_access`
  * tracking issue: rust-lang#99301
  * RFC (WIP): rust-lang/rfcs#2895
* `provide_any`
  * tracking issue: rust-lang#96024
  * RFC: rust-lang/rfcs#3192

The changes in this PR are intended to address libs meeting feedback summarized by `@Amanieu` in rust-lang#96024 (comment)

The specific items this PR addresses so far are:

> We feel that the names "demand" and "request" are somewhat synonymous and would like only one of those to be used for better consistency.

I went with `Request` here since it sounds nicer, but I'm mildly concerned that at first glance it could be confused with the use of the word in networking context.

> The Provider trait should be deleted and its functionality should be merged into Error. We are happy to only provide an API that is only usable with Error. If there is demand for other uses then this can be provided through an external crate.

The net impact this PR has is that examples which previously looked like
```
    core::any::request_ref::<String>(&err).unwramp()
```

now look like
```
    (&err as &dyn core::error::Error).request_value::<String>().unwrap()
```

These are methods that based on the type hint when called return an `Option<T>` of that type. I'll admit I don't fully understand how that's done, but it involves `core::any::tags::Type` and `core::any::TaggedOption`, neither of which are exposed in the public API, to construct a `Request` which is then passed to the `Error.provide` method.

Something that I'm curious about is whether or not they are essential to the use of `Request` types (prior to this PR referred to as `Demand`) and if so does the fact that they are kept private imply that `Request`s are only meant to be constructed privately within the standard library? That's what it looks like to me.

These methods ultimately call into code that looks like:
```
/// Request a specific value by tag from the `Error`.
fn request_by_type_tag<'a, I>(err: &'a (impl Error + ?Sized)) -> Option<I::Reified>
where
    I: tags::Type<'a>,
{
    let mut tagged = core::any::TaggedOption::<'a, I>(None);
    err.provide(tagged.as_request());
    tagged.0
}
```

As far as the `Request` API is concerned, one suggestion I would like to make is that the previous example should look more like this:
```
/// Request a specific value by tag from the `Error`.
fn request_by_type_tag<'a, I>(err: &'a (impl Error + ?Sized)) -> Option<I::Reified>
where
    I: tags::Type<'a>,
{
    let tagged_request = core::any::Request<I>::new_tagged();
    err.provide(tagged_request);
    tagged.0
}
```
This makes it possible for anyone to construct a `Request` for use in their own projects without exposing an implementation detail like `TaggedOption` in the API surface.

Otherwise noteworthy is that I had to add `pub(crate)` on both `core::any::TaggedOption` and `core::any::tags` since `Request`s now need to be constructed in the `core::error` module. I considered moving `TaggedOption` into the `core::error` module but again I figured it's an implementation detail of `Request` and belongs closer to that.

At the time I am opening this PR, I have not yet looked into the following bit of feedback:

> We took a look at the generated code and found that LLVM is unable to optimize multiple .provide_* calls into a switch table because each call fetches the type id from Erased::type_id separately each time and the compiler doesn't know that these calls all return the same value. This should be fixed.

This is what I'll focus on next while waiting for feedback on the progress so far. I suspect that learning more about the type IDs will help me understand the need for `TaggedOption` a little better.
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Oct 17, 2023
core/any: remove Provider trait, rename Demand to Request

This touches on two WIP features:

* `error_generic_member_access`
  * tracking issue: rust-lang/rust#99301
  * RFC (WIP): rust-lang/rfcs#2895
* `provide_any`
  * tracking issue: rust-lang/rust#96024
  * RFC: rust-lang/rfcs#3192

The changes in this PR are intended to address libs meeting feedback summarized by `@Amanieu` in rust-lang/rust#96024 (comment)

The specific items this PR addresses so far are:

> We feel that the names "demand" and "request" are somewhat synonymous and would like only one of those to be used for better consistency.

I went with `Request` here since it sounds nicer, but I'm mildly concerned that at first glance it could be confused with the use of the word in networking context.

> The Provider trait should be deleted and its functionality should be merged into Error. We are happy to only provide an API that is only usable with Error. If there is demand for other uses then this can be provided through an external crate.

The net impact this PR has is that examples which previously looked like
```
    core::any::request_ref::<String>(&err).unwramp()
```

now look like
```
    (&err as &dyn core::error::Error).request_value::<String>().unwrap()
```

These are methods that based on the type hint when called return an `Option<T>` of that type. I'll admit I don't fully understand how that's done, but it involves `core::any::tags::Type` and `core::any::TaggedOption`, neither of which are exposed in the public API, to construct a `Request` which is then passed to the `Error.provide` method.

Something that I'm curious about is whether or not they are essential to the use of `Request` types (prior to this PR referred to as `Demand`) and if so does the fact that they are kept private imply that `Request`s are only meant to be constructed privately within the standard library? That's what it looks like to me.

These methods ultimately call into code that looks like:
```
/// Request a specific value by tag from the `Error`.
fn request_by_type_tag<'a, I>(err: &'a (impl Error + ?Sized)) -> Option<I::Reified>
where
    I: tags::Type<'a>,
{
    let mut tagged = core::any::TaggedOption::<'a, I>(None);
    err.provide(tagged.as_request());
    tagged.0
}
```

As far as the `Request` API is concerned, one suggestion I would like to make is that the previous example should look more like this:
```
/// Request a specific value by tag from the `Error`.
fn request_by_type_tag<'a, I>(err: &'a (impl Error + ?Sized)) -> Option<I::Reified>
where
    I: tags::Type<'a>,
{
    let tagged_request = core::any::Request<I>::new_tagged();
    err.provide(tagged_request);
    tagged.0
}
```
This makes it possible for anyone to construct a `Request` for use in their own projects without exposing an implementation detail like `TaggedOption` in the API surface.

Otherwise noteworthy is that I had to add `pub(crate)` on both `core::any::TaggedOption` and `core::any::tags` since `Request`s now need to be constructed in the `core::error` module. I considered moving `TaggedOption` into the `core::error` module but again I figured it's an implementation detail of `Request` and belongs closer to that.

At the time I am opening this PR, I have not yet looked into the following bit of feedback:

> We took a look at the generated code and found that LLVM is unable to optimize multiple .provide_* calls into a switch table because each call fetches the type id from Erased::type_id separately each time and the compiler doesn't know that these calls all return the same value. This should be fixed.

This is what I'll focus on next while waiting for feedback on the progress so far. I suspect that learning more about the type IDs will help me understand the need for `TaggedOption` a little better.
@lygstate
Copy link

maybe this proposal need updated to reflect that fact?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Proposals relating to error handling. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.