Tracking issue for function attribute #[coverage]

[richkadel]

This issue will track the approval and stabilization of the attribute coverage, needed to give developers a way to "hide" a function from the coverage instrumentation enabled by rustc -Z instrument-coverage.

The Eq trait in the std library implements a marker function that is not meant to be executed, but results in an uncovered region in all rust programs that derive Eq. This attribute will allow the compiler to skip that function, and remove the uncovered regions.

[richkadel]

cc: @tmandry @wesleywiser

[nagisa]

Nit: should probably follow the pattern of #[coverage(...)] much like #[inline] and many other attributes.

[Swatinem]

Just a wish that I have:
Instead of making this a function level attr, it should work equally on mod or even just blocks.

Another thing I wish for would be a crate-level attr that would disable coverage for every #[test] fn.

[richkadel]

Nit: should probably follow the pattern of #[coverage(...)] much like #[inline] and many other attributes.

I considered this, and discussed it in the initial PR. I'll copy those comments to this tracking issue, to make it easer to read and reply:

@tmandry said:

I also like the idea of combining the first two comments on #84605 by making it #[coverage(always/never)] and making some things (like tests?) disabled by default, but #[coverage(always)] could be used to override this. The only drawbacks I see are that #[coverage] on its own seems meaningless (unlike #[inline]) and these are longer to type.

The particulars of which things to make covered by default are a little hazy to me at this point, though. Really the problem I want to fix is that assert messages in tests appear uncovered, but that's kind of the point :)

@richkadel replied:

I also like the idea of combining the first two comments on #84605 by making it #[coverage(always/never)] and making some things (like tests?) disabled by default

Yes, I saw the comments, and I do want to consider them. Since this is feature-gated (and fixes a bug), I'd like to land this with no_coverage initially, and follow up with a different PR if we decide to change how it works.

I did consider something like #[coverage(never)], but (as you point out) I currently feel that #[coverage] or #[coverage(always)] doesn't really make sense to me.

In fact, I think coverage is especially relevant to tests, so I wouldn't want to disable it for tests by default, or require the user add the attribute in order to get test coverage.

The only other coverage-related attribute we may want to consider, in the future, is an attribute to enable coverage of unwind paths (#78544). But the MIR InstrumentCoverage pass isn't close to ready to support that yet.

It's definitely not uncommon for attributes to start with no_, and I just felt that no_coverage is much easier to understand and easier to remember compared to coverage(never) or coverage(none) or coverage(disable). (Also, never might be consistent with the terminology for inline, but the semantics are very different. inline has the notion of "best effort" or "when appropriate" inlining; but those semantics don't apply as well to coverage I think.)

@tmandry replied:

Agreed that coverage is relevant to tests, but I don't know if it's very relevant to the test functions themselves. But like I said, there might be a different way of getting at the problem I'm talking about.

In fact, your reply made me think of an approach I like better, which is removing counters that are along a path that always panics. We don't instrument the full panic paths anyway, and this is a straightforward analysis to do.

None of this is very relevant to this PR though! I just wanted to put my ideas down somewhere..

[richkadel]

Just a wish that I have:
Instead of making this a function level attr, it should work equally on mod or even just blocks.

Another thing I wish for would be a crate-level attr that would disable coverage for every #[test] fn.

@Swatinem - Can you expand on this? What are the use cases for disabling coverage at the crate level, or in crate tests?

For example, the use case addressed in PR #83601 is clear: The implementation of Eq required adding a special marker function (which gets added in every struct that derives from Eq) that was not intended to be called. Every struct that derived from Eq could never achieve 100% coverage. This unexecutable function clearly justifies the requirement for #[no-coverage].

I personally see the Eq trait issue as an exceptional case.

Generally speaking...

I think we want to keep the bar high for now (by restricting the scope of the attribute to functions, until any broader requirement is vetted by the Rust community).

So if there are other strong cases for excluding coverage instrumentation, please post them here so we can validate the use cases before expanding the scope of the coverage attribute.

[richkadel]

One more comment for clarity: PR #83601 has been merged and adds the feature-gated #[no_coverage] attribute. The attribute can be enabled on an individual function (by also adding #[feature(no_coverage]) or at the crate level.

[richkadel]

I should have added: This tracking issue will remain open until the attribute is stablized. This means there is still an opportunity to change the attribute, based on the tracking issue feedback.

Thanks!

[Swatinem]

@Swatinem - Can you expand on this? What are the use cases for disabling coverage at the crate level, or in crate tests?

This might be a bit personal preference, but is also done that way in other language ecosystems.
You generally want to have coverage for the code under test, not the test code itself. Having coverage metrics for test code provides zero value to me as developer.

[nagisa]

Note: there are previous initiatives to introduce inheritable/scoped/propagated attributes, e.g. for the optimize attribute, but implementing the propagation behaviour there seemed to be pretty involved to me at the time and IMO any attempts to introduce such attribute propagation mechanisms should be a standalone RFC introducing a mechanism in a more general way.

[jhpratt]

Would it be possible to add this attribute to the code generated by unreachable!()? Behind a flag, of course. Both #![feature(no_coverage)] and LLVM-based code coverage are unstable, so I don't think this would cause any issues unless I'm missing something.

