Improve errors for misused attributes

[tjkirch]

Fixes #105.

The first commit directly addresses the discussion in #105 by adding errors for cases of attribute misuse that were known to be wrong but didn't yet have errors. Previously, the derive would just continue without the effect the user was trying to request.

The second commit addresses similar comments elsewhere in the file that suggested we warn if attributes are duplicated. This wasn't discussed in #105 but seemed closely related.

---

Testing done:

Existing unit/doc tests still pass, and I added compile-fail tests to exercise each of the new error cases.

[shepmaster]

The invalid cases can't all be tested together, because the struct derive depends on the enum being valid

Yep, there’s “phases” of compilation where we can’t continue. My hope is basically to make those phases as few and large as possible, but there will always be some case where it just doesn’t work.

• throwing warnings, but it seemed better to me to throw errors

I don’t know that there’s an actual way to generate warnings, either…

• Should there be unit tests showing that these new cases throw errors

I’m sorry that I forgot to mention this! Take a look at the compile-fail tests. These assert that the error messages continue to work, and I strongly agree that we should have tests for each of the new errors.

I don’t know yet if “more errors in one file” or “more files” is a better approach. Probably more files, but I have a feeling that will be slower in CI at some point. For now, I’d suggest creating at least one new file for these errors.

You’ll want to rebase on master to get the fixes to these tests (causing your CI failure)

• are there any concerns about existing use cases breaking because we throw new errors

This is where warnings would be nice. They could be warnings for the current semver release, and made into errors for the next breaking change.

Since we are still pre-1.0, I don’t mind bumping the version early and often, so I’d probably move to 0.5 after this is merged if we can’t figure out warnings. If it was already 1.0, I’d be more torn about a major bump for such a thing, as it was code that wasn’t working the way people intended anyway.

[tjkirch]

compile-fail tests

Ah, thanks! Hopefully I can add these tonight.

probably move to 0.5 after this is merged

Would it help if I add commits for the update? It looks like it'd be snafu-derive/Cargo.toml, Cargo.toml, CHANGELOG.md, and src/guide/upgrading.md. I'm not sure what else you'd like in CHANGELOG but I could start it...

Or, that could wait until we have more research on warning capabilities.

[shepmaster]

if I add commits for the update

Thank you, but that’s all right! I’m lazy and don’t keep the changelog up-to-date with each PR, only doing it when I do a release. It does provide a good opportunity to review the changes and make sure that nothing obvious is missing or untested.

CHANGELOG.md and src/guide/upgrading.md

Feel free to suggest some text that you’d like to see as a seed for what will end up here!

[tjkirch]

Thanks for all your guidance @shepmaster! I marked this as ready for review, since I think I've addressed your concerns, at least for an initial implementation.

[shepmaster]

There’s one place where I was lazy and didn’t put the “report this” text:

snafu/snafu-derive/src/lib.rs

Lines 147 to 151 in da2d4ac

	let default_visibility = attributes_from_syn(attrs)?
	.into_iter()
	.flat_map(SnafuAttribute::into_visibility)
	.next()
	.unwrap_or_else(private_visibility);

Can you expand that code to also report the attributes?

[tjkirch]

There’s one place where I was lazy and didn’t put the “report this” text:

snafu/snafu-derive/src/lib.rs

Lines 147 to 151 in da2d4ac

	let default_visibility = attributes_from_syn(attrs)?
	.into_iter()
	.flat_map(SnafuAttribute::into_visibility)
	.next()
	.unwrap_or_else(private_visibility);

Can you expand that code to also report the attributes?

To make sure I understand this - you mean changing from this functional style to a loop that checks all of the attributes from attributes_from_syn and adds errors for anything that's not visibility?

[shepmaster]

changing from this functional style to a loop

Yep. I don’t see an obvious way, but if you see something that’s a bit more functional-style, feel free to go for it!

[shepmaster]

