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.

[davidhewitt]

Unfortunately, #[no_coverage] (in its current form) applies to a function definition only.

I think I just ran into this when looking at coverage in PyO3. I tried to put #![no_coverage] attribute on a test module.

When you speak of #[no_coverage] blocks, does that include using the attribute as a module inner attribute like I tried here?

[tmandry]

@davidhewitt Applying #[no_coverage] (or its equivalent) to an entire module might indeed be useful. I'm curious to know more about your use case, though. Are you measuring coverage using tests? What code in the test module do you not want to see in coverage reports?

[davidhewitt]

@tmandry the use case is in PyO3, in particular this module and its children such as this.

We measure coverage using our test suite with the help of cargo-llvm-cov.

PyO3 has a lot a of proc macros to generate Python bindings for Rust code. To make them hygienic, the generated code uses PyO3 as a global symbol ::pyo3. This can be overriden by using #[pyo3(crate = "some_path")], which changes the generated code to use PyO3 symbols underneath some_path instead.

To verify this, we have a compile-time test inside the lib itself, where ::pyo3 is not a symbol and we have to apply #[pyo3(crate = "crate")]. Any generated code which is not hygienic fails to compile. However, none of this code needs to be run; we have a full set of other tests which verify behaviour is as expected.

Thus the desire to add #![no_coverage] to the whole of mod test_hygiene, so that this entire module doesn't contribute to coverage stats.

[bossmc]

Something else to consider would be adding an (optional) reason or justification parameter to the attribute - one pattern of use for coverage is to cover 100% of the lines that are "sensibly coverable" and it would be nice (and in some corporate situations, required) to explain the lines that cannot be covered (e.g. "this code is platform specific" or "this is unreachable code because of X").

I'm picturing that the parameter is optional, but with a lint for missing parameters which can be set to deny if desired. Bonus marks if there's some way for the justification status to make it into generated reports ("97% covered, 2% justified uncovered, 1% unexpectedly uncovered" or similar).

[PoignardAzur]

Now that -C instrument-coverage is stable, is there anything stopping us from stabilizing this?

Even a basic version that only applies to functions would be very useful. Code coverage would start to feel like something integrated in the code, and not just a pile of hacks.

[clarfonthey]

AFAIK, the only breaking change that might need to be made is naming the attribute #[coverage(never)] or something similar as @nagisa suggested. I'm kind of in favour, but wouldn't be distraught if this weren't done.

The feature is far from done, but we could stabilise the attribute before everything is there since we don't really make guarantees about coverage, and the syntax is really the main thing.

[PoignardAzur]

Yeah, I'd be in favor of merging an MVP as soon as possible.

Breaking changes can always be done later in an edition change.

[clarfonthey]

Editions don't exist to rush basic design decisions. Please be patient; the feature already works on nightly and can be used there in the meantime.

[PoignardAzur]

That's fair, but in the case of coverage(never) vs no_coverage, it's less of a basic design decision and more of a bikeshedding one.

I didn't mean "breaking changes" in the sense of fundamentally changing what the attribute did.

[bstrie]

I like the idea of coverage(foo) for the various options, as I think it will make discovery and documentation easier.

[bstrie]

I also agree that being able to annotate at the module level would be useful, but perhaps that doesn't need to block stabilization of the current functionality.

[clarfonthey]

So, seeing how I'm one of the people who's been suggesting working on this before stabilisation, I figured I should actually do something about that and try to provide the changes I've suggested.

So, specifically:

1. Add an error similar to that for #[inline] being applied to non-functions, where stuff like structs and consts can't be marked as #[no_coverage]
2. Add extra notes in warnings on recursive #[no_coverage] not working
3. Potentially rename to #[coverage(never)] (will probably be a separate change since this is spicier)

[clarfonthey]

So, I thought about it a bit, and I've gone back on whether the #[no_coverage] attribute should be renamed, and I think it should keep its existing name. If there were other options for a potential #[coverage(...)] attribute, they would be processed by completely different code and/or tools, and it wouldn't make sense to group them under the same name. Additionally, there's the precedent of #[no_link] and #[link = "..."] attributes coexisting, which helps.

Such an attribute would primarily be used for marking whether coverage for certain code is always included or excluded when doing automated coverage checks, which would be included in tools that analyse coverage data, rather than generate coverage instrumentation. The only potential I could ever see for another coverage instrumentation attribute would be to only generate coverage instrumentation for specific code rather than all code, which I don't think is a use case that will ever be available, but is worth mentioning for completeness.

So, I think that with #97495 merged, this should be ready for FCP, since functionality to add #[no_coverage] at higher granularity than a function (e.g. directly to match branches) or lower granularity (entire modules) can be done after stabilisation. Right now, this emits unused_attributes lints instead of an error, which will allow crates to use these at that point without bumping MSRV. (technically not required for backwards compatibility, but nice anyway)

[tmandry]

The use case I'm imagining for another attribute is one that overrides #[no_coverage]. So you'd have something like