[richkadel]

Unfortunately, #[no_coverage] (in its current form) applies to a function definition only. Applying that attributed before a function definition will cause the entire function to be ignored by coverage. But I assume you want the "call" to unreachable!() to be ignored by coverage. Ignoring a statement or block is not currently supported.

I can understand the motivation for this request, and you may want to file a new issue, if it doesn't already exist. There is a potential downside: coverage reports would not be able to show that an unreachable block was unintentionally executed. Maybe that's a minor tradeoff, but the current coverage reports confirm that unreachable blocks are not reached, by showing the unreachable block has a coverage count of zero.

I think adding support for #[no_coverage] blocks may be the best solution, but macro expansion may make this tricky to get right. There may not be a quick solution.

[richkadel]

And I may be wrong about block #[no_coverage] being the "best solution". Thinking about this just a bit more, adding the #[no_coverage] attribute at the function level was not that difficult, because AST-level functions map directly to MIR bodies (and coverage is instrumented within each MIR body). That's easy to turn on or off at the function level. But AST-level blocks don't translate directly to internal MIR structure. That will also make implementing more granular support difficult, in addition to macro expansion complications.

[tmandry]

To me, #[no_coverage] blocks seem like the best solution from a language perspective. Agreed that there might be some work threading things through on the implementation side.

For macros, we can just think about this as applying to a particular expansion (and the attribute applies to the block in HIR, i.e. after macro expansion). Maybe there are other issues I'm not thinking about.

[jhpratt]

I don't think accidentally executing an "unreachable" expression is an issue for coverage, as that's not the job of coverage; tests are where that should be caught.

There is a tracking issue for this already, I wasn't sure if there was an easy-ish way to land it. Apparently that's not the case. I would imagine in the future statement and expression-level masking for coverage purposes will exist, which would necessarily allow for this. Ultimately this would just cause the macro to include the attribute in its expansion.

[clarfonthey]

Is there a good reason why derived traits are included in coverage reports, and not also marked as #[no_coverage]? Considering how the assumption for derived traits is that they're already "tested" by the standard library, I figure that it makes the most sense to simply not include coverage for them by default.

But, I could see an argument for including them by default but allowing a #[no_coverage] attribute on struct definitions to disable them, if you feel like going that route.

[clarfonthey]

In terms of specifically how this attribute should be named, I would be in favour of going with a #[coverage(...)] style attribute as @nagisa recommended. This also opens up adding other coverage-based attributes in the future, like:

• #[coverage(hide)] -- the existing #[no_coverage] attribute
• #[coverage(ignore)] -- would mark code as "coverage doesn't matter," e.g. derives, telling tools to still show coverage for lines but not including it in total coverage metrics
• #[coverage(disallow)] -- would mark code as "should not be covered", e.g. the unreachable!() macro, allowing tools to show warnings when they are covered
• #[coverage(require)] -- would mark code as "should always be covered", allowing tools to show warnings when they are not covered

This obviously wouldn't be required for an initial stable #[coverage] attribute, but I think in particular the distinction between #[coverage(hide)] and #[coverage(ignore)] might be useful to have. Something like the Eq method should be marked #[coverage(hide)] because it's more confusing to show coverage than not, but in general I think it would make sense to mark other stuff as #[coverage(ignore)] in general.

[richkadel]

I'm not convinced ignoring trait functions is a good idea, from a test coverage perspective.

When you derive a trait, your struct's API contract has changed. That struct now supports every function defined in that trait. To guarantee 100% test coverage of the behaviors of every function for your struct, you would have to test it.

Without adding a test, the "ignore" concept is really just a cheat, because you want to achieve 100% test coverage without actually testing it 100%. You can say you are "really confident" that it will work fine, but you can't prove it without some kind of test analysis or observed behavior, via a test.

[clarfonthey]

Yes, the API contract changes when you derive standard traits, but I personally don't believe that testing them adds any value for the developer. For example, assert_eq!(value, value.clone()); would provide coverage for derived PartialEq and Clone implementations, but as far as your library goes, you're not really testing anything important; the only way those would fail is if there's an error in the compiler, which I don't think is everyone's responsibility to test.

I should clarify I'm explicitly talking about the standard derives; individual proc macros can make the decision whether they want to add the attribute to their generated code or not.

[joshtriplett]

I don't think applying this to automatically derived code is a blocker for stabilization, though it seems like a potentially good idea.

Let's go ahead and see if we have consensus to stabilize this other than the potential issue of applying it automatically to nested functions or inlined functions.

@rfcbot merge

@rfcbot concern should-attribute-disable-coverage-for-nested-functions?
@rfcbot concern should-attribute-disable-coverage-for-inlined-functions?

I've also marked this I-lang-nominated, to discuss these two questions.

My proposal would be that coverage(off) should automatically apply to functions (and closures) nested inside the function in question, but that it shouldn't automatically apply to inlined functions.

Rationale: putting #[coverage(off)] on a function should apply to everything inside it for convenience, but things it calls might still want coverage for other reasons. I'd propose that if we want something that recursively disables coverage, that should be an additional attribute.