This all seems great; thanks so much for putting up with all my pedantry!

Is there anything else you think we need to add or discuss before merging?

[tjkirch]

This all seems great; thanks so much for putting up with all my pedantry!

Your knowledge and guidance are invaluable to the community. I'm thankful to have been able to take advantage, while helping a project I love. It inspires me to find other ways to contribute. So, thank you!

Is there anything else you think we need to add or discuss before merging?

I don't think so. I agree with you that the spans the new errors point to aren't ideal, but it would require a bit more wide-reaching changes that aren't strictly related, so I think it should be a separate issue. I do think this change sets it up for success with how it accepts/stores spans, but the right ones need to be made available. Do you agree that should be another issue? I can make it if so.

[shepmaster]

Sorry for the long radio silence — I was away for work. I re-read some of the error text and it seems like a few errors got mixed up; am I losing it?

[tjkirch]

I re-read some of the error text and it seems like a few errors got mixed up; am I losing it?

Nope! I didn't have the right context in a few spots. I've cleaned those up (easy to do by adding a context location to the AtMostOne) and tried to make the error wording more consistent with yours.

While doing this, I found a couple more cases of duplication I'm not catching yet, which I think I can fix fairly quickly with the same tools:

• We catch backtrace* on multiple fields in a variant, but not multiple backtrace* on a single field
• We don't catch source(from) and source(flag) together on a variant field (only wrong if flag=true?)

[shepmaster]

Thanks for the last push! Everything looks good to me now; are there any outstanding issues (other than trying to find better things to point to) we should address before merging and releasing?

[tjkirch]

Thanks for the last push! Everything looks good to me now; are there any outstanding issues (other than trying to find better things to point to) we should address before merging and releasing?

There are a couple more cases of duplication that I mentioned in #109 (comment) -- they're a little harder to fix than I initially expected, but if you're not worried about those, or you'd like them done in a separate change, then no blockers that I know of. I was assuming you'd like those now, because technically it'd be another breaking change. :)

[shepmaster]

Oh, I assumed that they were part of your last force push. I'm in no hurry, I just don't want to forget about this PR (something I unfortunately have a tendency to do). Please feel free to poke me when you have a chance to cycle back to those last few. Feel free to ask questions if you think it's something I can help with!

[tjkirch]

Feel free to ask questions if you think it's something I can help with!

I'd like to clarify whether some combinations of attributes should be errors, to be sure I'm not going to change behavior.

• I'm guessing that source(false) and source(from(...)) on the same field would be wrong, but I'm not sure - maybe the user is effectively saying "this isn't the source directly, but you can make it from ...". Error?
• Would you want to allow source(true) and source(from(...)) on the same field? Thinking charitably, the user could be double-stating their intention, but it feels more like a confusion about the from. Error?
• I'm guessing backtrace(false) and backtrace(delegate) on the same field would be wrong, though it's less clear - they could be trying to say "this shouldn't backtrace, look inside this delegate". Still feels like a confusion to me. Error?
• How about backtrace(true) and backtrace(delegate)? That seems even weirder to me. Error?

Separate from those, I think there are a couple more cases to clean up regardless:

• a field is only intended to have a backtrace delegate if it's a source, but a backtrace delegate on a non-source field is quietly ignored - I think that should be an error.
• a field shouldn't be a non-delegate backtrace and a source, but we quietly ignore the backtrace attribute on a source - I think it should be an error.

