Added support for `thiserror` #18

TakaakiFuruse · 2020-05-19T02:03:35Z

This is a pull request for Add support for thiserror attributes #15
.

This enables displaydoc to work with thiserror as shown bellow.
It basically does almost nothing if it found out thiserror.

    #[displaydoc(with_thiserror = true)]
    #[derive(Display, Error, Debug)]
    enum MyError {
        /// I'm not a doc for Variant
        #[error("I'm a doc for Variant")]
        Variant,
    }
    assert_display(MyError::Variant, "I'm a doc for Variant");

I also referenced djc/askama 's implementation here for src/input.rs.

The patch went a little diverted from the use case we have discussed here, but this is what I could done so far.

If you have any suggestions and advice, I would be very happy have them.

…estion

TakaakiFuruse · 2020-05-19T02:54:27Z

I have no idea why this test is failing...

yaahc · 2020-05-19T03:40:11Z

I think I made a mistake marking this issue as easy. It looks like the plan I had isn't actually possible and needs to be adjusted. I had intended for you to just modify attr.rs and lib.rs to additionally parse error attributes in addition to doc attributes, but as it turns out thiserror will still see those error attributes and parse them and generate its own conflicting display implementation, which I'm guessing you went into which is why you added the with_thiserror attribute.

I think the right approach is to instead support attributes that are like thiserrors, while not actually accepting error attributes directly. Instead we should support #[display(transparent)] attributes and what not. I went ahead and tried implementing this to see if its possible and I got most of the way done, the only thing left to do is make the transparent variant generate a valid display implementation to forward to its inner method and cleanup possible panics. I've pushed this working copy to https://github.com/yaahc/displaydoc/tree/thiserror-interop, so feel free to pick up from there if you're still interested in finishing this PR. I'm sorry for accidentally giving you an impossible task 😬

TakaakiFuruse · 2020-05-19T03:56:28Z

Thanks for uploading your tree, I will take a look at it and let me finish PR.
Hard job is always welcome.
I'm not good at handling Rust macros, so very happy to see your work.

yaahc · 2020-05-19T03:59:25Z

I'm glad I can help! And admittedly I'm also not very good at using proc-macros, I usually have @dtolnay or @mystor look over my work whenever I need to write one 😅.

TakaakiFuruse · 2020-05-24T11:50:18Z

I have applied your code refactored, and also re-wrote some tests.
(Tests are copied from thiserror crate.)

Mostly it's working but

Here

#[test]
fn test_transparent_for_enum() {
    #[derive(Display, Error, Debug)]
    enum MyError {
        /// Doc for Variant.
        #[display(transparent)]
        Variant(anyhow::Error),
    }

    let var = MyError::Variant(anyhow!("inner").context("outer"));
    assert_display(&var, "outer");
    // assert_eq!(var.source().unwrap().to_string(), "inner") // <====== HERE
}

and here

