Tracking issue for stabilizing the `#[coverage(..)]` attribute

[Zalathar]

This is a sub-issue of #84605 for tracking the remaining obstacles to stabilization of the coverage attribute. The #[coverage(off)] and #[coverage(on)] attributes provide hints for whether -Cinstrument-coverage should instrument a particular function or not.

The feature gate for the issue is #![feature(coverage_attribute)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Discussion comments will get marked as off-topic or deleted.
Repeated discussions on the tracking issue may lead to the tracking issue getting locked.

Steps

• Draft and review user-facing documentation (guide-level and reference-level) that describes the feature, and clearly indicates which aspects are stably guaranteed vs subject to change.
• Check that the feature is adequately tested.
• Get clear sign-off from T-lang, including approval of the documented stability boundary.
  • Bring behavior around closures in line with optimize; see Stabilize optimize attribute #157273 (comment).
• Get clear sign-off from the de-facto code owner (Zalathar) and T-compiler.
• Adjust error messages and error-code documentation to reflect the current design.
  • Update #[coverage(..)] attribute error messages to match the current implementation #134750
• New stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

• Which aspects of the attribute are stably guaranteed vs subject to change?

Implementation history

This functionality was originally introduced as an unstable feature, without a formal RFC. After a period of inactivity, there was a user-led push for stabilization, leading to further design and implementation changes.

A stabilization PR was briefly merged on nightly due to a process mixup, then reverted. This tracking issue picks up after that revert.

• (add earlier PRs here as desired)
• coverage: Make #[coverage(..)] apply recursively to nested functions #126721
• Stabilize #[coverage] attribute #130766
• Revert stabilization of the #[coverage(..)] attribute #134672

[jieyouxu]

Broken build after updating: coverage is ambiguous; ambiguous because of a name conflict with a builtin attribute #121157 was an untriaged regression due to ambiguous resolution when the unstable coverage attribute as a builtin attribute was added (might be a general problem with feature gating of builtin attribute versus something like #[test] that's exposed via prelude, as mentioned by oli here). Of course, it has already hit stable in 1.74.0, so T-lang/T-compiler may want to decide if that is an acceptable breakage, and if any mitigation/changes are warranted (but that's already a long time ago).

EDIT: this is probably not relevant for stabilization of this attribute now because that ship has already sailed, so moving to a separate discussion.

[clarfonthey]

What feels like a large source of confusion here is that this attribute isn't actually changing the status quo in terms of the stabilisation of -C instrument-coverage itself.

Even though -C instrument-coverage is usable on the stable compiler, its output has been prone to change between compiler versions and will likely remain so for a very long time. People are not expected to rely on its output and stabilising #[coverage] does not change that; the reason why the flag itself is stable is because we're comfortable with the idea of people using that flag to instrument coverage, even if the resulting instrumentation can change.

I feel like this attribute should have the same treatment: we're comfortable accepting that #[coverage(off)] and #[coverage(on)] will be reasonable to have in the language from now on, and we're even comfortable with the idea of what those mean relative to the minimally stabilised meaning of -C instrument-coverage, but that's it.

We can't guarantee that instrumentation is emitted since, at least for the moment, dead code removal will also remove code from coverage instrumentation. We don't even guarantee that code with #[coverage(off)] isn't emitted, even though that seems pretty reasonable to ensure, just out of an abundance of caution. The only guarantees we make are that off and on are opposites and applied recursively to items, which feels very reasonable. Unless anyone has any particular objections, it feels likely that this is the one and only change to documentation that will come with stabilisation: rust-lang/reference#1628

I think that part of the reason why this "feels" like a large language change is because of the fact that, until now, coverage instrumentation has not been mentioned outside of the compiler realm, and thus the lang team hasn't really needed to comment on the stability of coverage instrumentation. In the reference docs I proposed initially, I only clarified that the attribute indicates whether instrumentation should be emitted, and not what the instrumentation does or whether it's actually emitted. I'm definitely open to a better way of wording this that makes this aspect more clear.

Responding specifically to some of the remaining items below:

---

It's entirely reasonable to say that we should have a better guide-level explanation of code coverage available, but it feels like the lack of said explanation is an explicit decision at the moment. Right now, I treat code coverage as a diagnostic tool only: it can help me determine if I'm missing obvious cases in tests, but I can't rely on it for anything, including whether my code is adequately tested. There are tools like cargo-llvm-cov, but those aren't official and the actual reference docs of the feature are quite technical. Certainly, you shouldn't gate merges on code coverage, since that definitely will break.

The reason why I think it's okay to stabilise the attribute now is that #[coverage(off)] has a very clear meaning regardless of the current and future state of code coverage: whatever you're going to do to make this function show up on coverage, I don't want it. And #[coverage(on)] is similarly useful to disable #[coverage(off)]. Macros in particular can add #[coverage(off)] to their emitted items to avoid cluttering up instrumentation with code that users didn't write themselves. And all of this can happen on stable because, well, ultimately, it's a toggle switch for something which isn't fully defined.

In terms of testing, I can't comment for the entirety of coverage instrumentation, but it looks like everything for the attribute in particular is quite thorough, based upon what I saw when making the stabilisation PR. @ehuss had mentioned verifying that trait methods don't get infected by #[coverage(off)], but this has since been added in #134517. Everything mentioned in the reference page I linked seems to be tested.

I definitely agree that we should make sure that everyone is on board with the stabilisation before it happens, but I'm not sure if there's anything else we need to do to be "ready" to start collecting that feedback and running the FCP for the lang team. As I mentioned in my PR, I'm more than happy to do the work on the stabilisation PR and reference docs, including whatever merge conflicts show up until everyone's on board.

[Zalathar]

I am already very unhappy with how much stress and hassle you have created for me by trying to rush this stabilization while dismissing real concerns.

Seeing more of the same has more than exhausted my patience.

[clarfonthey]

Stressing you out was never my intention, and if the answer is just to wait until you've got less on your plate then I'm fine with that too; genuinely. I mean, I should have expected that you'd be stress given the revert and coordinating that, but this is the first time I'm hearing about this specifically, either because I'm bad at reading (very possible; I even missed this issue entirely initially) or it's the first time you've mentioned it.

Like, maintainer burnout is a real thing and I'm only able to work on this type of stuff because I've been unemployed for almost two years at this point, and counter-intuitively, I work on FOSS stuff because it helps me forget the impending existential crisis of unemployment. Having to manage much larger commitments at the same time is substantially more difficult.

And, it's especially difficult because having less time to work on this means less time to express your concerns, meaning people like me come by and just assume that means you have no concerns, because you haven't fully expressed them. And I'm sorry for that; genuinely, I should have been able to understand that better, but I haven't because I've not had that full context.

I'll close the stabilisation PR for now and we can revisit this topic at a later point when you're willing.

[psandana]

@Zalathar have you been able to move forward with this? It has been a year without any changes. Do you need any help?