Add provider API to error trait #98072

yaahc · 2022-06-13T21:11:10Z

rust-highfive · 2022-06-13T21:11:13Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-13T21:11:15Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

yaahc · 2022-06-13T21:11:28Z

@rustbot label +T-libs-api -T-libs

library/std/src/error.rs

shepmaster · 2022-06-15T18:52:52Z

library/std/src/error.rs

+    ///     fn provide<'a>(&'a self, req: &mut Demand<'a>) {
+    ///         req
+    ///             .provide_ref::<MyBacktrace>(&self.backtrace)
+    ///             .provide_ref::<dyn std::error::Error + 'static>(&self.source);


Do you foresee people doing this? I'd probably discourage it as it's replicating Error::source.

I could also see people doing .provide_ref::<SourceError> as a way of getting the source error and downcasting it in one step instead of two.

Do you foresee people doing this? I'd probably discourage it as it's replicating Error::source.

In the original PoC impl of error in core I thought it might be a good idea to deduplicate all of the provide style APIs to encourage a single point of interaction with the error trait, we could implement source in terms of provide by default, here's the old version of that:

fn source(&self) -> Option<&(dyn Error + 'static)> { core::any::request_by_type_tag::<'_, core::any::tags::Ref<dyn Error + 'static>, _>(self) }

So you'd still default to using source as the getter but you could implement it via either method. I'm not sure if this is a good idea but I think it's worth considering. The biggest concern I can think of is that it will be easy to provide the source as the wrong type, similar to what you mentioned, someone could be trying to provide a trait object but forget to add the type parameter field and just write .provide_ref(&self.source). This could be improved a bit by adding an extra helper on Demand for .provide_source which is hard coded to dyn Error, but that seems a bit hacky given Demand will probably see usage in APIs unrelated to the error trait.

I don't really see a problem with providing it via both, although it's somewhat odd. I think you might want to provide it as the concrete type in more cases? Regardless, I think landing this will give a chance for idioms to settle.

shepmaster · 2022-06-15T18:53:11Z

library/std/src/error.rs

+    /// impl std::error::Error for Error {
+    ///     fn provide<'a>(&'a self, req: &mut Demand<'a>) {
+    ///         req
+    ///             .provide_ref::<MyBacktrace>(&self.backtrace)


This turbofish is redundant, I believe, so I'd recommend removing it.

The examples all use turbofish unconditionally to reduce the chance of accidentally erasing the member as an unexpected type, with the turbofish you'll get a compiler error if you don't provide that exact type.

accidentally erasing the member as an unexpected type

How would that occur? My understanding is that without the turbofish, the compiler will only be able to use the concrete type of the argument for type inference.

How would that occur? My understanding is that without the turbofish, the compiler will only be able to use the concrete type of the argument for type inference.

by providing a type that is different from what you intended / expected, the example I gave in the other comment on source is the best example I have off of the top of my head. If you mean to provide something as a trait object but you don't turbofish the trait object type or remember to manually coerce it you'll end up providing the original unerased type, and then any attempt to request the trait object will fail at runtime.

you'll end up providing the original unerased type

Right, but this is "accidentally forgetting to erase" while you said (emphasis mine)

accidentally erasing the member as an unexpected type

aah, yea, I overused erasing. In the first comment I was referring to .provide_ref as if it erased the T that is being provided, but in reality its the Demand that is the type erased memory location where the T is stored, and provide_ref is acting more like downcast on Any.

When I've played around with this API, I've always used explicit turbofish, and think it's a good practice so that you know what you're providing.

shepmaster · 2022-06-15T18:57:11Z

library/std/src/error.rs

+    ///     let backtrace = MyBacktrace::new();
+    ///     let source = SourceError {};
+    ///     let error = Error { source, backtrace };
+    ///     let dyn_error = &error as &dyn std::error::Error;


Why the choice of dyn Error here? Perhaps the idea is to demonstrate that it works on concrete types and trait objects? If so, I might encourage doing that more explicitly:

let a = error.request_ref::<MyBacktrace>().unwrap(); assert!(core::ptr::eq(&error.backtrace, a)); let dyn_error = &error as &dyn std::error::Error; let b = dyn_error request_ref::<MyBacktrace>().unwrap(); assert!(core::ptr::eq(a, b));

or can you not call request_ref on a concrete type? If so, that'd make me sad.

or can you not call request_ref on a concrete type? If so, that'd make me sad.

Not until we move error into core sadly, for now I had to impl the Provider trait directly on dyn Error in order to make coherence treat it as a foreign trait on a local type. impl<E: Error> Provider for E produces a compiler error complaining about a blanket impl on a foreign trait.

Not until we move error into core sadly

OK, if it can happen then, I'll be happy.

rustbot · 2022-07-11T19:19:30Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

thomcc · 2022-07-12T14:50:01Z

r? @thomcc

thomcc

This looks great to me! No real issues to speak of, and can't wait to start playing with it.

thomcc · 2022-07-13T20:55:08Z

library/std/src/error.rs

+    /// impl std::error::Error for Error {
+    ///     fn provide<'a>(&'a self, req: &mut Demand<'a>) {
+    ///         req
+    ///             .provide_ref::<MyBacktrace>(&self.backtrace)


When I've played around with this API, I've always used explicit turbofish, and think it's a good practice so that you know what you're providing.

thomcc · 2022-07-13T20:56:21Z

library/std/src/error.rs

+    ///     fn provide<'a>(&'a self, req: &mut Demand<'a>) {
+    ///         req
+    ///             .provide_ref::<MyBacktrace>(&self.backtrace)
+    ///             .provide_ref::<dyn std::error::Error + 'static>(&self.source);


I don't really see a problem with providing it via both, although it's somewhat odd. I think you might want to provide it as the concrete type in more cases? Regardless, I think landing this will give a chance for idioms to settle.

thomcc · 2022-07-13T20:57:19Z

@bors r+

bors · 2022-07-13T20:57:21Z

📌 Commit 655d6e8 has been approved by thomcc

It is now in the queue for this repository.

Rollup of 6 pull requests Successful merges: - rust-lang#98072 (Add provider API to error trait) - rust-lang#98580 (Emit warning when named arguments are used positionally in format) - rust-lang#99000 (Move abstract const to middle) - rust-lang#99192 (Fix spans for asm diagnostics) - rust-lang#99222 (Better error message for generic_const_exprs inference failure) - rust-lang#99236 (solaris: unbreak build on native platform) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…plett add tracking issue to generic member access APIs Missed as part of rust-lang#98072

add tracking issue to generic member access APIs Missed as part of rust-lang/rust#98072

Add provider API to error trait

03999c2

rust-highfive assigned kennytm Jun 13, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2022

rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 13, 2022

remove outdated references

fb2d2e5

This comment has been minimized.

Sign in to view

fix broken doc comment

e3839cc

shepmaster reviewed Jun 15, 2022

View reviewed changes

yaahc mentioned this pull request Jun 17, 2022

Move the Error trait into core (without fn backtrace) #90328

Closed

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022

apply suggestions from code review

655d6e8

rust-highfive assigned thomcc and unassigned kennytm Jul 12, 2022

thomcc approved these changes Jul 13, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2022

Dylan-DPC mentioned this pull request Jul 14, 2022

Rollup of 6 pull requests #99242

Merged

bors merged commit 2b17aa6 into rust-lang:master Jul 14, 2022

rustbot added this to the 1.64.0 milestone Jul 14, 2022

yaahc deleted the generic-member-access branch July 14, 2022 19:56

This was referenced Jul 15, 2022

Move Error trait into core rust-lang/project-error-handling#3

Open

Tracking Issue for error_generic_member_access #99301

Open

add tracking issue to generic member access APIs #99302

Merged

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 17, 2022

Rollup merge of rust-lang#99302 - yaahc:gma-tracking-issue, r=joshtri…

28cce68

…plett add tracking issue to generic member access APIs Missed as part of rust-lang#98072

workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022

Rollup merge of #99302 - yaahc:gma-tracking-issue, r=joshtriplett

cb31055

add tracking issue to generic member access APIs Missed as part of rust-lang/rust#98072

JonasJebing mentioned this pull request Oct 23, 2022

Tracking Issue for Provider API #96024

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add provider API to error trait #98072

Add provider API to error trait #98072

yaahc commented Jun 13, 2022

rust-highfive commented Jun 13, 2022

rust-highfive commented Jun 13, 2022

yaahc commented Jun 13, 2022

This comment has been minimized.

shepmaster Jun 15, 2022

yaahc Jun 15, 2022 •

edited

Loading

thomcc Jul 13, 2022

shepmaster Jun 15, 2022

yaahc Jun 15, 2022

shepmaster Jun 15, 2022

yaahc Jun 15, 2022

shepmaster Jun 15, 2022

yaahc Jun 16, 2022 •

edited

Loading

thomcc Jul 13, 2022

shepmaster Jun 15, 2022

yaahc Jun 15, 2022

shepmaster Jun 15, 2022

rustbot commented Jul 11, 2022

thomcc commented Jul 12, 2022

thomcc left a comment

thomcc Jul 13, 2022

thomcc Jul 13, 2022

thomcc commented Jul 13, 2022

bors commented Jul 13, 2022

Add provider API to error trait #98072

Add provider API to error trait #98072

Conversation

yaahc commented Jun 13, 2022

rust-highfive commented Jun 13, 2022

rust-highfive commented Jun 13, 2022

yaahc commented Jun 13, 2022

This comment has been minimized.

Choose a reason for hiding this comment

yaahc Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaahc Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Jul 11, 2022

thomcc commented Jul 12, 2022

thomcc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomcc commented Jul 13, 2022

bors commented Jul 13, 2022

yaahc Jun 15, 2022 •

edited

Loading

yaahc Jun 16, 2022 •

edited

Loading