RFC: Generic member access for dyn Error trait objects #2895
Conversation
|
One You cannot make the default |
dtolnay
added
the
T-libs-api
|
@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. |
There was a problem hiding this comment.
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.
|
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 |
@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 😅 . |
|
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. |
|
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 |
Add provider API to error trait Implements rust-lang/rfcs#2895
Add provider API to error trait Implements rust-lang/rfcs#2895
| `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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
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.
|
maybe this proposal need updated to reflect that fact? |
| } | ||
| ``` | ||
|
|
||
| # Drawbacks |
There was a problem hiding this comment.
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:
- A crate
foowith versions 1.0.0 and 2.0.0, each of which provides a typeFoo. - A crate
barwhich depends on version 1.0.0 offooand whose errors provideFoo. - A crate
bazwhich dependsbarand on version 2.0.0 offooand tries to requestFoofrombar's errors.
Then a situation is created in which baz will fail to request Foo from bar's error because the requested Foo and provided Foo are actually distinct types because the transitive dependency foo's versions don't match up.
This could even happen in a semver-compatible release of bar and baz because technically the API is not broken: the type can still be requested, but it will simply begin returning None instead of Some(_). This would be even worse if foo, bar, and baz are all library crates depended on by an application crate and the semver-incompatible versions of foo are introduced by a semver-compatible change in bar or baz which would get pulled in by running cargo update on the application crate. The application crate has no recourse in this situation aside from waiting for bar and baz to agree on a semver-compatible version of foo again.
(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.)
Rendered
Also, I've mocked up the proposal in this repository - https://github.com/yaahc/nostd-error-poc/