From d19d5256a03014b2c529ed591c6480be0ef22ef3 Mon Sep 17 00:00:00 2001 From: tyranron Date: Sat, 23 Mar 2019 23:22:32 +0200 Subject: [PATCH 1/5] Refactor detecting attr msg finding function and fix tests to compile --- failure_derive/src/lib.rs | 103 ++++++++++++--------- failure_derive/tests/backtrace.rs | 1 - failure_derive/tests/custom_type_bounds.rs | 1 - failure_derive/tests/no_derive_display.rs | 1 - failure_derive/tests/wraps.rs | 15 ++- 5 files changed, 69 insertions(+), 52 deletions(-) diff --git a/failure_derive/src/lib.rs b/failure_derive/src/lib.rs index 7df10d1..0c5d75c 100644 --- a/failure_derive/src/lib.rs +++ b/failure_derive/src/lib.rs @@ -6,9 +6,9 @@ extern crate synstructure; #[macro_use] extern crate quote; -use proc_macro2::{TokenStream, Span}; -use syn::LitStr; +use proc_macro2::{Span, TokenStream}; use syn::spanned::Spanned; +use syn::LitStr; #[derive(Debug)] struct Error(TokenStream); @@ -43,6 +43,8 @@ fn fail_derive_impl(s: synstructure::Structure) -> Result { let ty_name = LitStr::new(&s.ast().ident.to_string(), Span::call_site()); + //let name_body = + // s.each_variant(|v| quote!(return Some(concat!(module_path!(), "::", #ty_name)))); let cause_body = s.each_variant(|v| { if let Some(cause) = v.bindings().iter().find(is_cause) { quote!(return Some(::failure::AsFail::as_fail(#cause))) @@ -50,7 +52,6 @@ fn fail_derive_impl(s: synstructure::Structure) -> Result { quote!(return None) } }); - let bt_body = s.each_variant(|v| { if let Some(bi) = v.bindings().iter().find(is_backtrace) { quote!(return Some(#bi)) @@ -99,36 +100,23 @@ fn fail_derive_impl(s: synstructure::Structure) -> Result { } fn display_body(s: &synstructure::Structure) -> Result, Error> { - let mut msgs = s.variants().iter().map(|v| find_error_msg(&v.ast().attrs)); + let mut msgs = s.variants().iter().map(|v| find_msg_of("display", &v.ast().attrs)); if msgs.all(|msg| msg.map(|m| m.is_none()).unwrap_or(true)) { return Ok(None); } let mut tokens = TokenStream::new(); for v in s.variants() { - let msg = - find_error_msg(&v.ast().attrs)? - .ok_or_else(|| Error::new( - v.ast().ident.span(), - "All variants must have display attribute." - ))?; - if msg.nested.is_empty() { - return Err(Error::new( - msg.span(), - "Expected at least one argument to fail attribute" - )); - } + let msg = find_msg_of("display", &v.ast().attrs)?.ok_or_else(|| { + Error::new( + v.ast().ident.span(), + "All variants must have display attribute.", + ) + })?; let format_string = match msg.nested[0] { - syn::NestedMeta::Meta(syn::Meta::NameValue(ref nv)) if nv.ident == "display" => { - nv.lit.clone() - } - _ => { - return Err(Error::new( - msg.span(), - "Fail attribute must begin `display = \"\"` to control the Display message." - )); - } + syn::NestedMeta::Meta(syn::Meta::NameValue(ref nv)) => nv.lit.clone(), + _ => unreachable!(), }; let args = msg.nested.iter().skip(1).map(|arg| match *arg { syn::NestedMeta::Literal(syn::Lit::Int(ref i)) => { @@ -146,13 +134,13 @@ fn display_body(s: &synstructure::Structure) -> Result Result { return Err(Error::new( arg.span(), - "Invalid argument to fail attribute!" + "Invalid argument to fail attribute!", )); - }, + } }); let args = args.collect::, _>>()?; @@ -189,30 +177,57 @@ fn display_body(s: &synstructure::Structure) -> Result Result, Error> { - let mut error_msg = None; +fn find_msg_of(attr_name: &str, attrs: &[syn::Attribute]) -> Result, Error> { + let mut found_msg = None; for attr in attrs { if let Some(meta) = attr.interpret_meta() { if meta.name() == "fail" { - if error_msg.is_some() { - return Err(Error::new( - meta.span(), - "Cannot have two display attributes" - )); - } else { - if let syn::Meta::List(list) = meta { - error_msg = Some(list); - } else { + if let syn::Meta::List(msg) = meta { + if msg.nested.is_empty() { return Err(Error::new( - meta.span(), - "fail attribute must take a list in parentheses" + msg.span(), + "Expected at least one argument to fail attribute", )); } + let mut found = false; + match msg.nested[0] { + syn::NestedMeta::Meta(syn::Meta::NameValue(ref nv)) + if nv.ident == "display" || nv.ident == "name" => + { + if nv.ident == attr_name { + if found_msg.is_some() { + return Err(Error::new( + msg.span(), + &format!("Cannot have two {} attributes", attr_name), + )); + } + // Can't assign directly, because of: + // error[E0505]: cannot move out of `msg` because it is borrowed + found = true; + } + } + _ => { + return Err(Error::new( + msg.span(), + "Fail attribute must begin with `display = \"\"` \ + to control the Display message,\ + or with `name = \"\"` to specify the name of the error.", + )); + } + } + if found { + found_msg = Some(msg); + } + } else { + return Err(Error::new( + meta.span(), + "fail attribute must take a list in parentheses", + )); } } } } - Ok(error_msg) + Ok(found_msg) } fn is_backtrace(bi: &&synstructure::BindingInfo) -> bool { diff --git a/failure_derive/tests/backtrace.rs b/failure_derive/tests/backtrace.rs index a307184..705b29f 100644 --- a/failure_derive/tests/backtrace.rs +++ b/failure_derive/tests/backtrace.rs @@ -1,5 +1,4 @@ extern crate failure; -#[macro_use] extern crate failure_derive; use failure::{Backtrace, Fail}; diff --git a/failure_derive/tests/custom_type_bounds.rs b/failure_derive/tests/custom_type_bounds.rs index fd1c8b9..fc05bb4 100644 --- a/failure_derive/tests/custom_type_bounds.rs +++ b/failure_derive/tests/custom_type_bounds.rs @@ -1,4 +1,3 @@ -#[macro_use] extern crate failure; use std::fmt::Debug; diff --git a/failure_derive/tests/no_derive_display.rs b/failure_derive/tests/no_derive_display.rs index 20eeb30..a84c4c6 100644 --- a/failure_derive/tests/no_derive_display.rs +++ b/failure_derive/tests/no_derive_display.rs @@ -1,5 +1,4 @@ extern crate failure; -#[macro_use] extern crate failure_derive; use failure::Fail; diff --git a/failure_derive/tests/wraps.rs b/failure_derive/tests/wraps.rs index 2049273..e9e8ffe 100644 --- a/failure_derive/tests/wraps.rs +++ b/failure_derive/tests/wraps.rs @@ -1,5 +1,4 @@ extern crate failure; -#[macro_use] extern crate failure_derive; use std::fmt; @@ -58,8 +57,11 @@ fn wrap_backtrace_error() { .and_then(|err| err.downcast_ref::()) .is_some()); assert!(err.backtrace().is_some()); - assert!(err.backtrace().is_empty()); - assert_eq!(err.backtrace().is_empty(), err.backtrace().to_string().trim().is_empty()); + assert!(err.backtrace().unwrap().is_empty()); + assert_eq!( + err.backtrace().unwrap().is_empty(), + err.backtrace().unwrap().to_string().trim().is_empty() + ); } #[derive(Fail, Debug)] @@ -93,6 +95,9 @@ fn wrap_enum_error() { .and_then(|err| err.downcast_ref::()) .is_some()); assert!(err.backtrace().is_some()); - assert!(err.backtrace().is_empty()); - assert_eq!(err.backtrace().is_empty(), err.backtrace().to_string().trim().is_empty()); + assert!(err.backtrace().unwrap().is_empty()); + assert_eq!( + err.backtrace().unwrap().is_empty(), + err.backtrace().unwrap().to_string().trim().is_empty() + ); } From f2e149ff1949b6a65f5733b1fb396b31ce4459a4 Mon Sep 17 00:00:00 2001 From: tyranron Date: Sun, 24 Mar 2019 00:21:39 +0200 Subject: [PATCH 2/5] Impl name specifying via attr --- failure_derive/src/lib.rs | 41 ++++++++++++++++++++++++++++++----- failure_derive/tests/tests.rs | 30 ++++++++++++++++++++----- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/failure_derive/src/lib.rs b/failure_derive/src/lib.rs index 0c5d75c..078059f 100644 --- a/failure_derive/src/lib.rs +++ b/failure_derive/src/lib.rs @@ -41,10 +41,7 @@ fn fail_derive_impl(s: synstructure::Structure) -> Result { quote! { & } }; - let ty_name = LitStr::new(&s.ast().ident.to_string(), Span::call_site()); - - //let name_body = - // s.each_variant(|v| quote!(return Some(concat!(module_path!(), "::", #ty_name)))); + let name_body = name_body(&s)?; let cause_body = s.each_variant(|v| { if let Some(cause) = v.bindings().iter().find(is_cause) { quote!(return Some(::failure::AsFail::as_fail(#cause))) @@ -64,7 +61,7 @@ fn fail_derive_impl(s: synstructure::Structure) -> Result { quote!(::failure::Fail), quote! { fn name(&self) -> Option<&str> { - Some(concat!(module_path!(), "::", #ty_name)) + #name_body } #[allow(unreachable_code)] @@ -99,8 +96,40 @@ fn fail_derive_impl(s: synstructure::Structure) -> Result { }) } +fn name_body(s: &synstructure::Structure) -> Result { + let mut msgs = s + .variants() + .iter() + .map(|v| find_msg_of("name", &v.ast().attrs)); + if msgs.all(|msg| msg.map(|m| m.is_none()).unwrap_or(true)) { + let ty_name = LitStr::new(&s.ast().ident.to_string(), Span::call_site()); + return Ok(quote!(return Some(concat!(module_path!(), "::", #ty_name)))); + } + + let mut match_body = TokenStream::new(); + for v in s.variants() { + let msg = find_msg_of("name", &v.ast().attrs)?.ok_or_else(|| { + Error::new( + v.ast().ident.span(), + "All variants must have name attribute.", + ) + })?; + + let name_value = match msg.nested[0] { + syn::NestedMeta::Meta(syn::Meta::NameValue(ref nv)) => nv.lit.clone(), + _ => unreachable!(), + }; + let pat = v.pat(); + match_body.extend(quote!(#pat => { return Some(#name_value) })); + } + Ok(quote!(match *self { #match_body })) +} + fn display_body(s: &synstructure::Structure) -> Result, Error> { - let mut msgs = s.variants().iter().map(|v| find_msg_of("display", &v.ast().attrs)); + let mut msgs = s + .variants() + .iter() + .map(|v| find_msg_of("display", &v.ast().attrs)); if msgs.all(|msg| msg.map(|m| m.is_none()).unwrap_or(true)) { return Ok(None); } diff --git a/failure_derive/tests/tests.rs b/failure_derive/tests/tests.rs index 4e73255..5f5ae81 100644 --- a/failure_derive/tests/tests.rs +++ b/failure_derive/tests/tests.rs @@ -1,27 +1,36 @@ extern crate failure; -#[macro_use] extern crate failure_derive; +use failure::Fail; + #[derive(Fail, Debug)] #[fail(display = "An error has occurred.")] +#[fail(name = "UNIT_ERROR")] struct UnitError; #[test] fn unit_struct() { let s = format!("{}", UnitError); assert_eq!(&s[..], "An error has occurred."); + + assert_eq!(UnitError.name().unwrap(), "UNIT_ERROR"); } #[derive(Fail, Debug)] #[fail(display = "Error code: {}", code)] +#[fail(name = "RECORD_ERROR")] struct RecordError { code: u32, } #[test] fn record_struct() { - let s = format!("{}", RecordError { code: 0 }); + let err = RecordError { code: 0 }; + + let s = format!("{}", err); assert_eq!(&s[..], "Error code: 0"); + + assert_eq!(err.name().unwrap(), "RECORD_ERROR"); } #[derive(Fail, Debug)] @@ -37,19 +46,30 @@ fn tuple_struct() { #[derive(Fail, Debug)] enum EnumError { #[fail(display = "Error code: {}", code)] + #[fail(name = "STRUCT")] StructVariant { code: i32 }, #[fail(display = "Error: {}", _0)] + #[fail(name = "TUPLE")] TupleVariant(&'static str), #[fail(display = "An error has occurred.")] + #[fail(name = "UNIT")] UnitVariant, } #[test] fn enum_error() { - let s = format!("{}", EnumError::StructVariant { code: 2 }); + let structure = EnumError::StructVariant { code: 2 }; + let s = format!("{}", structure); assert_eq!(&s[..], "Error code: 2"); - let s = format!("{}", EnumError::TupleVariant("foobar")); + assert_eq!(structure.name().unwrap(), "STRUCT"); + + let tuple = EnumError::TupleVariant("foobar"); + let s = format!("{}", tuple); assert_eq!(&s[..], "Error: foobar"); - let s = format!("{}", EnumError::UnitVariant); + assert_eq!(tuple.name().unwrap(), "TUPLE"); + + let unit = EnumError::UnitVariant; + let s = format!("{}", unit); assert_eq!(&s[..], "An error has occurred."); + assert_eq!(unit.name().unwrap(), "UNIT"); } From 75db9988dcfb9ec017197f20bfc49a8b8c67f74f Mon Sep 17 00:00:00 2001 From: tyranron Date: Sun, 24 Mar 2019 00:41:07 +0200 Subject: [PATCH 3/5] Describe in docs and upd design --- book/src/derive-fail.md | 36 +++++++++++++++++++++++++++++++++++ examples/simple.rs | 3 ++- failure_derive/tests/tests.rs | 3 +-- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/book/src/derive-fail.md b/book/src/derive-fail.md index 6fffd99..00b2166 100644 --- a/book/src/derive-fail.md +++ b/book/src/derive-fail.md @@ -175,3 +175,39 @@ enum MyEnumError { Variant2(#[fail(cause)] io::Error), } ``` + +## Overriding `name` + +By default, `name()` method of derived implementation of `Fail` returns absolute type name: +```rust +#[derive(Fail, Debug)] +struct MyError; + +assert_eq!(MyError.name(), Some("crate_name::MyError")); +``` + +To specify your own value for error's name use the `#[fail(name = ...)]` attribute: +```rust +#[macro_use] extern crate failure; + +use std::io; + +/// MyError::name will return Some("MY_ERROR") now. +#[derive(Fail, Debug)] +#[fail(name = "MY_ERROR")] +struct MyError { + io_error: io::Error, +} + +/// MyEnumError::name will return Some("MY_VARIANT_1") for Variant1 +/// and Some("MY_VARIANT_2") for Variant2, +/// but None for Variant 3. +#[derive(Fail, Debug)] +enum MyEnumError { + #[fail(name = "MY_VARIANT_1")] + Variant1, + #[fail(name = "MY_VARIANT_2")] + Variant2(#[fail(cause)] io::Error), + Variant3, +} +``` diff --git a/examples/simple.rs b/examples/simple.rs index fc39601..20209f2 100644 --- a/examples/simple.rs +++ b/examples/simple.rs @@ -9,6 +9,7 @@ struct MyError; #[derive(Debug, Fail)] #[fail(display = "my wrapping error")] +#[fail(name = "WRAPPING_ERROR")] struct WrappingError(#[fail(cause)] MyError); fn bad_function() -> Result<(), WrappingError> { @@ -17,6 +18,6 @@ fn bad_function() -> Result<(), WrappingError> { fn main() { for cause in Fail::iter_chain(&bad_function().unwrap_err()) { - println!("{}: {}", cause.name().unwrap_or("Error"), cause); + println!("{}: {}", cause.name().unwrap(), cause); } } diff --git a/failure_derive/tests/tests.rs b/failure_derive/tests/tests.rs index 5f5ae81..c4dab04 100644 --- a/failure_derive/tests/tests.rs +++ b/failure_derive/tests/tests.rs @@ -52,7 +52,6 @@ enum EnumError { #[fail(name = "TUPLE")] TupleVariant(&'static str), #[fail(display = "An error has occurred.")] - #[fail(name = "UNIT")] UnitVariant, } @@ -71,5 +70,5 @@ fn enum_error() { let unit = EnumError::UnitVariant; let s = format!("{}", unit); assert_eq!(&s[..], "An error has occurred."); - assert_eq!(unit.name().unwrap(), "UNIT"); + assert!(unit.name().is_none()); } From 9857ce5289bd2e9ff8f4994477cfcf8b4d6e4f49 Mon Sep 17 00:00:00 2001 From: tyranron Date: Sun, 24 Mar 2019 00:43:51 +0200 Subject: [PATCH 4/5] Upd changelog --- RELEASES.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/RELEASES.md b/RELEASES.md index a02c10a..d1a3228 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -1,3 +1,7 @@ +# Version 0.1.6 + +- Add `#[fail(name = ...)]` for overriding derived error's name. + # Version 0.1.5 - Resolve a regression with error conversions (#290) From d6cdba9a864a64afdf057e1f9d923deab0d9a950 Mon Sep 17 00:00:00 2001 From: tyranron Date: Sun, 24 Mar 2019 00:48:12 +0200 Subject: [PATCH 5/5] Fix implementation --- failure_derive/src/lib.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/failure_derive/src/lib.rs b/failure_derive/src/lib.rs index 078059f..24aca30 100644 --- a/failure_derive/src/lib.rs +++ b/failure_derive/src/lib.rs @@ -108,19 +108,17 @@ fn name_body(s: &synstructure::Structure) -> Result nv.lit.clone(), - _ => unreachable!(), - }; - let pat = v.pat(); - match_body.extend(quote!(#pat => { return Some(#name_value) })); + if let Some(msg) = find_msg_of("name", &v.ast().attrs)? { + let name_value = match msg.nested[0] { + syn::NestedMeta::Meta(syn::Meta::NameValue(ref nv)) => nv.lit.clone(), + _ => unreachable!(), + }; + let pat = v.pat(); + match_body.extend(quote!(#pat => { return Some(#name_value) })); + } else { + let pat = v.pat(); + match_body.extend(quote!(#pat => { return None })); + } } Ok(quote!(match *self { #match_body })) }