Error when attributes have been misused

[shepmaster]

Find and fix all the Report this isn't valid here comments. Super annoying!

[tjkirch]

I'd like to take a shot at this since it bit me. :)

I've got it reporting a better error that points to the enum variant with the source attribute. (I couldn't think of a better span for the error message.) Does this seem OK? Is this the right error type?

-                    SnafuAttribute::Source(..) => { /* Report this isn't valid here? */ }
+                    SnafuAttribute::Source(..) => {
+                        return Err(vec![syn::Error::new(
+                            name.span(),
+                            "'source' attribute is only valid on individual fields; attempted to place on this variant:",
+                        )])
+                    }

(name is set to variant.ident a few lines above.)

Here's the result, with the bad code from https://gist.github.com/tjkirch/8917dd863bc284db20384fb303651ea6:

error: 'source' attribute is only valid on individual fields; attempted to place on this variant:
  --> src/main.rs:10:5
   |
10 |     Recursive { source: Box<Error> },
   |     ^^^^^^^^^

If this seems OK, I can do something similar for the other "Report this isn't valid here" comments.

[shepmaster]

I'd like to take a shot at this since it bit me. :)

Wonderful, thank you!

a better span for the error message

That’s a tough question. In an ideal world, we’d have the ability to add help text, and maybe that would point to the source field if present and say “did you mean to add it here”, but (a) I don’t think there is that ability (b) it might not be where they wanted it anyway.

The other thing I could think of would be to point at the attribute itself, so that it was very clear which attribute we are talking about. This would probably involve keeping some span information around from attribute parse time.

That being said, I think that any error will save people some heartache, so I’m down with getting messages in and iterating.

I can do something similar for the other "Report this isn't valid here" comments.

One thing to note is that I’d like to try and report as many errors in one pass as possible. For example, this should generate two errors, one for source and one for backtrace:

#[derive(Snafu)]
enum X {
    #[snafu(source)]
    #[snafu(backtrace)]
    Variant,
}

I’m still figuring out great patterns for this kind of work. In another branch, I have a macro that allows multiple back-to-back errors to happen.

In this case, we might want a vector to collect all the errors in and then return it; something like:

let mut errors = Vec::new();
for attr in attributes_from_syn(variant.attrs)? { … }
if !errors.is_empty() { return Err(errors) }

[shepmaster]

Feel free to open a draft PR early. Unfortunately for you, I’m fairly nit-picky in my reviews 😇

[tjkirch]

let mut errors = Vec::new();
for attr in attributes_from_syn(variant.attrs)? { … }
if !errors.is_empty() { return Err(errors) }

First, I tried a Vec for the entire variants.map(|variant| so we could report errors for all attributes of all variants at once instead of just the attributes of the first variant with an error. That seemed to work, and seems more complete, so I'll keep going with that unless you say it's bad for some reason :)

[shepmaster]

for the entire variants.map(|variant|

Yep, that was my eventual idea, but I wasn’t sure how complicated it would end up being, so I didn’t want to bring it up. For example, that Vec should be able to mesh with other not-immediately-fatal errors, like the thing mentioned in my WIP branch.

[tjkirch]

Yep, that was my eventual idea, but I wasn’t sure how complicated it would end up being, so I didn’t want to bring it up. For example, that Vec should be able to mesh with other not-immediately-fatal errors, like the thing mentioned in my WIP branch.

It was trivial to do the larger scope in the end. I didn't look through all of the existing errors, but I did see some that might fit better into the Vec than as an immediate return. I left that for separate consideration, though.