Skip to content

core/any: remove Provider trait, rename Demand to Request - #113464

Merged
bors merged 1 commit into
rust-lang:masterfrom
waynr:remove-provider-trait
Aug 14, 2023
Merged

core/any: remove Provider trait, rename Demand to Request#113464
bors merged 1 commit into
rust-lang:masterfrom
waynr:remove-provider-trait

Conversation

@waynr

@waynr waynr commented Jul 7, 2023

Copy link
Copy Markdown
Contributor

This touches on two WIP features:

The changes in this PR are intended to address libs meeting feedback summarized by @Amanieu in #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 Requests 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 Requests 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.