As for why it's more challenging than anticipated, aside from the above, it's the interplay between has_backtrace, is_backtrace, and is_source in the field handling code. I understand what they do now, I'm just trying to find a clean pattern for accomplishing the same checks. I lean toward adding sources/backtraces to the higher-level AtMostOne as soon as we find them, and using the is_/has_ fields for special error checks (mentioned above) after the attribute loop. I haven't tried that yet (it'd be nice to confirm the above confusions first) but I'll give an update here if I have trouble.

[shepmaster]

• source(true) and source(from(...)) on the same field
• source(false) and source(from(...)) on the same field

I could go either way on these. true and from together don't bother me. I see true as meaning "this is a source that cannot be named source" and from as "here is how you transform from one to the other". It may be better if from implied true, which it doesn't seem like it does currently.

false and source do seem like a point of confusion.

• a non-delegate backtrace and a source

This is interesting. I wonder if we should have made it such that #[snafu(source, backtrace)] implicitly meant delegate. Then we wouldn't have needed the surface-level API for backtrace(delegate) at all. That might also remove some possibilities that we need to prevent.

Do you think that this would be confusing to end users?

I lean toward adding sources/backtraces to the higher-level AtMostOne as soon as we find them, and using the is_/has_ fields for special error checks (mentioned above) after the attribute loop

Sounds reasonable. I think you'd use the AtMostOne before/during the loop, call finish (collecting errors), then perform a match on the resulting Options to ensure that there's only a reasonable combination.

[tjkirch]

true and from together don't bother me

That makes sense to me, I'll make sure this still works.

It may be better if from implied true

That makes sense and it's easy to do, I'll add that.

false and source do seem like a point of confusion.

Ok, I'll make this an error.

I wonder if we should have made it such that #[snafu(source, backtrace)] implicitly meant delegate

That's a good idea. Any backtrace on a source would be the equivalent of today's delegate, and on a non-source wouldn't change. I also lean toward that simpler API. As a new user, I think it does have a slight potential for confusion, but no more than delegate itself, to be honest :)

I think I could do this. (It'd be a third commit, of course.)

[shepmaster]

Sure. If at any point you think that this PR is getting too big, I definitely don't mind merging and creating some new issues / PRs to iterate.

[tjkirch]

If at any point you think that this PR is getting too big, I definitely don't mind merging and creating some new issues / PRs to iterate.

I think the delegate change could definitely be separate; my concern there was just that merging these breaking changes, without having the remaining breaking change ready, could impact your ability to cut a release. If you wanted to or had to for other reasons, you might need yet another release for the breaking delegate change.

If you're not worried about that, then all that would remain for this PR is preventing source(false) and source(from(...)) on the same field (as a form of misuse).

[shepmaster]

We can merge this but not immediately release anything; bundling up the breaking changes. If we need to release a patch version, I can always fork from before these commits.

[tjkirch]

Ok! I was being perhaps overly cautious.

To recap, here's the remaining work that I think could fit in this PR:

• Error on source(false) and source(from) on one field (still allowing source(true) and source(from))
• Error on multiple source or backtrace on one field

These could be other PRs, and I'll make issues for them:

• Making source(from) imply source(true)
• Removing backtrace(delegate) in favor of backtrace on a source field
• Improving the accuracy of the spans in the new errors

[tjkirch]

I've addressed the remaining work for this PR:

• source(false) and source(from) on a single field now cause an IncompatibleAttributes error
• Multiple source or backtrace attributes on a single field now error through new AtMostOne trackers

compile-fail tests were added for both. If you like the approach I took, I think this PR is ready for merging.

With the restructuring, making source(from) imply source(true) for #124 should be a one-line change in another PR. It also exposes a couple more duplicates in existing compile-fail tests, because the tests had multiple fields with source(from) that weren't counted as sources and therefore not as duplicates :)

[tjkirch]

Hmm. Rust 1.18 fails with a lifetime issue: https://github.com/shepmaster/snafu/pull/109/checks?check_run_id=176857626

I'm not sure offhand what changed between Rust 1.18 and 1.30 that allows this slice to be considered static: https://github.com/shepmaster/snafu/compare/6cb438122453631443eb361840d9499768a9d54a..27504266fd1b266e101b22de103881e84af0a01c#diff-89e1fad4d2637dae0904dce6aba08394R487

@shepmaster - assuming you're OK with this general approach, would you recommend IncompatibleAttributes store a Vec, and I use a vec![] during construction to resolve that lifetime issue, or is there a better way?

[tjkirch]

I just learned about cd compatibility-tests/v1_18 && rustup run 1.18.0 cargo test so hopefully I can prevent bad pushes in the future - sorry about that. Another tip I'm thinking about adding to a contributor's guide...

[tjkirch]

I'm not sure offhand what changed between Rust 1.18 and 1.30 that allows this slice to be considered static: https://github.com/shepmaster/snafu/compare/6cb438122453631443eb361840d9499768a9d54a..27504266fd1b266e101b22de103881e84af0a01c#diff-89e1fad4d2637dae0904dce6aba08394R487

I think it might be this change: rust-lang/rust#38865
...but there was also this one that I don't really understand :) rust-lang/rust#47408

[shepmaster]

making source(from) imply source(true) for #124 should be a one-line change in another PR.

Nice!

multiple fields with source(from) that weren't counted as sources and therefore not as duplicates

Makes sense 😉

what changed between Rust 1.18 and 1.30 that allows this slice to be considered static

Yeah, it'd be the automatic promotion of literals to statics, as you linked.

or is there a better way

You can do the promotion by hand:

static FOO: &[&str] = &["source(false)", "source(from)"];

attributes: FOO,

cd compatibility-tests/v1_18 && rustup run 1.18.0 cargo test

There's actually a "rust-toolchain" file in that directory, so you can just do cd compatibility-tests/v1_18 && cargo test. If that file wasn't there, I'd recommend doing cd compatibility-tests/v1_18 && cargo +1.18.0 test for fewer keystrokes.

adding to a contributor's guide

That would probably be a good thing!

[shepmaster]

bad pushes in the future - sorry about that

Not at all; that's the point of CI - to catch the things that we forget. That's why I try to be pedantic about adding new tests so frequently. I don't want to have to think about exactly what to remember!

It is useful to check in 1.18 locally only because it's comparatively so different to current Rust that a lot of little things sneak in while developing. My biggest mistake is around match ergonomics.

[tjkirch]

You can do the promotion by hand:

Thanks, that worked! I just pushed that fix, and hopefully this is everything we need to get this first PR merged.

There's actually a "rust-toolchain" file in that directory, so you can just do cd compatibility-tests/v1_18 && cargo test. If that file wasn't there, I'd recommend doing cd compatibility-tests/v1_18 && cargo +1.18.0 test for fewer keystrokes.

That's good to know! The other piece that was missing for me is understanding that .cirrus.yml lists some other steps that are required for each of the Rust versions - for example installing precise versions of dependencies that work with that version of Rust. Without doing anything special, I was hitting errors building dependencies, like backtrace using the crate keyword in use statements.

Do you know if there's a way to run those locally without a lot of copy+paste? I couldn't find any resources for replicating Cirrus CI locally. I see they have an issue for making a CLI (cirruslabs/cirrus-ci-docs#108) but didn't see anything else.

[shepmaster]

a way to run those locally without a lot of copy+paste

Nothing I'm aware of. However, it's been pretty standard practice in my other crates to actually have most of the CI-related junk as separate shell scripts so that they are at least easier to access outside of CI (and you get syntax highlighting, etc.)

In this case, I wonder if we should just move those to the respective Cargo.toml files...

[shepmaster]

What an amazing first contribution! Thanks so much!

[tjkirch]

it's been pretty standard practice in my other crates to actually have most of the CI-related junk as separate shell scripts

In this case, I wonder if we should just move those to the respective Cargo.toml files...

Either of these sound like they would be pretty helpful. I can create an issue if you like?

What an amazing first contribution! Thanks so much!

Thanks again for the library, and for your great feedback here! I hope to keep contributing.

[tjkirch]

Either of these sound like they would be pretty helpful. I can create an issue if you like?

Ah, you already did #128! Wow!