• #[coverage(off)] – Current meaning of #[no_coverage]; can be applied to modules, functions, and blocks
• #[coverage(on)] – Reverses #[coverage(off)] within some scope
• #[coverage(force_on)] – Overrides all #[coverage(off)] within a scope, including in macro expansions

All of these would be handled within the compiler frontend. @clarfonthey I believe this addresses your concerns about the syntax, do you agree?

The latter might be useful for certification-like scenarios. It's unclear to me if #[coverage(on)] will ever be useful or not.

I have a slight preference for #[coverage(off)] and it seems like most other people like that syntax too. With that said, I would be okay with stabilizing #[no_coverage] (and perhaps considering #[force_coverage] later on) if there's difficulty getting consensus around #[coverage(off)].

[clarfonthey]

Honestly? I feel like that kind of control being done by one macro isn't necessary. Consider the main mechanism that'd be compared to, linting; we have allow, warn, and deny as three separate attributes. Wouldn't that be covered by just having an emit_coverage attribute in addition to no_coverage?

[tmandry]

I would liken it to #[inline], #[inline(always)], and #[inline(never)]. The pattern in each case is that you have fewer options outside the parens than inside.

That said, I'm fine with #[no_coverage] and #[emit_coverage]. The most important thing is shipping something that is easy to use and ideally easy remember.

#[coverage(...)] requires us to rationalize whatever words we choose inside the (...) which is actually a significant downside. While these words seem like they might be generally useful for other things, they don't have precedent as attribute arguments as far as I'm aware. Meanwhile we have no_std, no_core, no_link.

[clarfonthey]

Note that inline also doesn't have any kind of recursive behaviour, unlike the kind of enable/disable nesting you're describing.

Like I said, I feel like this has waited long enough and we should be able to merge what we have as-is.

[tmandry]

@rustbot label +I-lang-nominated

Lang team, can we move forward on an FCP? Does this need only a stabilization report, or do we need a full RFC?

The coverage feature itself was implemented via the compiler team MCP process, but this is adding a new attribute to the language, so I'm not 100% clear on the process.

[jimblandy]

Here's a use case for module-level #[coverage(off)]:

The perf-event-open-sys crate includes a few hand-written functions for a system call that is not covered by the libc crate (nor by glibc, for that matter), and a few ioctls associated with that system call. These, it is reasonable to want coverage for.

However, it also has a file bindings.rs that is generated by bindgen from Linux kernel C header files. The generated code includes bitfield accessors and other random stuff:

impl Default for perf_event_attr__bindgen_ty_1 {
    fn default() -> Self {
        let mut s = ::std::mem::MaybeUninit::<Self>::uninit();
        unsafe {
            ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1);
            s.assume_init()
        }
    }
}

Technically, I suppose thorough coverage would check storing and reading from every struct field that required special treatment from bindgen, but, honestly, I just don't care at all.

So I'd like to include lib.rs in coverage, but not bindings.rs.

[Zalathar]

On the reporting side, cargo-llvm-cov supports ignoring entire files or directories via a path regex. So that might be a useful workaround for wanting to ignore a module.

(The ignored code still gets instrumented; it just doesn’t show up in reports or summary statistics.)

Ignoring smaller regions of code is trickier, since neither the compiler nor the reporter have any support for that. At present I can’t think of a workaround short of writing one’s own custom reporting program that consumes LLVM’s json reports.

[clarfonthey]

For what it's worth, I do think that recursively disabling coverage and disabling coverage for specific branches would be useful as a future extension, but would require considerable work in the compiler for the former. I don't think either should block stabilisation, though.

In the PR I made adding an error to improper uses of #[no_coverage], those kinds of usages only count as unused attributes for now, instead of the hard error you'd get on something like marking a type alors as #[no_coverage].

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting.

We do think it makes sense to make this coverage(off) and leave room for other coverage arguments in the future.

Apart from that, if someone wants to write a stabilization report, we'd be happy to stabilize this.

[davidhewitt]

Possibly the ship has already sailed with stabilization of -C instrument-coverage; a quick thought struck me this morning...

If we'll have #[coverage(off)], does it make sense to:

• add #[coverage(on)] at the same time
• change #[test] to imply #[coverage(off)]

In my opinion coverage of test code just dilutes results of the actual library code you want to measure, and if we had #[coverage(on)] users could measure tests where required.

I wonder if "ship has sailed" because this would be change of behaviour for existing test coverage measurements.

EDIT: I'm aware the converse is true and could just add #[coverage(off)] to all tests, just wondering what a more sensible default is.

[wesleywiser]

I wonder if "ship has sailed" because this would be change of behaviour for existing test coverage measurements.

The stability of -Cinstrument-coverage is scoped so that the exact process of instrumentation and result generation are not specified. As such, I think it's reasonable for us to evolve the feature in ways that makes it generally more useful, even if it does change the coverage data in some way.

[clarfonthey]

Notes for implementing the recommended changes, since it's actually pretty approachable for anyone who hasn't touched the compiler before. Besides the trivial changes to from no_coverage to coverage + the extra tests, there's actually not much to do.

To make sure only #[coverage(off)] disables coverage, you'll need to modify rustc_typeck/src/collect.rs to check the attribute parameter before emitting the NO_COVERAGE flag. Relevant lines from the initial PR are here.

