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

Combined from and backtrace field #137

Merged
merged 4 commits into from Aug 28, 2021

Conversation

astraw
Copy link
Contributor

@astraw astraw commented Jun 12, 2021

This implements support for #[from] and #[backtrace] attributes used together on the same field. The purpose is to forward the backtrace() 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).

@astraw
Copy link
Contributor Author

astraw commented Jun 13, 2021

Oh, I just saw that the tests are failing. Let me investigate...

@astraw
Copy link
Contributor Author

astraw commented Jun 13, 2021

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()`
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]).

@@ -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! {

Comment on lines +250 to +252
let source_backtrace = quote_spanned! {source.span()=>
#varsource.as_dyn_error().backtrace()
};
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.

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

Copy link
Owner

@dtolnay dtolnay left a 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.

@dtolnay dtolnay merged commit c45d7e4 into dtolnay:master Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants