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

[waynr]

This touches on two WIP features:

• error_generic_member_access
  • tracking issue: Tracking Issue for error_generic_member_access #99301
  • RFC (WIP): RFC: Generic member access for dyn Error trait objects rfcs#2895
• provide_any
  • tracking issue: Tracking Issue for Provider API #96024
  • RFC: Add provide_any module to core rfcs#3192

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.

[rustbot]

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

[waynr]

One problem I'm currently running into when attempting to reproduce the failed test by running x doc is that there are rust tools (rustfmt and rust-analyzer that depend on thiserror which in turn actually depends on the unstable features my PR deals with):

Building compiler artifacts (stage0 -> stage1, x86_64-unknown-linux-gnu)
   Compiling cfg-if v1.0.0
   Compiling stable_deref_trait v1.2.0
   Compiling once_cell v1.18.0
   Compiling smallvec v1.10.0
   Compiling scopeguard v1.1.0
   Compiling log v0.4.19
   Compiling pin-project-lite v0.2.10
   Compiling libc v0.2.147
   Compiling memchr v2.5.0
   Compiling rustc-hash v1.1.0
   Compiling bitflags v1.3.2
   Compiling typenum v1.16.0
   Compiling zerofrom v0.1.2
   Compiling equivalent v1.0.0
   Compiling hashbrown v0.14.0
   Compiling thin-vec v0.2.12
   Compiling instant v0.1.12
   Compiling lock_api v0.4.10
   Compiling linux-raw-sys v0.3.8
   Compiling psm v0.1.21
   Compiling fastrand v1.9.0
   Compiling arrayvec v0.7.4
   Compiling either v1.8.1
   Compiling elsa v1.7.1
   Compiling yoke v0.7.1
   Compiling rustc_graphviz v0.0.0 (/home/wayne/projects/rust-lang/rust/compiler/rustc_graphviz)
   Compiling tracing-core v0.1.30
   Compiling itertools v0.10.5
   Compiling rustc_arena v0.0.0 (/home/wayne/projects/rust-lang/rust/compiler/rustc_arena)
   Compiling ena v0.14.2
   Compiling ppv-lite86 v0.2.17
   Compiling thiserror v1.0.40
   Compiling zerovec v0.9.4
error[E0432]: unresolved imports `std::any::Demand`, `std::any::Provider`
 --> /home/wayne/.cargo/registry/src/index.crates.io-6f17d22bba15001f/thiserror-1.0.40/src/provide.rs:1:16
  |
1 | use std::any::{Demand, Provider};
  |                ^^^^^^  ^^^^^^^^ no `Provider` in `any`
  |                |
  |                no `Demand` in `any`

For more information about this error, try `rustc --explain E0432`.
error: could not compile `thiserror` (lib) due to previous error
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:01:39

Does this mean I'll have to update thiserror to reflect the new API changes and include updating that to a new version in this PR? I was already considering that since the behavior I want is to enable automatic backtrace capture by thiserror.

[cuviper]

This is a big change that I don't have any context on, so I'm reassigning based on the prior feedback.

r? @Amanieu

[thomcc]

Can you write up new API somewhere (or at least the changes)? It's hard to get the full picture from the diff.

[the8472]

One problem I'm currently running into when attempting to reproduce the failed test by running x doc is that there are rust tools (rustfmt and rust-analyzer that depend on thiserror which in turn actually depends on the unstable features my PR deals with):

Are the nightly features of those crates critical or can we disable them?

[waynr]

@thomcc Okay here's what I understand of the full API changes for provide_any and error_generic_member_access prior to this PR:

