Tracking Issue for error_generic_member_access

[yaahc]

Feature gate: #![feature(error_generic_member_access)]

This is a tracking issue for the generic member access API on the error trait, which generalizes the pattern of fn source(&self) -> Option<&dyn Error> and fn backtrace(&self) -> Option<&Backtrace> into fn request_ref::<T>(&self) -> Option<&T>

Public API

// std::error

pub trait Error: Debug + Display {
    // existing API unchanged

    fn provide<'a>(&'a self, req: &mut Demand<'a>) {}
}

impl dyn Error + Send + Sync + 'static { // and all combinations of send/sync
    pub fn request_ref<T: ?Sized + 'static>(&self) -> Option<&T> {
        core::any::request_ref(self)
    }

    pub fn request_value<T: 'static>(&self) -> Option<T> {
        core::any::request_value(self)
    }
}

Steps / History

Now tracked by @waynr in #99301 (comment)

Unresolved Questions

• None yet.

[dtolnay]

Hello from dtolnay/thiserror#185. I noticed it can be problematic for macros that Provider and Error both have a method named provide. I know this has been the case for other traits (Display and Debug both have a fmt method, for example) but one thing that makes the situation worse for provide is that we're commonly forced to rely on deref in order to call it, so fully-qualified method call syntax is not suitable to resolve the ambiguity:

• In the case of Display, macros can write core::fmt::Display::fmt(&$thing, formatter) and that will always work, because even if $thing is something like Box<dyn Display>, there is an impl<T> Display for Box<T> where T: Display + ?Sized which makes it work.

• However in the case of Error, writing core::error::Error::provide(&$thing, demand) is less good than $thing.provide(demand) because there is no impl<E> Error for Box<E> where E: Error + ?Sized. If $thing is Box<dyn Error + Send + Sync + 'static>, or anyhow::Error, these things rely on the deref done implicitly by .provide(demand) in order to deref to dyn Error + Send + Sync + 'static, which calling fully qualified core::error::Error::provide will not do.

• The situation gets worse as other traits in the ecosystem follow the lead of Error and possibly add their own provide method. Maybe it's not so common that Provider and Error would be both in scope at the same time, but are we confident that Error and arbitrary other ecosystem trait won't be?

Two proposals:

1. There should be impl Provider for Box<dyn Error + …> in the standard library. This makes core::any::Provider::provide(&$thing, demand) accommodate more cases, but still not as good as having a deref.

2. Is there someone who might be interested in pursuing a "fully qualified method call but with deref/autoref" RFC? The syntax thing.core::any::Provider::provide(demand) has been suggested for this in the past, and would eliminate the difficulty for macros that want a deref or autoref but also need to be robust to multiple traits having the same method name.

[JonasJebing]

I agree, that

a "fully qualified method call but with deref/autoref" RFC

would be great.
An immediate remedy would also be to rename Error::provide.
The error_generic_member_access RFC and the dyno/provide_any RFC were already using Error::provide_context instead of Error::provide in their examples.
I'd gladly create a PR to change the name if there are no objections.

[robamler]

Not sure if this has been proposed before, but maybe an alternative to "fully qualified method call but with deref/autoref" could be that, in case of an ambiguity, trait method resolution favors traits that were introduced in inner scopes.

Currently, the following code doesn't compile:

fn foo(x: u32, formatter: &mut core::fmt::Formatter) {
    use core::fmt::{Display, Debug};

    { // Imagine this block was generated by a macro that only needs `Display` but not `Debug`.
        use core::fmt::Display; // currently leads to a warning: "the item `Display` is imported redundantly"
        x.fmt(formatter); // currently leads to an error: "multiple `fmt` found"
    }
}

The proposal would be to allow the above code, and to resolve x.fmt(formatter) to core::fmt::Display::fmt(&x, formatter) since Display was used in a strictly narrower scope than Debug.

I don't think this would change any behavior in code that currently compiles since it would only disambiguate cases that are currently ambiguous. And it would allow macro authors to simply introduce a new scope and use any traits that their macros will need (this wouldn't leak any identifies to the outer scope since, already today, those uses are scoped to the inner block).

[nrc]

An easier solution would be to rename Error::provide. At one point it was provide_context and we should be able to find some other name for it.

[drmason13]

For what it's worth, provide_context feels like a good name for this method to me.

Sure it's longer but it's more clear that it's about the error providing some sort of context to a user that comes from the concrete error type.