However, this is a loosely held position, and I'd love to see a good argument / use case for also disabling it on inlined functions.

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

Concerns:

• should-attribute-disable-coverage-for-inlined-functions? (Tracking issue for function attribute #[coverage] #84605 (comment))
• should-attribute-disable-coverage-for-nested-functions? (Tracking issue for function attribute #[coverage] #84605 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[Zalathar]

One other concern about the current implementation that I want to reiterate is that #[coverage(off)] can be attached to various things that cannot actually be instrumented for coverage. In some cases this results in a hard error; in other cases it only results in an unused-attribute warning.

This inconsistency seems undesirable for a newly-stabilized feature; presumably it would be better to always issue an error for everything other than a function/method implementation or closure expression.

[Zalathar]

The current error/warning checks were added by #97495, but I don't see any specific discussion of which scenarios should result in warnings vs errors.

[davidhewitt]

Rationale: putting #[coverage(off)] on a function should apply to everything inside it for convenience, but things it calls might still want coverage for other reasons. I'd propose that if we want something that recursively disables coverage, that should be an additional attribute.

Another possible rationale: as a user I was tempted to assume that a hypothetical #[coverage(off)] on a a module or block would disable coverage of all items inside. If that sounds like a reasonable design assumption, #[coverage(off)] on a function applying to everything inside it would be consistent with that design.

That said, thinking of edge cases like the below, where the nested function is inside a local submodule:

#[coverage(off)]
fn outer() {
    mod foo {
        fn inner() { }
    }
}

... I wonder if disabling coverage for inner in that snippet would require enough machinery in place for #[coverage(off)] on a module to be trivial to implement afterward.

[clarfonthey]

I think that having a coverage(off) on an outer function should require an explicit coverage off/on for child items as a backward-compatibility thing, similar to how using coverage on other items will warn for now. We don't guarantee recursion but would like this in the future.

Shouldn't necessarily block stabilisation though, since changing coverage isn't something that qualifies as a breaking change.

[clarfonthey]

In terms of inlined functions -- do the coverage tooling not get carried over through inlining? I would assume that even if inlined, you'd still be incrementing the same counters for coverage in the original function, just from a different place. If that's not the case, then that feels like both a bug and a reason to warn on any coverage attributes for inlined functions.

But in a hypothetical future where that bug is fixed, I wouldn't expect inlined functions to have special treatment for the attribute itself.

[tmandry]

I agree with @joshtriplett here. While I'm less certain about the nested functions question, I do feel that I want to be able to write #![coverage(off)] in a module and have it apply to everything in that module, so making it apply to nested functions is consistent with that.

@rfcbot reviewed

[Zalathar]

In terms of inlined functions -- do the coverage tooling not get carried over through inlining? I would assume that even if inlined, you'd still be incrementing the same counters for coverage in the original function, just from a different place. If that's not the case, then that feels like both a bug and a reason to warn on any coverage attributes for inlined functions.

Your assumption is correct. If an instrumented function is inlined, the inlined code will increment the same counters as the non-inlined copy of the function.

(The context for this being a point of discussion is that a user was trying to ensure that a particular function contained no instrumentation of any kind, whether owned by that function or inlined from some other function. Currently there's no way to actually achieve that outcome, because that's not what #[coverage(off)] is trying to do.)

[jhpratt]

Major use case for using #[coverage(off)] on a module: mod test. I'm not going to write tests for tests, after all.

[clarfonthey]

So, to summarise the proposal:

Right now, coverage attribute is ignored on items with a warning that we intend to do so recursively, which is fine. The main confusion is that items can be nested inside functions, which don't emit this warning. We should probably add additional warnings if the attribute is used on functions with nested items, or functions containing closures. (I believe these always have coverage regardless of the parent.)

I think that warnings are enough to mostly just let people know about compatibility warnings. I don't think that we should guarantee stability of coverage even after this is merged, since we'd want to wait until coverage code is reliable enough and complete enough. This means that we're allowed to break CI, etc. if it has hard coverage requirements and don't guarantee forward compatibility at all, at least for now.

[joshlf]

+1 to coverage(off) applying at the module level. I came here because I tried applying it to mod tests modules in google/zerocopy#1301, and got the relevant warning. It's a significant usability issue: for example, we recently added this test which doesn't execute some code by design, and as a result, Codecov generated spurious warnings about untested lines.

[briansmith]

The Eq trait in the std library implements a marker function that is not meant to be executed, but results in an uncovered region in all rust programs that derive Eq. This attribute will allow the compiler to skip that function, and remove the uncovered regions.

A similar issue occurs when we have code that is only supposed to be executed in const contexts. Not only do we not want to be bothered about the code never executing at runtime, but in fact we do want to be notified if it does execute at runtime. For example, codecov.io should warn us when code marked as coverage(no) is actually covered.

I'll also repeat my comment on excluding tests using coverage(no) that I left in the zeroconf issue:

We still want code coverage measurement for tests so we can be notified about any branches in test code that aren't executed (i.e. bugs in tests). Code coverage reporting tools like codecov.io have features like components and flags that could be used to separate test and non-test code with various levels of inconvenience.