• new public trait core::any::Provider with one method, fn provide<'a>(&'a self, demand: &mut Demand<'a>)
• new public type core::any::Demand with several methods related to checking for or retrieving values or references:
  • pub fn provide_value<T>(&mut self, value: T) -> &mut Self
  • pub fn provide_value_with<T>(&mut self, fulfil: impl FnOnce() -> T) -> &mut Self
  • pub fn provide_ref<T: ?Sized + 'static>(&mut self, value: &'a T) -> &mut Self
  • pub fn provide_ref_with<T: ?Sized + 'static>( &mut self, fulfil: impl FnOnce() -> &'a T,) -> &mut Self
  • pub fn would_be_satisfied_by_value_of<T>(&self) -> bool
  • pub fn would_be_satisfied_by_ref_of<T>(&self) -> bool
• new functions for working with Provider and Demand:
  • pub fn request_value<'a, T>(provider: &'a (impl Provider + ?Sized)) -> Option<T>
  • pub fn request_ref<'a, T>(provider: &'a (impl Provider + ?Sized)) -> Option<&'a T>
• new public method with default noop implementation for trait core::error::Error: fn provide<'a>(&'a self, demand: &mut Demand<'a>) {}
• Provider implementation for dyn Error

Again, this is the state of the provide_any and error_generic_member_access prior to this PR. The changes this PR makes is:

• remove the new public trait core::any::Provider
• rename core::any::Demand tocore::any::Request
• remove core::any::request_ref and core::any::request_value functions
  • but it adds analogous request_ref and request_value methods to <dyn Error>
    • this is necessary because the current Request API doesn't allow instances to be constructed; request_ref and request_value internally do the work of constructing the Request instance and returning an Option<T> where T is the type annotation provided when calling request_value or request_ref.
• remove the impl Provider for <dyn Error> block, but leave the fn provide<'a>(&'a self, demand: &mut Demand<'a>) {} on the Error trait

Not an API change, but this does also enable error_in_core, error_generic_member_access in library/core/tests/lib.rs which if I understand correctly is necessary because tests in library/core/tests/any.rs now need to import core::error::Error for some of their test cases (since Error is the only type that makes use of the new Request type being tested).

Also worth noting is that I had to add trait_upcasting which I don't fully understand but I think is because of test cases that look like this:

@@ -188,18 +196,18 @@ fn test_provider_boxed() {
 fn test_provider_concrete() {
     let obj = SomeConcreteType { some_string: "hello".to_owned() };

-    assert_eq!(&**request_ref::<String>(&obj).unwrap(), "hello");
-    assert_eq!(&*request_value::<String>(&obj).unwrap(), "bye");
-    assert_eq!(request_value::<u8>(&obj), None);
+    assert_eq!(&**(&obj as &dyn std::error::Error).request_ref::<String>().unwrap(), "hello");
+    assert_eq!(&*(&obj as &dyn std::error::Error).request_value::<String>().unwrap(), "bye");
+    assert_eq!((&obj as &dyn std::error::Error).request_value::<u8>(), None);
 }

-trait OtherTrait: Provider {}
+trait OtherTrait: std::error::Error {}

 impl OtherTrait for SomeConcreteType {}

 impl dyn OtherTrait {
     fn get_ref<T: 'static + ?Sized>(&self) -> Option<&T> {
-        request_ref::<T>(self)
+        <dyn core::error::Error>::request_ref_from::<T>(self)
     }
 }

Where OtherTrait requires that any implementing types also implement the dependency trait, in this case trait OtherTrait: std::error::Error. What I don't understand is why that feature wasn't required to be enabled prior to this PR since something similar was going on with OtherTrait: Provider 🤔

[waynr]

@thomcc Okay here's what I understand of the full API changes for provide_any and error_generic_member_access prior to this PR

To be absolutely clear, when I say "prior to this PR" I mean that these are unstable features that someone else worked on which are already in master branch. I am coming to these features late in the game picking up where other people left off and attempting to help address feedback in the tracking issues for these features.

[waynr]

One problem I'm currently running into when attempting to reproduce the failed test by running x doc is that there are rust tools (rustfmt and rust-analyzer that depend on thiserror which in turn actually depends on the unstable features my PR deals with):

Are the nightly features of those crates critical or can we disable them?