Whereas a plain provide had me thinking "provide what?".

[jyn514]

An immediate remedy would also be to rename Error::provide.
The error_generic_member_access RFC and the dyno/provide_any RFC were already using Error::provide_context instead of Error::provide in their examples.
I'd gladly create a PR to change the name if there are no objections.

@JonasJebing that sounds great! do you have time to follow up on that? :)

[waynr]

FYI for anyone subscribed to this issue hasn't been following #96024 or #113464 - I have volunteered to take on this issue to see what i can do to help push the generic member access work forward.

Next steps for this issue include:

• assign it to myself using rustbot
  • @yaahc is it possible for you to give me edit permissions for the description on this issue so i can keep the steps/history section updated? or should i just ping you whenever i need it updated? or maybe it would be simpler to open a new tracking issue?
• gathering all lingering TODOs and feedback from Tracking Issue for Provider API #96024, making sure it is all captured in this issue
• close Tracking Issue for Provider API #96024
• open PR to RFCs repo to remove the provide_any feature aka https://github.com/rust-lang/rfcs/blob/master/text/3192-dyno.md
• update my other PR to replace all instances of the provide_any feature name
• open PR for thiserror to incorporate removal of the Provider trait, use that PR branch in core/any: remove Provider trait, rename Demand to Request #113464 to update the dependencies for nightly rustfmt and rust-analyzer. I left some thoughts on a merge strategy between rust and thiserror in my PR

[waynr]

@rustbot claim

[yaahc]

• @yaahc is it possible for you to give me edit permissions for the description on this issue so i can keep the steps/history section updated? or should i just ping you whenever i need it updated? or maybe it would be simpler to open a new tracking issue?

Not that I know of. The only way I can think of is via team membership since I think permissions are set up so that team members can usually edit any comments in the repo, for instance, I can edit your reply in this issue for whatever reason...

Feel free to just ping me (zulip works best), I'll start by grabbing the next steps you added and vendoring them into the top level comment.

Edit: alright, I've gone ahead and edited the todolist on the issue. Not sure if the provide_context rename is still relevant, lmk either way.

[waynr]

Feel free to just ping me (zulip works best), I'll start by grabbing the next steps you added and vendoring them into the top level comment.

Thanks! I'm just starting to take a look at all this again today after neglecting it the past week or so, starting with a PR to thiserror: dtolnay/thiserror#243

My next step is to see if I can make use of my thiserror PR branch to fix the rust builds that were failing on thiserror builds.

One thing I'm a little confused about is that thiserror does have a build.rs probe that should already prevent any of the Provider behavior from being included in builds if the probe code fails: https://github.com/waynr/thiserror/blob/543e1237d600949398a7c2e301722ce63cabbd95/build.rs#L22

So maybe my thiserror PR isn't actually necessary anyway? I'll dedicate more time this weekend or early next week to figure it out.

[waynr]

@yaahc updates for the task list in this issue:

DONE items:

• open PR to RFCs repo to remove the provide_any feature aka https://github.com/rust-lang/rfcs/blob/master/text/3192-dyno.md
• update my other PR to replace all instances of the provide_any feature name
• gathering all lingering TODOs and feedback from Tracking Issue for Provider API Tracking Issue for Provider API #96024, making sure it is all captured in this issue (see NEW items below)

UPDATED items:

• open PR for thiserror to incorporate removal of the Provider trait
  • PR is open, waiting for feedback from maintainer: update error generic member access dtolnay/thiserror#243
• close Tracking Issue for Provider API Tracking Issue for Provider API #96024
  • I think we're ready to close this issue and check it off the list

NEW items:

• 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. See Add provide_any module to core rfcs#3192 (comment) for one possible solution (although alternative solutions may be possible).
  • leftover from the provide_any tracking issue task list, i haven't yet taken the time to try my hand at fixing this

To be clear about where I'm coming from, I haven't exhaustively read the entire history on the provide_any tracking issue but I did skim and ignored a lot of the discussion centering around the Provider trait since the libs team has decided to leave that out.

[waynr]

Also worth mentioning for discussion here is that at least one person has expressed a preference for using the name Demand rather than Request because of the potential for Request to be confused with, for example, the concept of an HTTP request: #113464 (comment)

I'll ultimately go either way on this one to be honest (though I really don't like Demand), but I do have a third alternative: Query

