Skip to content

Commit

Permalink
Auto merge of #113464 - waynr:remove-provider-trait, r=Amanieu
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bors committed Aug 14, 2023
2 parents b93842a + 8f3d34c commit ee0107a
Show file tree
Hide file tree
Showing 10 changed files with 736 additions and 739 deletions.
1 change: 0 additions & 1 deletion alloc/src/lib.rs
Expand Up @@ -138,7 +138,6 @@
#![feature(maybe_uninit_uninit_array_transpose)]
#![feature(pattern)]
#![feature(pointer_byte_offsets)]
#![feature(provide_any)]
#![feature(ptr_internals)]
#![feature(ptr_metadata)]
#![feature(ptr_sub_ptr)]
Expand Down
2 changes: 1 addition & 1 deletion alloc/src/sync.rs
Expand Up @@ -3575,7 +3575,7 @@ impl<T: core::error::Error + ?Sized> core::error::Error for Arc<T> {
core::error::Error::source(&**self)
}

fn provide<'a>(&'a self, req: &mut core::any::Demand<'a>) {
fn provide<'a>(&'a self, req: &mut core::error::Request<'a>) {
core::error::Error::provide(&**self, req);
}
}

0 comments on commit ee0107a

Please sign in to comment.