By "those crates" are you referring to the rustfmt and rust-analyzer crates? If so, I'm not sure what I would need to do to disable provide_any or error_generic_member_access for either of them. I tried this:

$ git show stash
commit bd3326d2276a490e6b212e9816f998cc76ecb6dd
Merge: 0c981cdd9cf 45dd2c0a6aa
Author: wayne warren <wayne.warren.s@gmail.com>
Date:   Sat Jul 8 12:11:33 2023 -0600

    WIP on master: 0c981cdd9cf core/any: remove references to Provider, update tests

diff --cc src/tools/rust-analyzer/crates/rust-analyzer/Cargo.toml
index 5b72d57560b,5b72d57560b..6a0643da4dc
--- a/src/tools/rust-analyzer/crates/rust-analyzer/Cargo.toml
+++ b/src/tools/rust-analyzer/crates/rust-analyzer/Cargo.toml
@@@ -51,7 -51,7 +51,7 @@@ always-assert = "0.1.2
  # These dependencies are unused, but we pin them to a version here to restrict them for our transitive dependencies
  # so that we don't pull in duplicates of their dependencies like windows-sys and syn 1 vs 2
  # these would pull in serde 2
--thiserror = "=1.0.39"
++thiserror = { version = "=1.0.39", features = [] }
  serde_repr = "=0.1.11"
  # these would pull in windows-sys 0.45.0
  mio = "=0.8.5"
diff --cc src/tools/rustfmt/Cargo.toml
index 8c312f47a28,8c312f47a28..a16accc33d7
--- a/src/tools/rustfmt/Cargo.toml
+++ b/src/tools/rustfmt/Cargo.toml
@@@ -50,7 -50,7 +50,7 @@@ regex = "1.5
  serde = { version = "1.0.160", features = ["derive"] }
  serde_json = "1.0"
  term = "0.7"
--thiserror = "1.0.40"
++thiserror = { version = "1.0.40", features = [] }
  toml = "0.7.4"
  unicode-segmentation = "1.9"
  unicode-width = "0.1"

But end up seeing the same error anyway when I run x doc. @the8472 where else might I look to disable them?

[waynr]

Are the nightly features of those crates critical or can we disable them?

So my vague understanding is that thiserror bases its usage of the provide_any features on the rust feature flag:

#![cfg_attr(provide_any, feature(provide_any))]

If I understand correctly, then provide_any is enabled on the rustfmt and rust-analyzer crates because when building using x doc they are built with nightly features of the standard library enabled and provide_any is considered a nightly feature (is that right?).

So to be clear I don't think that rustfmt and rust-analyzer are using these features intentionally, they just happen to be enabled when compiling thiserror on nightly.

I can think of three ways to fix the problem for this PR:

• add thiserror-specific feature flag to thiserror (ie open a PR against that repo) that allows us to explicitly disable usage of provide_any features eg

#![cfg_attr(all(provide_any, enable_provide_any), feature(provide_any))]

• somehow remove provide_any from the default set of enabled nightly features
• work on a PR against thiserror in parallel with this PR to keep the changes sync'd