To actually validate the attribute, you'll need to modify rustc_passes/src/check_attr.rs. Relevant lines from my PR are here.

Essentially, you'll want to retain the code that checks whether #[coverage] can even be used in a given context, plus:

1. If #[coverage(on)] is used, emit an unused_attribute lint with some explanation on how this has no effect right now. This way, people are aware that it isn't implemented, but it won't break anything in the future when we do implement it. (Technically, this isn't required, but it will help people avoid bumping their minimum rust version if they want to use the feature in the future.)
2. If the argument to #[coverage] isn't on or off, emit E0788 (the error code created in my PR) with some message stating it needs #[coverage(on)] or #[coverage(off)]. It's up to you whether you want to emit an error or unused_attribute if you pass #[coverage] with no argument; I personally think it should be an error.

---

Small note: this is basically the minimum required to implement the feature in a reasonable state. Technically, we could promote the NO_COVERAGE flag to a general COVERAGE flag on functions, then make coverage(on) actually enable coverage even if -Cinstrument-coverage isn't passed. But I'm not sure this is required for stabilisation, and it would be more work to do.

[richkadel]

I wonder if anyone has used #[no_coverage], outside of std. Should it be left active but marked deprecated for a little while, to give early adopters an opportunity to migrate to #[coverage(off)] before it's a breaking change? (Thinking about @davidhewitt 's comment, and I wondered if he might have implied that people are using it today, to disable coverage of test code?)

[clarfonthey]

You're right that we should probably add in some extra code to allow existing no_coverage attributes as an alias, although I'm not sure what we'd want to do in terms of deprecating that. I would assume basically everyone invested in this thread has used no_coverage outside libstd in some manner.

[messense]

I wonder if anyone has used #[no_coverage], outside of std.

I know it's used in pydantic-core: https://github.com/samuelcolvin/pydantic-core/search?q=no_coverage

for example https://github.com/samuelcolvin/pydantic-core/blob/3082c5be70c15c948e9d58783d63693192552fc1/src/input/parse_json.rs#L65

[davidhewitt]

Also https://crates.io/crates/coverage-helper/reverse_dependencies

[Maximkaaa]

The current implementation of no_coverage has a few unexpected peculiarities:

1. Since the attribute is only applied to the function it is set for, any function declared inside the marked function body will still be counted in the code coverage report. This is fine, I guess, for named functions, but is a little surprising for closures. For example:

   #[no_coverage]
   fn excluded() {
   let _ = vec![].iter().map(|x| x + 1);
   }

   The line starting with let will be counted in the report, since x + 1 is a different function not marked with the no_coverage attribute.

2. The above also leads to a second issue. If I use a macro that wraps my code with a closure, my code will be counted in the coverage report even if it's marked with no_coverage. Take a proptest crate as an example. This code:

proptest::prop_compose! {
    #[no_coverage]
    fn my_function(arg: u32) (index in any::<Index>()) -> u32 {
        todo!()
    }
}

Will be counted as 5 lines of code in the report. The macro is expanded into:

#[must_use = "strategies do nothing unless used"]
#[no_coverage]
fn exclueded_inside_proptest(arg: u32) -> impl ::proptest::strategy::Strategy<Value = u32> {
    let strat = any::<Index>();
    ::proptest::strategy::Strategy::prop_map(strat, move |index| {
        ::core::panicking::panic("not yet implemented")
    })
}

Interesting fact is that even though the inner closure in expanded code is only 3 lines long, all 5 lines of the original code are counted.

[zbraniecki]

My use case for ability to exclude derived traits from coverage statistics is a public library that implements traits intended to be usable by its customers. For example:

#[derive(Debug, PartialEq, Eq, Clone, Hash, PartialOrd, Ord, Copy)]
#[non_exhaustive]
pub enum ExtensionType {
    /// Transform Extension Type marked as `t`.
    Transform,
    /// Unicode Extension Type marked as `u`.
    Unicode,
    /// Private Extension Type marked as `x`.
    Private,
    /// All other extension types.
    Other(u8),
}

Our library implements thousands of them. We will not add manual tests to verify that Debug works on each of them, that would make no sense. With the current situation the coverage results are noisy because we have to manually ignore all the missing coverage that comes from derived traits.

[detly]

Just to add a use case for more fine-grained application of this ie. blocks rather than functions: non-exhaustive enums. They require a catch-all case in a match, but no matter what strategy you use (ignore and continue, panic, log, separate handling), it is impossible to contruct a test for such a case to make sure your chosen method of handling it actually does what you expect. So it seems a bit unfair you can't do something like:

use syn::ImplItem;

fn check(item: ImplItem) {
    match item {
        ImplItem::Const(_) => (),
        ImplItem::Method(_) => (),
        ImplItem::Type(_) => (),
        ImplItem::Macro(_) => (),
        ImplItem::Verbatim(_) => (),            

        #[no_coverage]
        _ => {
            log::warn!("Unknown item: {item:?}");
        }
    }
}

...given that there's no way, even in a test build, to construct something to test that path.