Add lint for holding a tracing Span Guard Across an .await

[davidbarsky]

Hello! This PR adds a new pedantic lint, [`await_holding_span_guard`] that prohibits the usage of a tracing span guard across an await boundary. If a tracing Span guard is used across an await boundary, tracing will record incorrect traces that don't appear until concurrent execution of futures occurs. In practice, this means that this issue won't occur while testing locally. While this is mentioned in tracing's documentation, I believe that this is a footgun that is best addressed through a lint or some language changes. Until such a feature can be landed, I think that a Clippy lint is the most expedient approach.

As for whether this lint should be included within Clippy, I think it should, but I can see the arguments why it shouldn't. In any case, here are the reasons in favor of inclusion within Clippy:

• Some prominent, third-party crates (regex, parking_lot, and tokio) have lints within Clippy. While tracing is probably the smallest of those three crates, it does have some non-trivial usage across the ecosystem (e.g., Tokio, rustc, chalk). Unfortunately, I don't have a principled policy in mind for what crates can and cannot be linted by Clippy and I understand if linting tracing is a step too far.
• The causes as to why tracing's guards can't be held across an await point are similar to that of mutex guards. However, instead of that causing a deadlock, incorrect traces will be reported.

Some additional notes for review:

• I'm not sure what clippy's policy for deprecating lints are, but once #[must_not_suspend] lands in rustc, I expect that tracing will move to it as soon as its MSRV allows it to. Once that occurs, I would be in favor of removing this lint from clippy.
• In keeping with the naming of #[must_not_suspend], should this lint be renamed to #[`suspend_holding_span_guard`]? Should this lint have tracing in the name?

---

changelog: Add a lint for holding a tracing Span guard across an .await boundary.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (or someone else) soon.

Please see the contribution instructions for more information.

[flip1995]

This adds another dependency to an external crate. Clippy is not a tool to implement crate-specific lints. We have exceptions for regex, serde and parking_lot .

• regex is a repo of the rust-lang org
• serde is IMHO a relic of the past, for which I don't see the justification for the 2 lints we have for it.
• parking_lot was added, because many people use those mutexes over the std mutexes in production code and there is the possibility that parking_lot will replace the std implementation at some point.

So I'm not sure if we should continue sliding down this slope of adding more and more lints for external/non-std crates.

Also those two points by you rather convince me to not add it to Clippy:

• However, instead of that causing a deadlock, incorrect traces will be reported.

So it doesn't break you program, if you do this. You just see weird output, right? You can also google why that happens I assume. It's also in the documentation. I made this point before, but it is not Clippy's resposibility to point people to documentation or explicitly spell out the documentation for them

• I expect that tracing will move to it as soon as its MSRV allows it to. Once that occurs, I would be in favor of removing this lint from clippy.

Adding a lint to Clippy with a deadline in mind when it can be removed is questionable. Instead of adding the lint, wouldn't it make more sense to put that work into the feature so it get's stabilized quicker?

---

I'll nominate this PR for discussion in our next Clippy meeting.

[davidbarsky]

Thanks for leaving your thoughts! As for:

So I'm not sure if we should continue sliding down this slope of adding more and more lints for external/non-std crates.

No worries, I think that this is should be ironed out. I know the feeling of being responsible for crates/code that you'd rather not be and finding a clear policy is tricky, and if this PR is rejected but a clear policy comes of it, then I'd consider that this PR to be a net benefit.

Adding a lint to Clippy with a deadline in mind when it can be removed is questionable. Instead of adding the lint, wouldn't it make more sense to put that work into the feature so it get's stabilized quicker?

Possibly, but the work needed to land #[must_not_suspend] seems tricky and cross-cutting (especially the resolving rust-lang/rust#69663), but I can ask and find out.

[camsteffen]

Given that this may (probably) be fixed by #[must_not_suspend] and that this lint detects a low-severity issue, I don't think it is quite worth the added dependency personally.

[bors]

☔ The latest upstream changes (presumably #8419) made this pull request unmergeable. Please resolve the merge conflicts.

[davidbarsky]

Given that this may (probably) be fixed by #[must_not_suspend] and that this lint detects a low-severity issue, I don't think it is quite worth the added dependency personally.

No worries! Then it might be better to see what can be done to improve #[must_not_suspend]. I'll close this PR in that case.