My preference would be the last option. I will test the thiserror branch I work on builds using a toolchain that i build from the rust repo. For the sake of getting CI to pass on this PR I think I would need to pin that version to a branch in the cargo.toml of rustfmt and rust-analyzer (I'm still a little fuzzy on how the compilation stages/steps work)

One problem I can anticipate with this approach is the need to work out a merge strategy between rust maintainers and thiserror maintainers since there is kind of a chicken-and-egg problem here:

• would we merge into thiserror first, leaving its master/main branch broken until this PR merges into rust master?
  • I would think not because this would break anyone using thiserror on nightly.
• or would we merge into rust first? but could we if tools aren't building?

One way to resolve the issue might be to merge this PR with a git-pinned version, for which there seems to be precedent:

./compiler/rustc_codegen_gcc/Cargo.toml
25:gccjit = { git = "https://github.com/antoyo/gccjit.rs" }

Then we would merge the thiserror PR, get a release for it, and update the rustfmt and rust-analyzer crates with the new thiserror version. At this point users who haven't updated their old nightly would just need to update it to keep building their project on nightly.

I'm probably overthinking this. If nightly is temporarily broken for a particular crate users can just hold off on updating until there is a good nightly build of the toolchain.

I hope none of this sounds crazy, I'm just trying to be thorough in my response!

[workingjubilee]

bikesheddy/nonblocking comment: I agree that Demand makes this API look a lot different from "some networking thing", whereas the "nicer" sounding Request is indeed a bit confusing, and thus would indeed slightly prefer the meaner type. 😈

It wouldn't be the first overloaded name in std, though, if the consensus was otherwise, so it's not the end of the world either way.

[waynr]

bikesheddy/nonblocking comment: I agree that Demand makes this API look a lot different from "some networking thing", whereas the "nicer" sounding Request is indeed a bit confusing, and thus would indeed slightly prefer the meaner type.

Since we're getting bike-sheddy...

In addition to the lack of manners, I also don't like Demand because the word implies a sense of intolerance for not getting what is being demanded whereas the value/reference being returned here is wrapped in an Option<T> which implies the expectation that it may not be fulfilled. Therefore, for Demand to be semantically correct shouldn't we return a Result?

So instead of Demand or Request, what do you think about Query? I feel like this is better-aligned with returning an Option<T> than Demand and possibly even better than Request.

[workingjubilee]

clearly, if we want to be that polite, we should make it an Invitation! /silly

[waynr]

@Amanieu gentle ping for review, or suggestions as to who might have bandwidth to review in the near future. I'll also ping the t-release/triage channel in zulip as suggested in the dev guide.

@thomcc did my comment satisfy your request for a summary of changes?

@the8472 regarding my previous report of x doc failing -- I can no longer reproduce that issue locally since I removed the provide_any flag in my latest commit but did see a suggestion from dtolany that it may have been some kind of bug in the bootstrap tooling related to unexpected standard lib used for build scripts:

That error is caused by a bug in rustc's bootstrap where, in some stages, the standard library that gets picked up if $RUSTC is run from a build script is not the standard library that the rest of the crate will get built against.

I don't fully understand this yet. It seems strange to me that it was happening at all given that the thiserror build script performs a trybuild-based probe to detect the availability of the Provider feature: https://github.com/dtolnay/thiserror/blob/543e1237d600949398a7c2e301722ce63cabbd95/build.rs#L22

I guess if the nightly toolchain automatically enables provide_any than the build script is kind of a noop?

Anyway, to answer your original question again more concisely:

Are the nightly features of those crates critical or can we disable them?

They are effectively disabled with the latest changes in this PR (ie removing the provide_any feature flag entirely).

[waynr]

@Amanieu I believe I have addressed all your feedback. I also moved some tests from library/core/tests/any.rs into a newly-created file library/core/tests/error.rs since even though they are making use of core::any::Request in some of the test helper code, the tests themselves actually exercise core::error::request_{ref,value}. But I added this as the last commit in my series of changes so if that reasoning doesn't jive with you it's easy to revert.

I mentioned this in a comment on #104485, but I am curious what makes the addition of ?Sized as a bound to generic type parameter a backwards incompatible/breaking change (this claim was made on #104485). Doesn't ?Sized merely relax/remove the implicit Sized bound on type parameters? With the addition of ?Sized to a type parameter, types that are Sized should still be valid if I understand correctly (and I often don't 🙂). If we could relax that Sized requirement on Box<dyn Error> then we could keep the test I commented out with a NOTE attempting to explain this situation in my recent updates. I think I just realized the issue here -- it's the downstream usages of Box<dyn Error> (ie contexts receiving and using Box<dyn Error> such as an error-reporting library like anyhow that rely on it having a size known at compile time) that potentially rely on the Sized bound which would be broken if it's removed, not the Box<dyn Error> itself.

Anyway, thanks again for your patience and quick responses to my questions!