Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions library/core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,56 @@ pub trait Error: Debug + Display {
/// assert!(request_ref::<MyLittleTeaPot>(dyn_error).is_none());
/// }
/// ```
///
/// # Delegating Impls
///
/// <div class="warning">
///
/// **Warning**: We recommend implementors avoid delegating implementations of `provide` to
/// source error implementations.
///
/// </div>
///
/// This method should expose context from the current piece of the source chain only, not from
/// sources that are exposed in the chain of sources. Delegating `provide` implementations cause
/// the same context to be provided by multiple errors in the chain of sources which can cause
/// unintended duplication of information in error reports or require heuristics to deduplicate.
///
/// In other words, the following implementation pattern for `provide` is discouraged and should
/// not be used for [`Error`] types exposed in public APIs to third parties.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case for internal errors?

It seems like we could just say "don't do this", it seems clear that in fully private code people can always do whatever they want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels to me like the way to say "don't do this, but it will not cause 'weird behavior'"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't mind erasing the nuance here but I added it to point out where this form of misuse will have a significant impact.

If you have a codebase that is a large workspace of unpublished crates and your codebase is the only one that ever interacts with and reports your errors if you duplicate this information in the chain but never observe it because you only ever call provide on the outermost error in the chain and don't care about identifying exactly which error it came from then its like a tree falling in the forest with nobody around to hear it. If in the future you change your mind and want to associate context with specific errors in the chain of your reports and your implementation becomes a problem you as the author can trivially fix it because you control the definition and all invocations of your errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to defer to you either way on it :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to leave it as is. If someone complains about it in the future I would not object to changing or removing the language but I'm hoping it'll be fine.

///
/// ```rust
/// # #![feature(error_generic_member_access)]
/// # use core::fmt;
/// # use core::error::Request;
/// # #[derive(Debug)]
/// struct MyError {
/// source: Error,
/// }
/// # #[derive(Debug)]
/// # struct Error;
/// # impl fmt::Display for Error {
/// # fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
/// # write!(f, "Example Source Error")
/// # }
/// # }
/// # impl fmt::Display for MyError {
/// # fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
/// # write!(f, "Example Error")
/// # }
/// # }
/// # impl std::error::Error for Error { }
///
/// impl std::error::Error for MyError {
/// fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
/// Some(&self.source)
/// }
///
/// fn provide<'a>(&'a self, request: &mut Request<'a>) {
/// self.source.provide(request) // <--- Discouraged
/// }
/// }
/// ```
#[unstable(feature = "error_generic_member_access", issue = "99301")]
#[allow(unused_variables)]
fn provide<'a>(&'a self, request: &mut Request<'a>) {}
Expand Down
Loading