#[test]
fn test_transparent_for_struct() {
    #[derive(Display, Error, Debug)]
    #[display(transparent)]
    struct Error(ErrorKind);

    #[derive(Display, Error, Debug)]
    enum ErrorKind {
        #[display("E0")]
        E0,
        #[display("E1")]
        E1(#[from] io::Error),
    }

    let error = Error(ErrorKind::E0);
    assert_eq!("E0", error.to_string());
    assert!(error.source().is_none());

    let io = io::Error::new(io::ErrorKind::Other, "oh no!");
    let error = Error(ErrorKind::from(io));
    assert_eq!("E1", error.to_string());
    // error.source().unwrap().downcast_ref::<io::Error>().unwrap(); // <====== HERE
}

are the parts I couldn't implement.
Basically, you can't source() the error now.
I think I can copy what thiserror is doing for implementing Source trait, but decided to leave it in order to ask your advice...

yaahc · 2020-05-29T02:35:20Z

sorry for taking so long to review this, I just got a new job this week so its been a little hectic. I'm reviewing this now.

TakaakiFuruse · 2020-05-29T02:38:47Z

I just got a new job this week

Wow!! Congrats!!

yaahc · 2020-05-29T02:41:49Z

grr, it looks like the anyhow dev dependency breaks tests on 1.31.0, thats annoying

yaahc · 2020-05-29T02:50:20Z

src/attr.rs

+                nested.iter().find_map(|nested_attr| match nested_attr {
+                    NestedMeta::Meta(Meta::Path(path)) => {
+                        if path.is_ident("transparent") {
+                            Some(LitStr::new("{0}", attr.span()))


So this is only partially complete. This covers the case where an error is implemented as a tuple, but thiserror also supports implying the source via a field named source on struct variants, and it supports manually marking a field as the source, so we need to add test cases for the following struct definitions:

struct Error { source: OtherError, } struct Error2 { #[source] cause: OtherError, }

I have added tests to see the case you mentioned is working.
It's working.

yaahc

This looks great! I left a few comments, the main issue will be figuring out the source field and getting its name so we can generate the correct display impl when doing #[display(transparent)]. Ultimately though I'm not sure how useful display transparent will be, given that it still wont impl error transparently, maybe theres a way to get thiserror to do transparent without implementing display...

yaahc · 2020-05-29T02:51:07Z

src/attr.rs

+        let meta = attr.parse_meta()?;
+        let lit = match meta {
+            Meta::NameValue(MetaNameValue { lit: Str(lit), .. }) => Some(lit),
+            Meta::NameValue(_) => unimplemented!("namevalue"),


all of these unimplementeds should probably be removed and replaced with a catch all match arm that reports that the attribute was invalid with compile_error!

I couldn't came up any good idea.

According to the rust doc the compile_error! is supposed to use inside of macro. (I have never used this macro, so my interpretation might be wrong.)

If you just do

let lit = match meta { Meta::NameValue(MetaNameValue { lit: Str(lit), .. }) => Some(lit), Meta::NameValue(_) => complie_error!("ERROR!!),

and simply compile everything, it gives a compile error. I thought writing a macro only for this match statement sounds overkill... (It's a good idea to show an error on compile time.)

I have also tried to return Ok and Err instead of Some and None, but it might change current code a lot (for example, we can't use find_map if we decide to return Ok and Err).

aah, so the way to return a compile_error! is to construct it with quote! and insert it in the generated code, rather than invoking it directly in the proc-macro. You might need to change the fn signature on this function to allow you to return a TokenStream.

yaahc · 2020-05-29T02:51:28Z

src/lib.rs

@@ -7,7 +7,7 @@ mod fmt;
 use proc_macro::TokenStream;
 use syn::{parse_macro_input, DeriveInput};

-#[proc_macro_derive(Display)]
+#[proc_macro_derive(Display, attributes(display))]


we probably need to add source as one of the attributes this macro is allowed to see.

I found out that source was not necessary, since tests are passing without adding the attribute.

yaahc · 2020-05-29T02:52:29Z

tests/with_thiserror_tests.rs

+#[test]
+fn test_transparent_for_struct() {
+    #[derive(Display, Error, Debug)]
+    #[display(transparent)]


error.source() will work here if you do #[error(transparent)] and just let thiserror do the deriving for display.

I commented in this tests and made sure its working.

error.source().unwrap().downcast_ref::<io::Error>().unwrap();

Probably because I have changed #[display(transparent)]

#[error(transparent)]

you mentioned

TakaakiFuruse · 2020-05-31T08:41:42Z

tests/with_thiserror_tests.rs

@@ -44,7 +44,7 @@ fn test_transparent_for_struct() {
    let io = io::Error::new(io::ErrorKind::Other, "oh no!");
    let error = Error(ErrorKind::from(io));
    assert_eq!("E1", error.to_string());
-    // error.source().unwrap().downcast_ref::<io::Error>().unwrap();
+    error.source().unwrap().downcast_ref::<io::Error>().unwrap();


This is the only test failing now.

TakaakiFuruse · 2020-05-31T08:43:40Z

grr, it looks like the anyhow dev dependency breaks tests on 1.31.0, thats annoying

For this, I have no idea how to fix it. So I left it. May be up or downgrade version ??

yaahc · 2020-06-01T15:02:08Z

tests/with_thiserror_tests.rs

+#[test]
+fn test_thiserror_implicit_and_source_works() {
+    #[derive(Display, Error, Debug)]
+    #[error("implicit source")]


these are only passing because you're using error to derive the display which causes thiserror to do it.

Try doing an enum like this as a test case

#[derive(Debug, Display, Error)] enum SourceError { #[error(transparent)] #[display(transparent)] ImplicitSource { source: io::Error, }, #[error(transparent)] #[display(transparent)] ExplicitSource { source: String, #[source] io: io::Error, }, /// There isnt really a {source} DocSourceless { source: String, }, }

TakaakiFuruse · 2020-06-14T22:39:25Z

Sorry for the late response, I was struggled a lot and had some problems.

Here is the WIP commit.

As you suggested

aah, so the way to return a compile_error! is to construct it with quote! and insert it in the generated code, rather than invoking it directly in the proc-macro. You might need to change the fn signature on this function to allow you to return a TokenStream.

I have tried to return quote! in fn display at atttr.rs, but returning quote! did not work, since fn display creates Display struct and modifies it by calling expand_shorthand.

So, I just gave up returning quote! and decided to return Some(Ok(...))`` and Some(Errr(...))``` instead (this seemed more natural).

Then I had another problem.
The other day you gave me the following test case.

#[derive(Debug, Display, Error)]
enum SourceError {
    #[error(transparent)]
    #[display(transparent)]
    ImplicitSource {
        source: io::Error,
    },
    #[error(transparent)]
    #[display(transparent)]
    ExplicitSource {
        source: String,
        #[source]
        io: io::Error,
    },
    /// There isnt really a {source}
    DocSourceless {
         source: String,
    },
}

If you use cargo-expand, you would get following code for the newly added test case.

        impl core::fmt::Display for SourceError {
            fn fmt(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
                #[allow(unused_variables)]
                match self {
                    SourceError::ImplicitSource { source } => {
                        formatter.write_fmt(::core::fmt::Arguments::new_v1(
                            &[""],
                            &match (&_0.__displaydoc_display(),) {
                                (arg0,) => [::core::fmt::ArgumentV1::new(
                                    arg0,
                                    ::core::fmt::Display::fmt,
                                )],
                            },
                        ))
                    }

It's raelly hard to read but it opens SourceError::ImplicitSource { source } and then tries &_0.__displaydoc_display(). This code does not compile, since _0 should be attributes name like source .

The problem is this.

                    NestedMeta::Meta(Meta::Path(path)) => {
                        if path.is_ident("transparent") {
                            Some(Ok(LitStr::new("{0}", attr.span())))  <===== HERE!!!
                        } else {
                            Some(Err(Error::new_spanned(attr, "attr error")))
                        }
                    }

If attribute of derive were transparent , for Struct in Enum, "{0}" does not seems to work.
It should be certain attribute name of the Struct (we don't know the name of attribute within this function.).

What should I do?

yaahc · 2020-06-15T16:04:22Z

Ah, yes, the test case was supposed to show that we can't always just return "{0}" as the display string for source errors, we have to look at the identifier of the member that is the source error, either one named source or the one with a #[source] attribute on it.

yaahc · 2020-06-15T16:04:41Z

Cargo.toml

 quote = "1.0"
 proc-macro2 = "1.0"
+cargo-expand = "0.6.0"


I think you accidentally added cargo-expand as a dependency here

yaahc · 2020-06-15T16:11:15Z

src/attr.rs

+                nested.iter().find_map(|nested_attr| match nested_attr {
+                    NestedMeta::Meta(Meta::Path(path)) => {
+                        if path.is_ident("transparent") {
+                            Some(Ok(LitStr::new("{0}", attr.span())))


So I think what we might want to do is modify Display so instead of only taking a format string, it can take an enum:

enum DisplayFmt { Provided(LitStr), Transparent, }

That way we can differentiate between cases where we've been given a format string and cases where we need to figure it out ourselves. Then when we are constructing the actual display implementation for the type if the format is Transparent we inspect the struct to look at the members and look for one with a #[source] attribute, or a member named source, or if its a unit struct we just insert "{0}".

yaahc · 2021-04-27T22:43:36Z

Hey @TakaakiFuruse are you still interested in working on this PR? If not I think I'd like to take it over.

TakaakiFuruse · 2021-04-27T23:09:03Z

@yaahc
yes you can.

TakaakiFuruse added 2 commits May 18, 2020 22:33

made displaydoc works with thiserror

db7d306

downgraded dependency versions, fixed styles according to clippy sugg…

6767b51

…estion

TakaakiFuruse added 2 commits May 24, 2020 20:39

re-wrote tests, applied @yaaahc's code

167686f

format fix

fcea5e5

yaahc reviewed May 29, 2020

View reviewed changes

yaahc requested changes May 29, 2020

View reviewed changes

added test cases for #[source] attribute

a3aa140

TakaakiFuruse commented May 31, 2020

View reviewed changes

yaahc requested changes Jun 1, 2020

View reviewed changes

yaahc self-assigned this Jun 4, 2020

[WIP]

3c90a1b

yaahc approved these changes Jun 15, 2020

View reviewed changes

yaahc closed this Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for `thiserror` #18

Added support for `thiserror` #18

TakaakiFuruse commented May 19, 2020

TakaakiFuruse commented May 19, 2020

yaahc commented May 19, 2020 •

edited

Loading

TakaakiFuruse commented May 19, 2020

yaahc commented May 19, 2020

TakaakiFuruse commented May 24, 2020

yaahc commented May 29, 2020

TakaakiFuruse commented May 29, 2020

yaahc commented May 29, 2020

yaahc May 29, 2020

TakaakiFuruse May 31, 2020 •

edited

Loading

yaahc left a comment

yaahc May 29, 2020

TakaakiFuruse May 31, 2020

yaahc Jun 1, 2020

yaahc May 29, 2020

TakaakiFuruse May 31, 2020 •

edited

Loading

yaahc May 29, 2020

TakaakiFuruse May 31, 2020

TakaakiFuruse May 31, 2020

TakaakiFuruse commented May 31, 2020

yaahc Jun 1, 2020

TakaakiFuruse commented Jun 14, 2020

yaahc commented Jun 15, 2020

yaahc Jun 15, 2020

yaahc Jun 15, 2020

yaahc commented Apr 27, 2021

TakaakiFuruse commented Apr 27, 2021

Added support for thiserror #18

Added support for thiserror #18

Conversation

TakaakiFuruse commented May 19, 2020

TakaakiFuruse commented May 19, 2020

yaahc commented May 19, 2020 • edited Loading

TakaakiFuruse commented May 19, 2020

yaahc commented May 19, 2020

TakaakiFuruse commented May 24, 2020

yaahc commented May 29, 2020

TakaakiFuruse commented May 29, 2020

yaahc commented May 29, 2020

Choose a reason for hiding this comment

TakaakiFuruse May 31, 2020 • edited Loading

Choose a reason for hiding this comment

yaahc left a 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

TakaakiFuruse May 31, 2020 • 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

TakaakiFuruse commented May 31, 2020

Choose a reason for hiding this comment

TakaakiFuruse commented Jun 14, 2020

yaahc commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaahc commented Apr 27, 2021

TakaakiFuruse commented Apr 27, 2021

Added support for `thiserror` #18

Added support for `thiserror` #18

yaahc commented May 19, 2020 •

edited

Loading

TakaakiFuruse May 31, 2020 •

edited

Loading

TakaakiFuruse May 31, 2020 •

edited

Loading