Similar to Request, Query might be semantically confused with database queries, but it seems less likely to me and it retains the spirit of requesting vs demanding a value - in my view, if a demand isn't met that is more akin to an error than the None response we are going for.

[waynr]

For everyone's information, this comment is going to be the new status comment for this tracking issue while it's assigned to me. The high-level status here is that most feedback has been addressed, including merging what remains of provide_any into this tracking issue / feature.

Steps / History

• RFC: RFC: Generic member access for dyn Error trait objects rfcs#2895
  • new PR with updates: RFC: Generic member access for dyn Error trait objects rfcs#3461
• Implementation: Add provider API to error trait #98072
• Handoff to @waynr: Tracking Issue for error_generic_member_access #99301 (comment)
• Rename provide to provide_context to avoid conflicts with the Provider trait: Tracking Issue for error_generic_member_access #99301 (comment)
  • not necessary now that Provider has been removed in core/any: remove Provider trait, rename Demand to Request #113464 and Tracking Issue for Provider API #96024 is being closed
• close Tracking Issue for Provider API #96024
  • open PR to RFCs repo to remove the provide_any feature aka https://github.com/rust-lang/rfcs/blob/master/text/3192-dyno.md
  • gather all lingering TODOs and feedback, make sure it is captured here
  • marked provide_any RFC as rejected mark RFC-3192 (aka the 'provide_any' feature) as rejected rfcs#3460
• address majority of feedback from libs team core/any: remove Provider trait, rename Demand to Request #113464
  • update to replace all instances of the provide_any feature name with error_generic_member_access
• open PR for thiserror to incorporate removal of the Provider trait
  • Update to nightly's new Error::provide API dtolnay/thiserror#246
• 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. See Add provide_any module to core rfcs#3192 (comment) for one possible solution (although alternative solutions may be possible).
• Final comment period (FCP)1
• Stabilization PR

[WesleyAC]

Hi! Just wanted to check in on the status of this issue, and if @waynr is still shepherding this.

Is the optimization of multiple provide calls into a jump table a blocker for this? And is it the only blocker? Is implementing the solution discussed here for core::error::Request something that's being considered anywhere?

[waynr]

I haven't been a very good shepherd of this, but it has been on my mind as something I want to come back to. Part of the reason I haven't continued working on it is that the last discussion I participated in made it feel pretty well beyond my type system metaprogramming skills (see https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/error_generic_member_access).

[de-vri-es]

Hey,

Maybe it's a bit late to bring this up, but I also feel it's kinda important.

When I saw these new generic member access API, my first thought was "eep". It introduces a new concept in the standard library which I'm not sure is a good thing: to use types as function contracts.

To clarify what I mean: a type no longer represents just the type a "thing" can have, it also now gets an implied meaning of what it means to provide that type.

This system is designed to allow any T to potentially produce a Y. The problem I see with that is that there is no context information about what the Y value should mean. There is no function contract other than "produce a Y". And that means that Y now has an implicit function contract what it means to produce it.

For example: what am I supposed to expect when I ask MyError to produce a u8? I suppose MyError knows, but how is it going to document this? The most obvious place would be the main type documentation, but I don't think that's a particularly good idea.

Also, if I'm a generic function, there's nothing that stops a user from passing another type that produces a different u8 than what my generic function wants. It's a bit like duck typing in this respect: if a duck can produce a u8 and this T can produce a u8, then T is probably a duck.

[CobaltCause]

I imagine the intended workaround for that is to define a newtype struct that contains an otherwise ambiguous type like u8 where the newtype struct has documentation explaining the specific meaning in the specific context of that newtype struct and also serves as a way to disambiguate between other newtype structs that also just contain u8.

For example, instead of:

let x = request_value::<u8>(&error)?;
// What does the value in x even mean???

It should be preferred to do:

/// A description about e.g. how this is an application-specific error code and what it means goes here.
struct ErrorCode(u8);

// Other newtypes wrapping the same type but having a different meaning.
struct NotErrorCode(u8);
struct AlsoNotErrorCode(u8);`
struct Etc(u8);

let x = request_value::<ErrorCode>(&error)?;
// The value in x means what ErrorCode says it means.

[de-vri-es]

Sure, you can add more types to make more function contracts. But my concern remains that a function contract normally belongs on a function, and here it is on a type.

If this was just a crate on crates.io, then no problem. But for the standard library I would expect it to stick with idiomatic code and not explore what kind of tricks you can do with the typesystem.