-
-
Notifications
You must be signed in to change notification settings - Fork 170
Combined from and backtrace field #137
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
Combined from and backtrace field #137
Conversation
Oh, I just saw that the tests are failing. Let me investigate... |
I pushed a new commit in which the tests pass on my machine. (I missed that the relevant tests only ran in nightly and thus they didn't actually run in my initial PR. The implementation remains unchanged.) |
@@ -137,6 +137,20 @@ pub enum DataStoreError { | |||
} | |||
``` | |||
|
|||
- If a field is `#[from]` and `#[backtrace]`, the Error trait's `backtrace()` |
There was a problem hiding this comment.
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]
).
@@ -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 { |
There was a problem hiding this comment.
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! {
let source_backtrace = quote_spanned! {source.span()=> | ||
#varsource.as_dyn_error().backtrace() | ||
}; |
There was a problem hiding this comment.
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.
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() | |
} | |
}; |
@@ -235,14 +243,32 @@ fn impl_enum(input: Enum) -> TokenStream { | |||
} | |||
} | |||
(Some(backtrace_field), _) => { | |||
let source = variant.from_field().map(|f| &f.member); |
There was a problem hiding this comment.
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
.
let source = variant.from_field().map(|f| &f.member); | |
let source = variant.source_field().map(|f| &f.member); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I am merging this with my own fixes from a separate PR.
This implements support for
#[from]
and#[backtrace]
attributes used together on the same field. The purpose is to forward thebacktrace()
implementation to the source rather than capturing a new backtrace at conversion time.This was suggested in @dtolnay's comment "I would be prepared to consider a PR that does ..." #93 (review).