Repository: shepmaster/snafu (rust)

Issue:
Replacing `backtrace(delegate)` with `backtrace` on a source field
In #109, an idea was raised - is `backtrace(delegate)` needed as a separate attribute?

Its purpose is to signal that another error type will handle the backtrace.  (See https://docs.rs/snafu/0.4.3/snafu/guide/attributes/index.html#controlling-backtraces).  For this to work, that other type, where you add the `backtrace(delegate)` attribute, must be a source field.  And if it's a source field, it can't *directly* be a backtrace field; sources must be other error types, and backtraces must be `Backtrace`.

So, can we simplify and say that backtrace delegation happens by marking a source field with a `backtrace` attribute?

Example with a typical "source" field:

```rust
#[derive(Debug, Snafu)]
enum Error {
    MyError {
        #[snafu(backtrace)]
        source: another::Error,
    },
}
```

Example with a `source` attribute, required to handle another field name:
```rust
#[derive(Debug, Snafu)]
enum Error {
    MyError {
        #[snafu(source, backtrace)]
        my_source: another::Error,
    },
}
```

Source transformation shouldn't be affected; and with #124 (`source(from)` implying `source(true)`) even the most complicated case seems relatively simple:

```rust
#[derive(Debug, Snafu)]
enum Error {
    MyError {
        #[snafu(backtrace)]
        #[snafu(source(from(another::Error, Box::new)))]
        my_source: Box<another::Error>,
    },
}
```

Pull request description:
Replace `backtrace(delegate)` with `backtrace` on a source field
Fixes #125, see it for background.

As I was playing with some options, I came to believe that the clearest representation of this change is adding a flag to `SourceField` that represents whether the source field is a backtrace delegate.  We don't need the `backtrace_delegate` field in `VariantInfo` with its `Option<Field>`; all delegates are source fields, and we already have the source field information.

I was then able to simplify `parse_snafu_enum` by removing `backtrace_delegates` and `backtrace_delegate_location` - we can detect whether it's a delegate just by seeing if we got a backtrace attribute on a source field, both of which were already tracked, so we don't need separate tracking methods.

I added a friendly error if we do see `backtrace(delegate)`, and updated the tests and guide.  Updated compile-fail tests show the (even better?) errors for duplication.

No idea why rustfmt decided to rewrap some unchanged paragraphs, sorry :(

I'm interested in any feedback on this approach of moving knowledge of backtrace delegation to `SourceField`, and of course any other general feedback.

Patch:
diff --git a/snafu-derive/src/lib.rs b/snafu-derive/src/lib.rs
index 2285243..be7f2f8 100644
--- a/snafu-derive/src/lib.rs
+++ b/snafu-derive/src/lib.rs
@@ -44,7 +44,6 @@ struct VariantInfo {
     name: syn::Ident,
     source_field: Option<SourceField>,
     backtrace_field: Option<Field>,
-    backtrace_delegate: Option<Field>,
     user_fields: Vec<Field>,
     display_format: Option<UserInput>,
     doc_comment: String,
@@ -72,6 +71,7 @@ impl Field {
 struct SourceField {
     name: syn::Ident,
     transformation: Transformation,
+    backtrace_delegate: bool,
 }
 
 impl SourceField {
@@ -474,8 +474,6 @@ fn parse_snafu_enum(
             let mut user_fields = Vec::new();
             let mut source_fields = AtMostOne::new("source", "within an error variant");
             let mut backtrace_fields = AtMostOne::new("backtrace", "within an error variant");
-            let mut backtrace_delegates =
-                AtMostOne::new("backtrace(delegate)", "within an error variant");
 
             for syn_field in fields {
                 let span = syn_field.span();
@@ -504,10 +502,6 @@ fn parse_snafu_enum(
                 let mut source_opt_out = false;
                 let mut backtrace_opt_out = false;
 
-                // Keep track of the location of the backtrace delegate field, if any; duplication
-                // is already checked because backtrace delegates are counted in backtrace_attrs.
-                let mut backtrace_delegate_location = None;
-
                 for attr in attributes_from_syn(syn_field.attrs.clone())? {
                     match attr {
                         SnafuAttribute::Source(tts, ss) => {
@@ -517,7 +511,8 @@ fn parse_snafu_enum(
                                     Source::Flag(v) => {
                                         // If we've seen a `source(from)` then there will be a
                                         // `Some` value in `source_attrs`.
-                                        let seen_source_from = source_attrs.iter()
+                                        let seen_source_from = source_attrs
+                                            .iter()
                                             .map(|&(ref val, ref _location)| val)
                                             .any(Option::is_some);
                                         if !v && seen_source_from {
@@ -550,19 +545,13 @@ fn parse_snafu_enum(
                                 }
                             }
                         }
-                        SnafuAttribute::Backtrace(tts, b) => match b {
-                            Backtrace::Flag(v) => {
-                                if v {
-                                    backtrace_attrs.add((), tts);
-                                } else {
-                                    backtrace_opt_out = true;
-                                }
-                            },
-                            Backtrace::Delegate => {
-                                backtrace_attrs.add((), tts.clone());
-                                backtrace_delegate_location = Some(tts);
+                        SnafuAttribute::Backtrace(tts, v) => {
+                            if v {
+                                backtrace_attrs.add((), tts);
+                            } else {
+                                backtrace_opt_out = true;
                             }
-                        },
+                        }
                         SnafuAttribute::Visibility(tts, ..) => {
                             errors.add(
                                 tts,
@@ -593,29 +582,23 @@ fn parse_snafu_enum(
                 let (backtrace_attr, errs) = backtrace_attrs.finish_with_location();
                 errors.extend(errs);
 
-                let source_attr = source_attr
-                    .or_else(|| {
-                        if field.name == "source" && !source_opt_out {
-                            Some((None, syn_field.clone().into_token_stream()))
-                        } else {
-                            None
-                        }
-                    });
-
-                let backtrace_attr = backtrace_attr
-                    .or_else(|| {
-                        if field.name == "backtrace" && !backtrace_opt_out {
-                            Some(((), syn_field.clone().into_token_stream()))
-                        } else {
-                            None
-                        }
-                    });
+                let source_attr = source_attr.or_else(|| {
+                    if field.name == "source" && !source_opt_out {
+                        Some((None, syn_field.clone().into_token_stream()))
+                    } else {
+                        None
+                    }
+                });
 
-                if let Some((maybe_transformation, location)) = source_attr {
-                    if let Some(location) = backtrace_delegate_location {
-                        backtrace_delegates.add(field.clone(), location);
+                let backtrace_attr = backtrace_attr.or_else(|| {
+                    if field.name == "backtrace" && !backtrace_opt_out {
+                        Some(((), syn_field.clone().into_token_stream()))
+                    } else {
+                        None
                     }
+                });
 
+                if let Some((maybe_transformation, location)) = source_attr {
                     let Field { name, ty } = field;
                     let transformation = maybe_transformation
                         .map(|(ty, expr)| Transformation::Transform { ty, expr })
@@ -625,6 +608,9 @@ fn parse_snafu_enum(
                         SourceField {
                             name,
                             transformation,
+                            // Specifying `backtrace` on a source field is how you request
+                            // delegation of the backtrace to the source error type.
+                            backtrace_delegate: backtrace_attr.is_some(),
                         },
                         location,
                     );
@@ -635,26 +621,26 @@ fn parse_snafu_enum(
                 }
             }
 
-            let (source_field, errs) = source_fields.finish();
+            let (source, errs) = source_fields.finish_with_location();
             errors.extend(errs);
 
             let (backtrace, errs) = backtrace_fields.finish_with_location();
             errors.extend(errs);
 
-            let (backtrace_delegate, errs) = backtrace_delegates.finish_with_location();
-            errors.extend(errs);
-
-            if let (Some(backtrace), Some(backtrace_delegate)) = (backtrace.as_ref(), backtrace_delegate.as_ref()) {
-                let backtrace_location = backtrace.1.clone();
-                let backtrace_delegate_location = backtrace_delegate.1.clone();
-                errors.add(
-                    backtrace_location,
-                    "Cannot have `backtrace` and `backtrace(delegate)` fields in the same error variant",
-                );
-                errors.add(
-                    backtrace_delegate_location,
-                    "Cannot have `backtrace` and `backtrace(delegate)` fields in the same error variant",
-                );
+            match (&source, &backtrace) {
+                (&Some(ref source), &Some(ref backtrace)) if source.0.backtrace_delegate => {
+                    let source_location = source.1.clone();
+                    let backtrace_location = backtrace.1.clone();
+                    errors.add(
+                        source_location,
+                        "Cannot have `backtrace` field and `backtrace` attribute on a source field in the same error variant",
+                    );
+                    errors.add(
+                        backtrace_location,
+                        "Cannot have `backtrace` field and `backtrace` attribute on a source field in the same error variant",
+                    );
+                }
+                _ => {} // no conflict
             }
 
             let (display_format, errs) = display_formats.finish();
@@ -665,9 +651,8 @@ fn parse_snafu_enum(
 
             Ok(VariantInfo {
                 name,
-                source_field,
+                source_field: source.map(|(val, _tts)| val),
                 backtrace_field: backtrace.map(|(val, _tts)| val),
-                backtrace_delegate: backtrace_delegate.map(|(val, _tts)| val),
                 user_fields,
                 display_format,
                 doc_comment,
@@ -919,10 +904,9 @@ impl syn::parse::Parse for Source {
     }
 }
 
-enum Backtrace {
-    Flag(bool),
-    Delegate,
-}
+// Having a Backtrace newtype and implementing Parse gives us a way to handle careful parsing
+// errors outside of `impl Parse for SnafuAttribute`
+struct Backtrace(bool);
 
 impl syn::parse::Parse for Backtrace {
     fn parse(input: syn::parse::ParseStream) -> SynResult<Self> {
@@ -932,17 +916,17 @@ impl syn::parse::Parse for Backtrace {
 
         if lookahead.peek(LitBool) {
             let val: LitBool = input.parse()?;
-            Ok(Backtrace::Flag(val.value))
+            Ok(Backtrace(val.value))
         } else if lookahead.peek(Ident) {
             let name: Ident = input.parse()?;
 
             if name == "delegate" {
-                Ok(Backtrace::Delegate)
-            } else {
                 Err(syn::Error::new(
                     name.span(),
-                    "expected `true`, `false`, or `delegate`",
+                    "`backtrace(delegate)` has been removed; use `backtrace` on a source field",
                 ))
+            } else {
+                Err(syn::Error::new(name.span(), "expected `true` or `false`"))
             }
         } else {
             Err(lookahead.error())
@@ -961,7 +945,7 @@ enum SnafuAttribute {
     Display(proc_macro2::TokenStream, UserInput),
     Visibility(proc_macro2::TokenStream, UserInput),
     Source(proc_macro2::TokenStream, Vec<Source>),
-    Backtrace(proc_macro2::TokenStream, Backtrace),
+    Backtrace(proc_macro2::TokenStream, bool),
     DocComment(proc_macro2::TokenStream, String),
 }
 
@@ -998,10 +982,11 @@ impl syn::parse::Parse for SnafuAttribute {
         } else if name == "backtrace" {
             let lookahead = input.lookahead1();
             if input.is_empty() || lookahead.peek(Comma) {
-                Ok(SnafuAttribute::Backtrace(input_tts, Backtrace::Flag(true)))
+                Ok(SnafuAttribute::Backtrace(input_tts, true))
             } else if lookahead.peek(Paren) {
                 let v: MyParens<Backtrace> = input.parse()?;
-                Ok(SnafuAttribute::Backtrace(input_tts, v.0))
+                let backtrace = v.0;
+                Ok(SnafuAttribute::Backtrace(input_tts, backtrace.0))
             } else {
                 Err(lookahead.error())
             }
@@ -1345,6 +1330,7 @@ impl<'a> quote::ToTokens for ContextSelector<'a> {
                     let SourceField {
                         name: ref source_name,
                         transformation: ref source_transformation,
+                        ..
                     } = *source_field;
 
                     let source_ty2 = source_transformation.ty();
@@ -1584,32 +1570,35 @@ impl<'a> ErrorCompatImpl<'a> {
         self.0.variants.iter().map(|variant| {
             let VariantInfo {
                 name: ref variant_name,
+                ref source_field,
                 ref backtrace_field,
-                ref backtrace_delegate,
                 ..
             } = *variant;
 
 
-            if let Some(ref backtrace_delegate) = *backtrace_delegate {
-                let Field {
-                    name: ref field_name,
-                    ..
-                } = *backtrace_delegate;
-                quote! {
-                    #enum_name::#variant_name { ref #field_name, .. } => { snafu::ErrorCompat::backtrace(#field_name) }
-                }
-
-            } else if let Some(ref backtrace_field) = *backtrace_field  {
-                let Field {
-                    name: ref field_name,
-                    ..
-                } = *backtrace_field;
-                quote! {
-                    #enum_name::#variant_name { ref #field_name, .. } => { std::option::Option::Some(#field_name) }
+            match (source_field, backtrace_field) {
+                (&Some(ref source_field), _) if source_field.backtrace_delegate => {
+                    let SourceField {
+                        name: ref field_name,
+                        ..
+                    } = *source_field;
+                    quote! {
+                        #enum_name::#variant_name { ref #field_name, .. } => { snafu::ErrorCompat::backtrace(#field_name) }
+                    }
+                },
+                (_, &Some(ref backtrace_field)) => {
+                    let Field {
+                        name: ref field_name,
+                        ..
+                    } = *backtrace_field;
+                    quote! {
+                        #enum_name::#variant_name { ref #field_name, .. } => { std::option::Option::Some(#field_name) }
+                    }
                 }
-            } else {
-                quote! {
-                    #enum_name::#variant_name { .. } => { std::option::Option::None }
+                _ => {
+                    quote! {
+                        #enum_name::#variant_name { .. } => { std::option::Option::None }
+                    }
                 }
             }
         }).collect()
diff --git a/src/guide/attributes.md b/src/guide/attributes.md
index 42d143b..435ff7e 100644
--- a/src/guide/attributes.md
+++ b/src/guide/attributes.md
@@ -172,7 +172,8 @@ enum Error {
 
 If your error contains other SNAFU errors which can report
 backtraces, you may wish to delegate returning a backtrace to
-those errors. Use `#[snafu(backtrace(delegate))]` to specify this:
+those errors. To specify this, use `#[snafu(backtrace)]` on the
+source field representing the other error:
 
 ```rust
 # mod another {
@@ -184,7 +185,7 @@ those errors. Use `#[snafu(backtrace(delegate))]` to specify this:
 #[derive(Debug, Snafu)]
 enum Error {
     MyError {
-        #[snafu(backtrace(delegate))]
+        #[snafu(backtrace)]
         source: another::Error,
     },
 }


Test patch:
diff --git a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.rs b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.rs
index 48bdcb3..b5a5695 100644
--- a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.rs
+++ b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.rs
@@ -4,8 +4,8 @@ use snafu::Snafu;
 enum EnumError {
     AVariant {
         // Should detect second attribute as duplicate
-        #[snafu(backtrace(delegate))]
-        #[snafu(backtrace(delegate))]
+        #[snafu(backtrace)]
+        #[snafu(backtrace)]
         source: String,
     },
 }
diff --git a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.stderr b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.stderr
index 700fa65..c1b57f2 100644
--- a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.stderr
+++ b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate-attributes.stderr
@@ -1,5 +1,5 @@
 error: Multiple `backtrace` attributes are not supported on a field
  --> $DIR/duplication-backtrace-delegate-attributes.rs:8:17
   |
-8 |         #[snafu(backtrace(delegate))]
-  |                 ^^^^^^^^^^^^^^^^^^^
+8 |         #[snafu(backtrace)]
+  |                 ^^^^^^^^^
diff --git a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.rs b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.rs
index d6b5f9a..b5c9f4f 100644
--- a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.rs
+++ b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.rs
@@ -7,7 +7,7 @@ enum EnumError {
         backtrace: Backtrace,
 
         // Second backtrace, this time a delegate, can't have both
-        #[snafu(backtrace(delegate))]
+        #[snafu(backtrace)]
         source: String,
     },
 }
diff --git a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.stderr b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.stderr
index 7b10e21..d2d4624 100644
--- a/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.stderr
+++ b/compatibility-tests/compile-fail/tests/ui/duplication-backtrace-delegate.stderr
@@ -1,11 +1,12 @@
-error: Cannot have `backtrace` and `backtrace(delegate)` fields in the same error variant
+error: Cannot have `backtrace` field and `backtrace` attribute on a source field in the same error variant
+  --> $DIR/duplication-backtrace-delegate.rs:10:9
+   |
+10 | /         #[snafu(backtrace)]
+11 | |         source: String,
+   | |______________________^
+
+error: Cannot have `backtrace` field and `backtrace` attribute on a source field in the same error variant
  --> $DIR/duplication-backtrace-delegate.rs:7:9
   |
 7 |         backtrace: Backtrace,
   |         ^^^^^^^^^^^^^^^^^^^^
-
-error: Cannot have `backtrace` and `backtrace(delegate)` fields in the same error variant
-  --> $DIR/duplication-backtrace-delegate.rs:10:17
-   |
-10 |         #[snafu(backtrace(delegate))]
-   |                 ^^^^^^^^^^^^^^^^^^^
diff --git a/tests/backtrace_delegation.rs b/tests/backtrace_delegation.rs
index 0da1295..60cfb3c 100644
--- a/tests/backtrace_delegation.rs
+++ b/tests/backtrace_delegation.rs
@@ -18,7 +18,7 @@ mod house {
 #[derive(Debug, Snafu)]
 enum Error {
     MovieTrope {
-        #[snafu(backtrace(delegate))]
+        #[snafu(backtrace)]
         source: house::Error,
     },
 }
diff --git a/tests/multiple_attributes.rs b/tests/multiple_attributes.rs
index 44358f6..b82ce37 100644
--- a/tests/multiple_attributes.rs
+++ b/tests/multiple_attributes.rs
@@ -17,7 +17,7 @@ mod error {
         #[snafu(visibility(pub(super)), display("Moo"))]
         Alpha {
             // Ensure we can have multiple field attributes as well
-            #[snafu(source, backtrace(delegate))]
+            #[snafu(source, backtrace)]
             cause: InnerError,
         },
     }
diff --git a/tests/source_and_backtrace.rs b/tests/source_and_backtrace.rs
new file mode 100644
index 0000000..fd1c30b
--- /dev/null
+++ b/tests/source_and_backtrace.rs
@@ -0,0 +1,28 @@
+extern crate snafu;
+
+use snafu::{Backtrace, ErrorCompat, ResultExt, Snafu};
+
+#[derive(Debug, Snafu)]
+enum InnerError {
+    InnerVariant { backtrace: Backtrace },
+}
+
+#[derive(Debug, Snafu)]
+enum Error {
+    SourceAndBacktraceAttrs {
+        // Testing source and backtrace attributes; the field should be recognized as a source,
+        // and allow us to get a backtrace delegated from the source error
+        #[snafu(source, backtrace)]
+        cause: InnerError,
+    },
+}
+
+fn example() -> Result<(), Error> {
+    InnerVariant.fail().context(SourceAndBacktraceAttrs)
+}
+
+#[test]
+fn delegated_backtrace_works() {
+    let e = example().unwrap_err();
+    ErrorCompat::backtrace(&e).unwrap();
+}