Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for source forwarding in Error derive #293

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

MegaBluejay
Copy link
Contributor

@MegaBluejay MegaBluejay commented Aug 14, 2023

Related to #110, #200

Synopsis

Adds support for #[error(forward)] attribute, which forwards the .source() implementation to a field.

Solution

Use existing forward fields in State for parsing the attributes, and keep the source-inferring logic the same as without forwarding.

Backtrace forwarding

The current behaviour is that if the field annotated/inferred to be the source is annotated with #[error(backtrace)], a forwarded provide() is generated.

This was in the example usage, but not documented in the list above, so I added it.

Checklist

  • Documentation is updated (if required)
  • Add errors on usage of #[error(forward)] when no source field was specified/inferred
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

@MegaBluejay

This comment was marked as resolved.

@MegaBluejay MegaBluejay marked this pull request as ready for review August 15, 2023 12:28
@MegaBluejay MegaBluejay marked this pull request as draft August 16, 2023 12:07
@MegaBluejay
Copy link
Contributor Author

@tyranron I implemented this with the existing parsing since it was an easy change.

Would it be preferable to refactor the derive first, similarly to #286?

@tyranron
Copy link
Collaborator

@MegaBluejay no, let's postpone refactoring here as a minor priority task.

@@ -32,6 +32,11 @@ often [`From` as well](crate::From).
called `Backtrace`. Then it would return that field as the `backtrace`.
3. One of the fields is annotated with `#[error(backtrace)]`. Then it would
return that field as the `backtrace`.
4. The source field is annotated with `#[error(backtrace)]`. Then it would
Copy link
Owner

Choose a reason for hiding this comment

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

copy paste error?

Suggested change
4. The source field is annotated with `#[error(backtrace)]`. Then it would
4. The source field is annotated with `#[error(forward)]`. Then it would

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No that's correct, provide is forwarded only when the source is annotated with #[error(backtrace)].

That behaviour is unchanged in this PR.

The reasoning for keeping a separate attribute is to make source forwarding work on stable, since there's no good way of generating feature-gated code.

I also think this is somewhat unintuitive, but I haven't come up with a better idea.

Perhaps it could make sense to disallow #[error(backtrace)] on the source field if there's no #[error(forward)]?

@JelteF
Copy link
Owner

JelteF commented Aug 21, 2023

I'm having a hard time understanding the usecase for such forwarding. Could you share a concrete example for when this is useful?

@MegaBluejay
Copy link
Contributor Author

Could you share a concrete example for when this is useful?

The main use case I can see is transparent error variants

Something like

#[derive(Debug, Display, Error)
enum Error {
    ...
    #[error(forward)]
    #[display("{_0}")]
    #[debug("{_0:?}")]
    Other(anyhow::Error),
}

Where there's no new information added by the wrapper, so adding it to the source stack is unnecessary.

@tyranron
Copy link
Collaborator

tyranron commented Aug 23, 2023

@MegaBluejay yes, I start thinking that maybe transparent is better here than forward. Let's postpone it for a little while... I'll write up some design thoughts during the following week. And, of course, will describe the motivation more clearly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants