Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

[WIP] Add derive block for deriving additional traits on error types #163

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

pengowen123
Copy link

@pengowen123 pengowen123 commented Jul 1, 2017

Fixes #134.

Syntax is same as the types block:

error_chain! {
    derive {
        Hash, Eq, PartialEq
    }
}

The only difference is there is no semicolon at the end of the list of traits (it is complicated to add, and I just wanted to get the macro working first). The new feature seems to work fine, now the only thing left to do is to make it useful. I ran into the problems listed in this comment, and I'm not sure how to implement the suggested solution.

The suggested solution is to generate a trait with Self bound by the requested traits, and implement the trait for all types that implement the traits it is bound by. Using the above example, this trait would look like this:

trait Trait: Debug + ::std::error::Error + Send + Hash + Eq + PartialEq {}
impl<T> Trait for T where T: Debug + ::std::error::Error + Send + Hash + Eq + PartialEq {}

The Debug, Error, and Send bounds are always required so they are inserted for all generated traits. However, I don't know how to generate Hash + Eq + PartialEq with a macro because $($trait)+* won't work. I will try to make this work, I just wanted some initial feedback. When this is all complete, I can change the block to be a more general attributes block as requested in this comment

@Yamakaky
Copy link
Contributor

Yamakaky commented Jul 1, 2017

I like this! Could you also update the changelog and add some tests?

@pengowen123
Copy link
Author

I got the trait generation working, and added a generic parameter to State and ChainedError. Individual errors generate a trait that is bound by the traits listed in the derive block, plus a few default ones, that looks like this:

trait Trait: Foo + Bar {}

The Error and ErrorKind types will then derive these traits:

#[derive(Foo, Bar)]

And instead of storing State, each error type will store State<Trait>, which will store a Box<Trait>.

There are two things left to do:

  1. Get the error type to implement Trait. This requires State<Trait> to implement it as well, but I'm not sure if either of these are even possible.

  2. Finish the macro: it doesn't allow traits to be paths like std::fmt::Debug, or have parameters like PartialEq<i32>. In the case of PartialEq, leaving the parameter out gives the default Self, which causes errors because it won't be object safe.

@Yamakaky Perhaps you can help with the first problem?

I'm trying to make an example of what the expansion should look like (maybe it will help find the solution):

// Example usage:
error_chain! {
    derive {
        PartialEq
    }
}

// Should expand to this (ignoring irrelevant generated code):
trait Trait: PartialEq {}
impl<T: PartialEq> Trait for T {}

#[derive(PartialEq)]
struct Error(ErrorKind, State<Trait>);

// How I imagine code would use this:
assert_eq!(error, "error!".into());

Deriving PartialEq won't work because State doesn't implement it, and it can't because it contains a Backtrace, which is out of this crate's control. Have any ideas on what code should be generated here?

links $b
foreign_links $c
errors $d
derive $b
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you name id $e instead of shuffling everything?

@@ -54,6 +58,10 @@ macro_rules! error_chain_processed {
}

) => {
use ::std::fmt::Debug;
use ::std::error::Error as StdError;
create_super_trait!(Trait: Debug, StdError, Send $(, $bound)*);
Copy link
Contributor

Choose a reason for hiding this comment

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

It will not work if you define multiple errors in the same module.
Also, can you remove use and use absolute path instead?

Copy link
Author

Choose a reason for hiding this comment

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

This is a limitation of the create_super_trait macro currently. I will work on getting it fixed.

@Yamakaky
Copy link
Contributor

Yamakaky commented Jul 2, 2017

PartialEq (and others) can be derived directly on State. Though we have to decide if we want the entire chain to be compared or just the top level enum.

@pengowen123
Copy link
Author

I fixed the macro, and reverted the shuffling of things. I originally shuffled them to allow overrideable defaults for the derive block, similar to types, but I decided it wouldn't be useful because the only default is Debug, which cannot be removed.

Everything should be functional now, but deriving PartialEq is still useless because it generates an implementation of PartialEq<Self> for the type. The Self is an issue because State stores the trait object of it, so I get errors about the generated trait not being object safe.

I think comparing the entire chain would be useful to some, as a test may want to assert that the correct error chain is generated, not just the top level error. However, if someone wanted this, they could just iterate through the error chain and test each individual error.

@pengowen123
Copy link
Author

I think the best way forward is to separate the derive name from the bound name, so the macro would be used like this:

error_chain! {
    derive {
        // Format is `Trait, Bound`, separated by semi-colons
        PartialEq, PartialEq<Error>; Serialize, serde::Serialize
    }
}

This allows users to derive PartialEq, but the generated trait bounds will refer to it as PartialEq<Error>, solving the problem of Self.

If users don't derive anything, there is still an error here, stating that it expected example_generated::Trait, but found example_generated::inner::Trait. To solve this we need a way to convert from one trait object to the other.

Also, $error_name::with_chain (not the method from ChainedError) requires E: Trait, because it creates a State<Trait>. In the ChainedError impl for each error type, it calls $error_name::with_chain, so it must also require E: Trait. However, this fails because the bounds are stricter than the ones from the trait.

@samnardoni
Copy link

Any updates on this? This is something I sorely need.

@pengowen123
Copy link
Author

I ran into some problems when implementing this, I can take a look at it soon to see if I can fix it.

@richard-uk1
Copy link

This is really complicated - good luck with it!

@pengowen123
Copy link
Author

Unfortunately I got stuck again while implementing this. @samnardoni Can you provide an example of how you would use this feature? If a general solution isn't possible, it may be best to provide special cases for commonly used derives like Sync or PartialEq.

@samnardoni
Copy link

samnardoni commented Oct 12, 2017

@pengowen123 Sorry about taking so long to reply. I can't remember exactly which derive I wanted, but from thinking back to what I was working on, I'm 90% sure it was Serialize and Deserialize from serde I wanted to derive.

I don't really have any examples of how I'd want to use it in particular; all I know is I really wanted my error type to be serializable so I could send it over a little RPC service I wrote with the tarpc library.

@Yamakaky
Copy link
Contributor

Still conflicting ^^

@pengowen123
Copy link
Author

I renamed State to InternalState and made it generic over types that implement a new trait, State. The error_chain macro will now generate a state type and implement State for it. The reason for this change is so the user can derive or implement traits on the generated state type, as well as the error kind enum. One problem with this is the user will still need to manually implement traits on their error type, but the impl's should be simple because ErrorKind and State can derive the trait:

impl PartialEq for Error {
    fn eq(&self, other: &Error) -> bool {
        let state = self.state.inner == other.state.inner;
        let kind = self.kind == other.kind;

        state && kind
    }
}

The main problem now is the Sized bound on std::error::Error (see this pr for why it can't be removed). I think the only solution here is to use a custom Error trait that doesn't require Sized. @Yamakaky Would this be an acceptable addition to error-chain?

@dcsobral
Copy link

dcsobral commented Aug 8, 2019

Sad that this seems to have died of inattention.

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

Successfully merging this pull request may close these issues.

Allow deriving traits on the Error
5 participants