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

Added support for thiserror #18

Closed
Closed
Show file tree
Hide file tree
Changes from 5 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
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,6 @@ static_assertions = "0.3.4"
libc = { version = "0.2", default-features = false }
rustversion = "1.0.0"
pretty_assertions = "0.6.1"
thiserror = "1.0.18"
anyhow = "1.0.31"

47 changes: 31 additions & 16 deletions src/attr.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use proc_macro2::TokenStream;
use quote::{quote, ToTokens};
use syn::{Attribute, LitStr, Meta, Result};
use syn::{
spanned::Spanned, Attribute, Lit::Str, LitStr, Meta, MetaList, MetaNameValue, NestedMeta,
Result,
};

pub struct Display {
pub fmt: LitStr,
Expand All @@ -18,27 +21,39 @@ impl ToTokens for Display {
}

pub fn display(attrs: &[Attribute]) -> Result<Option<Display>> {
for attr in attrs {
if attr.path.is_ident("doc") {
let meta = attr.parse_meta()?;
let lit = match meta {
Meta::NameValue(syn::MetaNameValue {
lit: syn::Lit::Str(lit),
..
}) => lit,
_ => unimplemented!(),
};

let lit = LitStr::new(lit.value().trim(), lit.span());

let attr = attrs
.iter()
.find(|attr| attr.path.is_ident("display"))
.or_else(|| attrs.iter().find(|attr| attr.path.is_ident("doc")));
if let Some(attr) = attr {
let meta = attr.parse_meta()?;
let lit = match meta {
Meta::NameValue(MetaNameValue { lit: Str(lit), .. }) => Some(lit),
Meta::NameValue(_) => unimplemented!("namevalue"),
Copy link
Owner

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Meta::Path(_) => unimplemented!("path"),
Meta::List(MetaList { nested, .. }) => {
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()))
Copy link
Owner

Choose a reason for hiding this comment

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

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,
}

Copy link
Author

@TakaakiFuruse TakaakiFuruse May 31, 2020

Choose a reason for hiding this comment

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

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

} else {
unimplemented!()
}
}
NestedMeta::Lit(Str(lit)) => Some(lit.clone()),
_ => unimplemented!(),
})
}
};
if let Some(l) = lit {
let mut display = Display {
fmt: lit,
fmt: LitStr::new(l.value().trim(), l.span()),
args: TokenStream::new(),
};

display.expand_shorthand();
return Ok(Some(display));
}
};
}

Ok(None)
Expand Down
1 change: 0 additions & 1 deletion src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ fn specialization() -> TokenStream {
fn impl_struct(input: &DeriveInput, data: &DataStruct) -> Result<TokenStream> {
let ty = &input.ident;
let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();

let display = attr::display(&input.attrs)?.map(|display| {
let pat = match &data.fields {
Fields::Named(fields) => {
Expand Down
1 change: 0 additions & 1 deletion src/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ impl Display {
let mut read = fmt.as_str();
let mut out = String::new();
let mut args = TokenStream::new();

while let Some(brace) = read.find('{') {
out += &read[..=brace];
read = &read[brace + 1..];
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

@TakaakiFuruse TakaakiFuruse May 31, 2020

Choose a reason for hiding this comment

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

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

pub fn derive_error(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
expand::derive(&input)
Expand Down
98 changes: 98 additions & 0 deletions tests/with_thiserror_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
use anyhow::anyhow;
use displaydoc::Display;
use std::error::Error as StdError;
use std::io;
use thiserror::Error;

fn assert_display<T: std::fmt::Display>(input: T, expected: &'static str) {
let out = format!("{}", input);
assert_eq!(expected, out);
}

#[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")
}

#[test]
fn test_transparent_for_struct() {
#[derive(Display, Error, Debug)]
#[error(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();
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 the only test failing now.

}

#[test]
fn test_errordoc_for_enum() {
#[derive(Display, Error, Debug)]
enum MyError {
/// I'm not a doc for Variant
#[display("I'm a doc for Variant")]
Variant,
}
assert_display(MyError::Variant, "I'm a doc for Variant");
}

#[test]
fn test_errordoc_for_struct() {
#[derive(Display, Error, Debug)]
#[display("I'm a doc for MyError")]
struct MyError {
/// I'm not a doc for MyError
variant: u8,
}
assert_display(MyError { variant: 42 }, "I'm a doc for MyError");
}

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

Choose a reason for hiding this comment

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

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,
    },
}

struct ImplicitSource {
source: io::Error,
}

#[derive(Display, Error, Debug)]
#[error("explicit source")]
struct ExplicitSource {
source: String,
#[source]
io: io::Error,
}

let io = io::Error::new(io::ErrorKind::Other, "oh no!");
let error = ImplicitSource { source: io };
error.source().unwrap().downcast_ref::<io::Error>().unwrap();

let io = io::Error::new(io::ErrorKind::Other, "oh no!");
let error = ExplicitSource {
source: String::new(),
io,
};
error.source().unwrap().downcast_ref::<io::Error>().unwrap();
}