Tracking issue for function attribute `#[coverage]`

Question

Tracking issue for function attribute `#[coverage]`

richkadel opened this issue 5 years ago · 124 comments

Warning

This main #[coverage(..)] attribute tracking issue description is quite outdated, and likely needs to be revisited.
Please refer to #134749 for the stabilization issue for the #[coverage(..)] attribute.

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.

Unresolved questions

is it ok that the attribute applied to the crate root, a function, or a module applies to all items within the marked item? So far we do not have such attributes other than the lint level attributes.

Path to stabilization

Please refer to the stabilization issue: #134749.

Answer 1 · 2021-04-27T01:35:26.000Z

cc: @tmandry @wesleywiser

Answer 2 · 2021-04-27T12:27:34.000Z

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

Answer 3 · 2021-04-27T12:37:40.000Z

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.

Answer 4 · 2021-04-28T20:09:57.000Z

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..

Answer 5 · 2021-04-28T20:27:49.000Z

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.

Answer 6 · 2021-04-28T20:31:10.000Z

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.

Answer 7 · 2021-04-28T20:34:31.000Z

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!

Answer 8 · 2021-04-29T10:28:59.000Z

@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.

Answer 9 · 2021-04-29T11:18:35.000Z

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.

Answer 10 · 2021-09-08T09:27:47.000Z

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.

Answer 11 · 2021-09-08T16:40:30.000Z

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.

Answer 12 · 2021-09-08T16:49:13.000Z

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.

Answer 13 · 2021-09-08T22:02:09.000Z

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.

Answer 14 · 2021-09-08T23:03:57.000Z

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.

Answer 15 · 2021-12-02T04:41:04.000Z

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.

Answer 16 · 2021-12-02T05:19:34.000Z

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.

Answer 17 · 2021-12-02T15:20:04.000Z

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.

Answer 18 · 2021-12-02T21:14:33.000Z

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.

Answer 19 · 2021-12-20T21:10:19.000Z

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?

Answer 20 · 2021-12-21T02:05:43.000Z

@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?

Answer 21 · 2021-12-21T04:51:21.000Z

@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.

Answer 22 · 2022-01-26T17:02:21.000Z

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).

Answer 23 · 2022-04-11T13:58:27.000Z

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.

Answer 24 · 2022-04-11T14:03:56.000Z

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.

Answer 25 · 2022-04-11T14:11:08.000Z

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.

Answer 26 · 2022-04-11T14:36:27.000Z

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.

Answer 27 · 2022-04-11T14:54:22.000Z

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.

Answer 28 · 2022-05-19T13:49:51.000Z

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

Answer 29 · 2022-05-19T14:14:39.000Z

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.

Answer 30 · 2022-05-28T20:40:31.000Z

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:

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]
Add extra notes in warnings on recursive #[no_coverage] not working
Potentially rename to #[coverage(never)] (will probably be a separate change since this is spicier)

Answer 31 · 2022-06-07T23:27:49.000Z

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)

Answer 32 · 2022-06-24T23:32:57.000Z

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)].

Answer 33 · 2022-06-25T00:19:55.000Z

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?

Answer 34 · 2022-06-25T00:52:28.000Z

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.

Answer 35 · 2022-06-25T01:14:20.000Z

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.

Answer 36 · 2022-06-29T01:00:23.000Z

@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.

Answer 37 · 2022-07-03T07:56:32.000Z

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.

Answer 38 · 2022-07-04T23:13:25.000Z

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.

Answer 39 · 2022-07-05T07:42:15.000Z

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].

Answer 40 · 2022-07-05T17:32:52.000Z

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.

Answer 41 · 2022-07-06T07:13:34.000Z

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.

Answer 42 · 2022-07-06T21:49:21.000Z

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.

Answer 43 · 2022-07-06T22:38:17.000Z

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:

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.)
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.

Answer 44 · 2022-07-07T19:14:10.000Z

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?)

Answer 45 · 2022-07-07T20:07:37.000Z

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.

Answer 46 · 2022-07-18T07:23:57.000Z

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

