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.

Sign up for GitHub

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

Jump to bottom

Combined from and backtrace field #137

Merged
merged 4 commits into from Aug 28, 2021
Merged

Combined from and backtrace field #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86993c2
allow #[from] and #[backtrace] on same field
astraw Jun 11, 2021
da2454f
describe `#[from]` and `#[backtrace]` together
astraw Jun 12, 2021
52dc286
test combined `#[from]` and `#[backtrace]`
astraw Jun 12, 2021
cd2b9db
fixup tests for combined from and backtrace
astraw Jun 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -137,6 +137,20 @@ pub enum DataStoreError {
}
```

- If a field is `#[from]` and `#[backtrace]`, the Error trait's `backtrace()`
Copy link
Owner

Choose a reason for hiding this comment

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

#[from] is not necessary. It should be sufficient for a field to be #[source] (or just be named source, which is implicitly #[source]).

method is forwarded to the `source`.

```rust
#[derive(Error, Debug)]
pub enum MyError {
Io {
#[from]
#[backtrace]
source: io::Error,
},
}
```

- Errors may use `error(transparent)` to forward the source and Display methods
straight through to an underlying error without adding an additional message.
This would be appropriate for enums that need an "anything else" variant.
Expand Down
79 changes: 52 additions & 27 deletions impl/src/expand.rs
View file
Open in desktop
Expand Up @@ -58,18 +58,25 @@ fn impl_struct(input: Struct) -> TokenStream {
self.#source.as_dyn_error().backtrace()
}
};
let combinator = if type_is_option(backtrace_field.ty) {
if &source_field.member == backtrace {
Copy link
Owner

Choose a reason for hiding this comment

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

The changes on the next 20 lines here can be replaced with a simpler change that does the same thing and minimizes repetition:

     };
-    let combinator = if type_is_option(backtrace_field.ty) {
+    let combinator = if source == backtrace {
+        source_backtrace
+    } else if type_is_option(backtrace_field.ty) {
         quote! {

quote! {
#source_backtrace.or(self.#backtrace.as_ref())
use thiserror::private::AsDynError;
#source_backtrace
}
} else {
let combinator = if type_is_option(backtrace_field.ty) {
quote! {
#source_backtrace.or(self.#backtrace.as_ref())
}
} else {
quote! {
std::option::Option::Some(#source_backtrace.unwrap_or(&self.#backtrace))
}
};
quote! {
std::option::Option::Some(#source_backtrace.unwrap_or(&self.#backtrace))
use thiserror::private::AsDynError;
#combinator
}
};
quote! {
use thiserror::private::AsDynError;
#combinator
}
} else if type_is_option(backtrace_field.ty) {
quote! {
Expand Down Expand Up @@ -123,20 +130,21 @@ fn impl_struct(input: Struct) -> TokenStream {
}
});

let from_impl = input.from_field().map(|from_field| {
let backtrace_field = input.backtrace_field();
let from = from_field.ty;
let body = from_initializer(from_field, backtrace_field);
quote! {
#[allow(unused_qualifications)]
impl #impl_generics std::convert::From<#from> for #ty #ty_generics #where_clause {
#[allow(deprecated)]
fn from(source: #from) -> Self {
#ty #body
let from_impl = input.from_and_distinct_backtrace_fields().map(
|(from_field, backtrace_field)| {
let from = from_field.ty;
let body = from_initializer(from_field, backtrace_field);
quote! {
#[allow(unused_qualifications)]
impl #impl_generics std::convert::From<#from> for #ty #ty_generics #where_clause {
#[allow(deprecated)]
fn from(source: #from) -> Self {
#ty #body
}
}
}
}
});
},
);

let error_trait = spanned_error_trait(input.original);

Expand Down Expand Up @@ -235,14 +243,32 @@ fn impl_enum(input: Enum) -> TokenStream {
}
}
(Some(backtrace_field), _) => {
let source = variant.from_field().map(|f| &f.member);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think from_field is the right field to check here — it should be source_field. Otherwise this will generate noncompilable code in the case of #[source] #[backtrace] inside an enum, because the generated code would treat the source field as if its type were Backtrace.

Suggested change
let source = variant.from_field().map(|f| &f.member);
let source = variant.source_field().map(|f| &f.member);

let backtrace = &backtrace_field.member;
let body = if type_is_option(backtrace_field.ty) {
quote!(backtrace.as_ref())
if source == Some(backtrace) {
let varsource = quote!(source);
let source_backtrace = quote_spanned! {source.span()=>
#varsource.as_dyn_error().backtrace()
};
Comment on lines +250 to +252
Copy link
Owner

Choose a reason for hiding this comment

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

Thiserror permits the source field to be optional. This code should handle that like in the case above this one.

thiserror/impl/src/expand.rs

Lines 212 to 220 in 031fea6

let source_backtrace = if type_is_option(source_field.ty) {
quote_spanned! {source.span()=>
#varsource.as_ref().and_then(|source| source.as_dyn_error().backtrace())
}
} else {
quote_spanned! {source.span()=>
#varsource.as_dyn_error().backtrace()
}
};


quote! {
#ty::#ident {
#source: #varsource,
..
} => {
use thiserror::private::AsDynError;
#source_backtrace
}
}
} else {
quote!(std::option::Option::Some(backtrace))
};
quote! {
#ty::#ident {#backtrace: backtrace, ..} => #body,
let body = if type_is_option(backtrace_field.ty) {
quote!(backtrace.as_ref())
} else {
quote!(std::option::Option::Some(backtrace))
};
quote! {
#ty::#ident {#backtrace: backtrace, ..} => #body,
}
}
}
(None, _) => quote! {
Expand Down Expand Up @@ -315,8 +341,7 @@ fn impl_enum(input: Enum) -> TokenStream {
};

let from_impls = input.variants.iter().filter_map(|variant| {
let from_field = variant.from_field()?;
let backtrace_field = variant.backtrace_field();
let (from_field, backtrace_field) = variant.from_and_distinct_backtrace_fields()?;
let variant = &variant.ident;
let from = from_field.ty;
let body = from_initializer(from_field, backtrace_field);
Expand Down
28 changes: 28 additions & 0 deletions impl/src/prop.rs
View file
Open in desktop
Expand Up @@ -6,6 +6,20 @@ impl Struct<'_> {
from_field(&self.fields)
}

pub(crate) fn from_and_distinct_backtrace_fields(&self) -> Option<(&Field, Option<&Field>)> {
self.from_field().map(|from_field| {
if let Some(backtrace_field) = self.backtrace_field() {
if backtrace_field.member == from_field.member {
(from_field, None)
} else {
(from_field, Some(backtrace_field))
}
} else {
(from_field, None)
}
})
}

pub(crate) fn source_field(&self) -> Option<&Field> {
source_field(&self.fields)
}
Expand Down Expand Up @@ -47,6 +61,20 @@ impl Variant<'_> {
from_field(&self.fields)
}

pub(crate) fn from_and_distinct_backtrace_fields(&self) -> Option<(&Field, Option<&Field>)> {
self.from_field().map(|from_field| {
if let Some(backtrace_field) = self.backtrace_field() {
if backtrace_field.member == from_field.member {
(from_field, None)
} else {
(from_field, Some(backtrace_field))
}
} else {
(from_field, None)
}
})
}

pub(crate) fn source_field(&self) -> Option<&Field> {
source_field(&self.fields)
}
Expand Down
16 changes: 16 additions & 0 deletions src/lib.rs
View file
Open in desktop
Expand Up @@ -161,6 +161,22 @@
//! # };
//! ```
//!
//! - If a field is `#[from]` and `#[backtrace]`, the Error trait's `backtrace()`
//! method is forwarded to the field.
//!
//! ```rust
//! # const IGNORE: &str = stringify! {
//! #[derive(Error, Debug)]
//! pub enum MyError {
//! Io {
//! #[from]
//! #[backtrace]
//! source: io::Error,
//! },
//! }
//! # };
//! ```
//!
//! - Errors may use `error(transparent)` to forward the source and Display
//! methods straight through to an underlying error without adding an
//! additional message. This would be appropriate for enums that need an
Expand Down
39 changes: 37 additions & 2 deletions tests/test_backtrace.rs
View file
Open in desktop
Expand Up @@ -6,9 +6,16 @@ use thiserror::Error;
#[error("...")]
pub struct Inner;

#[cfg(thiserror_nightly_testing)]
#[derive(Error, Debug)]
#[error("...")]
pub struct InnerBacktrace {
backtrace: std::backtrace::Backtrace,
}

#[cfg(thiserror_nightly_testing)]
pub mod structs {
use super::Inner;
use super::{Inner, InnerBacktrace};
use std::backtrace::Backtrace;
use std::error::Error;
use std::sync::Arc;
Expand Down Expand Up @@ -50,6 +57,14 @@ pub mod structs {
backtrace: Backtrace,
}

#[derive(Error, Debug)]
#[error("...")]
pub struct CombinedBacktraceFrom {
#[from]
#[backtrace]
source: InnerBacktrace,
}

#[derive(Error, Debug)]
#[error("...")]
pub struct OptBacktraceFrom {
Expand Down Expand Up @@ -93,6 +108,11 @@ pub mod structs {
let error = BacktraceFrom::from(Inner);
assert!(error.backtrace().is_some());

let error = CombinedBacktraceFrom::from(InnerBacktrace {
backtrace: Backtrace::capture(),
});
assert!(error.backtrace().is_some());

let error = OptBacktraceFrom::from(Inner);
assert!(error.backtrace().is_some());

Expand All @@ -103,7 +123,7 @@ pub mod structs {

#[cfg(thiserror_nightly_testing)]
pub mod enums {
use super::Inner;
use super::{Inner, InnerBacktrace};
use std::backtrace::Backtrace;
use std::error::Error;
use std::sync::Arc;
Expand Down Expand Up @@ -153,6 +173,16 @@ pub mod enums {
},
}

#[derive(Error, Debug)]
pub enum CombinedBacktraceFrom {
#[error("...")]
Test {
#[from]
#[backtrace]
source: InnerBacktrace,
},
}

#[derive(Error, Debug)]
pub enum OptBacktraceFrom {
#[error("...")]
Expand Down Expand Up @@ -200,6 +230,11 @@ pub mod enums {
let error = BacktraceFrom::from(Inner);
assert!(error.backtrace().is_some());

let error = CombinedBacktraceFrom::from(InnerBacktrace {
backtrace: Backtrace::capture(),
});
assert!(error.backtrace().is_some());

let error = OptBacktraceFrom::from(Inner);
assert!(error.backtrace().is_some());

Expand Down