Answer 47 · 2022-07-18T07:39:53.000Z

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

Answer 48 · 2022-08-12T13:07:24.000Z

The current implementation of no_coverage has a few unexpected peculiarities:

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.

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.

Answer 49 · 2022-10-02T17:41:49.000Z

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.

Answer 50 · 2023-03-05T10:40:22.000Z

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.

Answer 51 · 2023-03-06T13:52:31.000Z

Probably related to @Maximkaaa 's issue above, I am seeing various lines within an async fn appearing in the report, despite the fn being marked #[no_coverage].

Answer 52 · 2023-03-31T19:24:49.000Z

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

@taiki-e recommended using it as a solution for ignoring #[test]-related code in cargo-llvm-cov: taiki-e/cargo-llvm-cov#123 (comment). The crate published for it has 5 dependents and a non-zero amount of daily downloads, and there are likely others who used the attribute as suggested without importing the crate.

Answer 53 · 2023-08-05T17:20:29.000Z

@tmandry I have another use case for disabling coverage on entire modules. Mockall is a proc macro that will generate a whole module of code for every mocked function and object. Most of it will never be used, and for most crates none of it will ever be used in production. If Mockall itself could emit #[no_coverage] attribute, that would significantly clean up "Unexecuted instantiation" warnings in coverage reports. As-is, Mockall must emit that attribute in 87 places (and even so, it doesn't work in all of them). If the attribute worked on modules, then Mockall would only need to use it in 2-4 places.

Answer 54 · 2023-08-05T18:14:29.000Z

I don't think this thread needs more arguments for why a more-applicable #[no_coverage] attribute would be desired, since I feel like everyone seems adequately convinced that, in the long term, it should be available and work on just about everything, minus maybe type definitions.

The issue is that right now, it doesn't, and I don't think that this is a good use of this issue since it doesn't further the larger issue of the feature not being stable at all.

Right now, I believe that the main blocker is converting the existing no_coverage attribute into a coverage(off) attribute, since people expressed the desire for other options in the future. If someone wants to put the effort into that, we can at least stabilise the attribute and then can later add in support for more scopes.

Answer 55 · 2023-09-15T17:26:52.000Z

The change from #[no_coverage] to #[coverage(off)] is complete (with a hat tip to @Zalathar for their help) in #114656. This clears the "main blocker" to stabilization as per @clarfonthey's last post. What needs to happen next to stabilize this attribute?

Answer 56 · 2023-09-15T17:57:36.000Z

As far as I'm aware, we should be ready for an FCP. Just need someone to start the process.

Answer 57 · 2023-09-19T07:37:48.000Z

Should the issue title be updated to #[coverage(off)]? :)

Answer 58 · 2024-01-19T12:24:28.000Z

I noticed that using #[coverage(off)] on a function that contains inlined functions that are being profiled, will include the profiling of these inlined functions.

https://rust.godbolt.org/z/W93cj7nP7 shows this issue; applying #[coverage(off)] on fun1() (the inlined function), fixes the issue.

Is this considered a bug? If it is I couldn't find a corresponding issue.

Answer 59 · 2024-01-19T13:15:46.000Z

Is this considered a bug? If it is I couldn't find a corresponding issue.

That's currently the intended behaviour. If fun1() is instrumented for coverage, then that instrumentation includes call sites where fun1() has been inlined, even if the surrounding function is not instrumented.

(But if there's some reason why you'd prefer different behaviour, that's valid feedback too.)

Answer 60 · 2024-01-19T13:21:36.000Z

While I'm here, I also have a few concerns I want to note before any potential stabilization of this attribute:

Currently it only affects the function that it's attached to, and has no effect on other functions/closures/coroutines nested within that function. I think it would be better for the attribute to also affect nested functions (which can override that behaviour with their own attribute), and it may be awkward to change to that behaviour after stabilization.
From what I've tried, attaching this attribute to a closure seems to be rather tricky.
- EDIT: It looks like attaching codegen attributes to a closure requires #15701, which is unstable.
I've seen cases where adding this attribute in certain places appears to have no effect, but is not rejected by the compiler.

Answer 61 · 2024-01-19T13:37:30.000Z

(But if there's some reason why you'd prefer different behaviour, that's valid feedback too.)

We've encountered this problem in wasm-bindgen (rustwasm/wasm-bindgen#3782) while implementing test coverage support.

The problem is two-fold:

The built-in minimal-interpreter isn't able to handle the generated profiling code.
We don't actually want to run any profiling code in this interpreter. The code in question here will be removed by the post-processor anyway.

The only wider use-case I see here is maybe somebody wants to be able to run a function without triggering any profiling in any other called function. The use-case in question here is too niche to justify any work in Rust imo.

In the meantime we solved it by just slapping a lot of #[coverage(off)] everywhere.

Answer 62 · 2024-01-19T15:29:27.000Z

I think the problem we are having in wasm-bindgen might have a different solution. Most of the code in question here is auto-derived. I think anything marked autoderive should probably be #[coverage(off)].

Answer 63 · 2024-01-20T11:19:22.000Z

I think the problem we are having in wasm-bindgen might have a different solution. Most of the code in question here is auto-derived. I think anything marked autoderive should probably be #[coverage(off)].

We have some code which required #[coverage(off)] that is not generated by proc-macros.
I have originally assumed that profiling is only applied on functions of the root crate, but that doesn't seem to be the case or something else I don't understand is playing a factor here.

In any case, it seems sensible to me too that #[automatically_derived] should be #[coverage(off)] as well.

Answer 64 · 2024-01-21T04:53:53.000Z

I've filed #120185 to ignore functions within an #[automatically_derived] impl, because I agree that doing so is preferable in most cases.

Answer 65 · 2024-02-11T00:43:26.000Z

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.

Answer 66 · 2024-02-11T00:43:28.000Z

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

Concerns:

~~should-attribute-disable-coverage-for-inlined-functions?~~ resolved by #84605 (comment)
~~should-attribute-disable-coverage-for-nested-functions?~~ resolved by #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.

Answer 67 · 2024-02-11T01:30:47.000Z

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.

Answer 68 · 2024-02-11T01:35:37.000Z

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.

Answer 69 · 2024-02-11T04:17:46.000Z

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.

Answer 70 · 2024-02-11T11:34:58.000Z

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.

Answer 71 · 2024-02-11T11:37:26.000Z

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.

Answer 72 · 2024-02-16T01:25:40.000Z

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

Answer 73 · 2024-02-16T02:18:48.000Z

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.)

Answer 74 · 2024-02-29T07:21:35.000Z

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

Answer 75 · 2024-03-01T19:29:08.000Z

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.

Answer 76 · 2024-05-18T19:41:08.000Z

+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.

Answer 77 · 2024-05-19T03:02:43.000Z

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.

Answer 78 · 2024-05-31T19:44:38.000Z

@joshtriplett what do you think should be done to resolve your concern, and do you think that my comment clarifying future steps is sufficient? #84605 (comment)

Right now I'm kind of tired of having to write #[cfg_attr(feature = "nightly", coverage(off))] and instead would prefer to be able to just use the attribute. As mentioned, there is lots of room for improvement on the existing implementation, but I don't think that there should be any stability guarantees on the attribute until coverage is mature anyway, and I think that it would be very beneficial to stabilise the attribute as-is and fix these issues after the fact.

One "low-hanging-fruit" for the current implementation would be to at least apply coverage recursively for nested functions (right now, for example, a closure included in a coverage(off) function will still have coverage), but again, I think that this is very fair to do post-stabilisation. We currently are fine with the actual attribute, and are mostly just iffy on the semantics, which will not be stable for a long time anyway.

Answer 79 · 2024-06-04T14:03:29.000Z

@rfcbot reviewed

I want to stabilize but I feel uncertain what we are stabilizing exactly. I don't really see a stabilization report summarizing the design we landed on, steps we took along the way, or rationale. Did I miss it?

@rfcbot concern no stabilization report

Answer 80 · 2024-06-13T18:29:54.000Z

Old version for posterity

What's being stabilised

The #[coverage(...)] attribute can now be as #[coverage(off)] and #[coverage(on)] to disable or enable code coverage instrumentation, respectively. All other arguments (including an empty #[coverage]) will result in an error.

Guarantees of coverage

At present, since code coverage instrumentation is very far from complete, no guarantee is made about the validity or inclusion of code coverage; these are merely hints to the compiler. In particular, code that relies on a certain amount of coverage should expect to be broken between Rust versions until proper guarantees are drafted, and this means that specifically what code is disabled or enabled by #[coverage(off)] and #[coverage(on)] may change.

While the compiler will make an attempt to emit compatibility lints on certain usages of the coverage attribute, these will not be deny-by-default due to the fact that coverage instrumentation is not guaranteed.

Valid locations

Right now, the coverage attribute will have the following behaviour based on its location:

Function definitions, closures, and method implementations are accepted with no error with one exception: extern fn and trait methods without a body emit an error, since there is no code to cover.
Modules, impl blocks, and trait definitions are accepted but emit a warning: these currently do nothing, but will likely be applied recursively in the future. (e.g., disabling coverage for an entire module)
Expressions, statements, and match arms are accepted but emit a warning: these currently do nothing, but could provide finer control of coverage instrumentation in the future. (e.g., disabling coverage for known-unreachable code)
All other locations result in an error. (e.g., marking a struct as #[coverage(off)])

Note that there is currently one location which could provide a future-compatibility lint but currently does not: functions nested inside functions. The most common case of this is closures: since the coverage attribute is not done recursively, closures and other functions nested inside other functions must be explicitly marked with the same coverage attribute as their parent, since they will otherwise always be covered. Users may be surprised that their closure inside an otherwise coverage-disabled function shows up on instrumentation, even if inlined.

Depending on which is easier, functions which are marked with a coverage attribute and contain other functions should either:

Emit a future-compatibility lint clarifying that the attribute is not recursive, and suggest to apply a coverage attribute to the nested functions which matches the user's expected behaviour.
Recursively apply coverage to functions specifically, ensuring that functions inside functions inherit their parent's coverage modifier.

This is not seen as a blocker to stabilising the attribute itself due to the existing guarantees on stability. The goal is to let users begin to add coverage(off) and coverage(on) to their code without having to condition this usage on a nightly-only feature, noting that the actual results of these attributes may vary between versions.

Future plans

As mentioned, ideally, this attribute will be applied recursively: users should be able to, for example, apply #[coverage(off)] to an entire module and apply #[coverage(on)] to specific functions in that module, rather than having to mark every function they want to disable as #[coverage(off)].

There also may be a desire for some form of #[coverage(ignore)] to be applied to expected-unreachable blocks, which could be used by additional tooling. Here, coverage instrumentation would be applied, but tools which count coverage would instead expect the coverage to not occur, rather than occur. This would help give an indicator for potential errors occurring in large projects which might be missed.

Extra motivation

Right now, usage of the coverage attribute is limited to projects that are nightly-only or who test on nightly, since it must always be gated behind an extra feature. As such, motivation to actually implement the full, recursive version of this feature are limited, since it doesn't have much use, and doing so would require a decent amount of work.

If we instead allow people to start to declare what they'd like for coverage under the assumption that this might not be precise for now, we can start to see more people use this feature and start to have a clearer picture of what stable coverage looks like. There is already a lot of work going into defining how exactly branch instrumentation should work, and it would be nice for people to be able to annotate these branches with #[coverage(off)] and still compile on stable, while being able to test the new instrumentation on nightly.

Overall, just having the attribute syntax stable despite is semantics being unstable feels like a good choice. But what do I know? I'm just writing the stabilisation report.

Caveat

This newer version of the stabilisation report is based upon the pending implementations in #126682 and #126721, which remove the ability to use the attribute in places where it's not implemented and apply it recursively to items, respectively.

What's being stabilised

The #[coverage(...)] attribute can now be as #[coverage(off)] and #[coverage(on)] to disable or enable code coverage instrumentation, respectively. All other arguments (including an empty #[coverage]) will result in an error.

Guarantees of coverage

At present, since code coverage instrumentation is very far from complete, no guarantee is made about the validity or inclusion of code coverage; these are merely hints to the compiler. In particular, code that relies on a certain amount of coverage should expect to be broken between Rust versions until proper guarantees are drafted, and this means that specifically what code is disabled or enabled by #[coverage(off)] and #[coverage(on)] may change.

However, that said, we can guarantee that functions that are marked #[coverage(off)], either directly or recursively, will not be included in coverage instrumentation, even if inlined.

Unfortunately, there really aren't many guarantees we can make about coverage at the moment: functions may not necessarily be included in the final executable (and thus not have coverage instrumentation there), and even if they are included, the compiler may decide to still not output coverage instrumentation. This will definitely improve over time, but it's the current state of things.

Valid locations

Right now, the coverage attribute will have the following behaviour based on its location:

extern functions and trait methods without a body emit an error, since there is no code to cover.
All other function definitions, closures, and method implementations are accepted and apply recursively to themselves and items inside them.
Modules, impl blocks, and trait definitions are accepted and apply recursively to the items contained within them, not having any effect if there are no functions to which they can apply. (Note: traits with this attribute apply only to their default method bodies, and does not affect implementations at all. For traits whose functions should "probably not be covered," consider marking implementations under a derive macro as #[coverage(off)] instead. This is what libstd does for many of its derivable traits.)
All other locations result in an error. More locations may be added in the future (e.g. marking individual branches as #[coverage(off)]) but for now, these return an error.

Caveats

This is actually the first non-diagnostic attribute that would be applied recursively, which is worth pointing out. It's kind of surprising that this hasn't happened yet, but, no better time than the present to set a precedent.

Future plans

As mentioned, there should eventually be the option to mark individual blocks, expressions, match arms, and other kinds of branches with the #[coverage] attribute to have more granular control.

There also may be a desire for some form of #[coverage(ignore)] to be applied to expected-unreachable blocks, which could be used by additional tooling. Here, coverage instrumentation would be applied, but tools which count coverage would instead expect the coverage to not occur, rather than occur. This would help give an indicator for potential errors occurring in large projects which might be missed.

Finally, it would make sense to potentially check whether a #[coverage] attribute applied to a recursive element like a module or impl block actually applies to any functions contained within the block and emit unused_attribute if this happens. In particular, an attribute applied to an item which either contains no functions, or which has all functions explicitly labelled with a coverage attribute, should be marked as unused. A low-hanging-fruit option here might be traits without any default methods, since this feels like a potentially common case.

Answer 81 · 2024-06-17T15:39:32.000Z

OK, so the concern is basically that we would be stabilizing a feature with what appear to be known bugs (no recursive handling). In principle people could rely on the feature having its current behavior and be surprised when it changes in the future?

I was going to suggest that we just stabilize it in the one position where it seems to work reliably, on functions, but I guess even there, it only works "properly" on functions with no closures or inner functions?

Are there open issues for the behavior we want in the future?

Is there a reason it is hard to implement the desired behavior? It (naively) seems fairly straightforward.

Answer 82 · 2024-06-17T17:22:03.000Z

It's not necessarily hard, it's just that no one knowledgeable enough of the code involved has actually done it. If I were to do it, for example, I'd have to learn quite a bit about the parser, HIR, and current coverage instrumentation to even get started, and that's completely ignoring the complexity of the actual implementation.

Answer 83 · 2024-06-17T22:44:31.000Z

I looked into implementing it a few months back, but didn’t get very far because I’m not familiar with the pre-MIR parts of the compiler.

The one obstacle I anticipate implementation-wise is that I don’t think there’s much precedent for recursively-applied function attributes, so I’m not sure how much extra plumbing is needed for that.

On the instrumentation side, the current code just checks a per-function bool to see whether it should add instrumentation or not, so I don’t think there will be much trouble on that side of things.

Answer 84 · 2024-06-17T22:48:49.000Z

Function definitions, closures, and method implementations are accepted with no error with one exception: extern fn and trait methods without a body emit an error, since there is no code to cover.

IIRC, adding an attribute to a closure requires the ability to add attributes to expressions, which is currently unstable?

If that’s still the case, we would be unable to have stable coverage attributes on closures.

Answer 85 · 2024-06-17T23:06:47.000Z

Yeah, I should clarify that even if the attribute is "stabilised" on closures it wouldn't be usable without extra feature flags. It's just that you wouldn't need the coverage attribute feature flag.

Which is, in a way, stabilising it. Just not really in the way folks expect.

Answer 86 · 2024-06-19T01:47:45.000Z

I've had another go at trying to implement recursive #[coverage(..)], and I managed to get a proof-of-concept working.

But in my attempts to polish that code into something more reasonable, I keep running into weird inconsistencies in how the compiler currently validates coverage attributes.

Things that I would expect to be an error instead result in just a warning, or in some cases are silently ignored with no diagnostic at all.
The error messages are inconsistent, and some suggest using the syntax #[coverage], which is usually illegal but in some cases turns out to be unexpectedly permitted.

I've documented some of these in the tests at #126621, but since then I've also encountered some more weird edge cases.

Answer 87 · 2024-06-19T02:53:33.000Z

I've written some tests and filed some issues that highlight concerns with the existing implementation of the coverage attribute:

In cases where the error messages are confusing or redundant, that's not necessarily a blocker (though we should prefer to fix them). But there are also some cases where things are incorrectly allowed.

Based on my recent work, I think that supporting recursive coverage attributes is going to be easier than adequately warning about the fact that they currently don't work recursively.

And there's also the question of whether putting the coverage attribute in places that aren't supported should result in a warning (that it will have no effect) or a hard error. I'm in favour of consistently producing an error, but it seems like the existing warning behaviour was deliberate, so this probably needs some kind of decision to be made.

Answer 88 · 2024-06-19T11:19:32.000Z

I've filed #126682 to clean up validation of the coverage attribute so that it no longer misses any known problem cases, and gives more reasonable error messages.

That cleanup will also make it easier to implement recursive behaviour for the coverage attribute.

Answer 89 · 2024-06-23T01:39:04.000Z

I have also filed #126721 (building on top of #126682), which makes #[coverage(..)] apply recursively, and also permits it on mod and impl blocks.

Answer 90 · 2024-06-23T04:29:47.000Z

I didn't figure it'd be that easy, but I'm happy it got done! I updated the stabilisation report with these PRs included, and put the old version under a fold so it's still viewable for posterity: #84605 (comment)

Answer 91 · 2024-06-23T05:21:14.000Z

Note that in the upcoming implementation, functions with #[coverage(on)] are still not guaranteed to be instrumented.

They are considered eligible for instrumentation, but the instrumentor might decide to skip them for other reasons.

Answer 92 · 2024-06-24T06:32:11.000Z

That's fair; I'll remove the guarantees about instrumentation being added since, although I'd like to have some sort of basic guarantee like "if the functions are run in the final executable, they will have coverage instrumentation" but it feels like the amount of caveats you have to add to make that work is not worth it at the moment.

Answer 93 · 2024-06-25T17:44:13.000Z

Are there known cases where the instrumentor won't create a counter for a function marked for coverage? If the coverage counter as created and added to the coverage map it doesn't matter if dead-code elimination or link-time GC removes the function, the coverage will still correctly report that the function wasn't hit.

Answer 94 · 2024-06-25T19:02:47.000Z

From what I understand, coverage is instrumented after dead code elimination, so, dead code won't ever be instrumented.

What you're saying makes sense as a solution, but I'm not sure it's possible with LLVM or the current setup, and it's better to just explain the state of coverage now than block the attribute on it being improved.

Answer 95 · 2024-06-25T19:13:44.000Z

I wasn't suggesting to hold up the implementation, I was just surprised there were cases where instrumentation (or at least, counters) weren't emitted for a function when coverage was enabled.

Though I'm surprised by your assertion, as I understand it, coverage markers are added by the frontend, whereas DCE is surely done by the backend (along with coverage-lowering which is uninteresting for this question)?

Answer 96 · 2024-06-25T19:31:55.000Z

Yeah, it's a very confusing system for sure.

My understanding is that the entire thing is done after code generation, meaning that effectively, only the code that ends up in the final executable will show up on the list of coverage markers. The compiler can add in extra annotations to source that tell the code generator how coverage should be set up, but ultimately, those markers are ignored until the final code is generated and the final list is made.

This is why so many suggestions about coverage involve disabling LTO, disabling dead code removal, etc. Because ultimately, the original source doesn't matter for coverage, and the instrumentation just tracks back to the original code from the code that's left at the end.

Answer 97 · 2024-06-25T19:47:27.000Z

That's true for things like gcov and kcov, where coverage data is reverse engineered from the compiled binary, but the whole point of frontend-driven coverage is that the compiler knows how to refer to the source code to create coverage data (see https://clang.llvm.org/docs/SourceBasedCodeCoverage.html and https://rustc-dev-guide.rust-lang.org/llvm-coverage-instrumentation.html which both make it clear that producing the coverage map and injecting the llvm.instrprof.* intrinsics into the IR - those intrinsics are then lowered by LLVM to update the counters, but the counters default to 0 so if that code gets stripped the counter will report that the code wasn't hit).

In fact a quick prototype shows that dead code is still reported as missed so maybe this is working today anyway?

$ rustc +nightly -C instrument-coverage src/main.rs -O
$ ./main
$ llvm-profdata-18 merge -sparse default_*.profraw -o default.profdata
$ llvm-cov show ./main --instr-profile default.profdata
    1|      1|fn main() {
    2|      1|    println!("Hello, world!");
    3|      1|}
    4|       |
    5|      0|fn wibble() {
    6|      0|    println!("Wibble!");
    7|      0|}

Edit: and yes, the wibble function was indeed stripped from the binary:

$ readelf --symbols --wide ./main | rustfilt | grep wibble
$ strings ./main | grep Wibble
$

Answer 98 · 2024-06-26T00:25:01.000Z

In my mind, the main thing we are stabilizing is the syntax of #[coverage(off)] and #[coverage(on)], so that they can be used in stable code without a compiler error.

The precise effect of those attributes should be left to the discretion of the compiler implementation, in much the same way that -Cinstrument-coverage itself stably exists, while its actual details are not stable.

(We can still document the intended meaning of those attributes, and in practice the compiler will make a good-faith effort to honour that as much as reasonably possible. But we shouldn't be making any stable promises about which functions do or don't get instrumented, just as we don't make any stable promises about how the code inside those functions is instrumented.)

Answer 99 · 2024-06-26T00:48:25.000Z

In the current implementation, there are three main reasons why a function might not show up in the final coverage output:

The compiler decides that it can't/shouldn't be instrumented (e.g. #[coverage(off)], #[naked], certain functions whose bodies are clearly unreachable).
The compiler tries to instrument a function, but can't find any spans worth instrumenting, so it gives up.
- It's hard for this to happen in “normal” code, but macros make things a lot more complicated.
The compiler does instrument a function, but somehow that function is omitted by codegen (e.g. due to a bug in the handling of coverage metadata for unused functions).

Answer 100 · 2024-06-26T06:51:15.000Z

An important note for T-lang raised by Oli on the implementation PR:

so the intent is for nested modules also to use the attribute from the outer module? This is a new concept in Rust iirc, so maybe add it as an important point on the tracking issue for T-lang to talk about during stabilization

Also copying my own response from there:

The current behaviour (scanning all the way up to the crate root) wasn't my original intention, but as I implemented the feature it ended up being (a) easier, and (b) plausibly the right design, so I decided to stick with it until/unless the team decides otherwise.