rfcs
rfcs copied to clipboard
rust-lang
RFC: Add native code coverage support in Cargo
This RFC proposes integrating Rust's support for source-based code coverage into Cargo.
Internals Thread Rendered rust-lang/cargo#13040
(Tarpaulin author) Making coverage easier to get for users on any system and reducing the overhead would definitely be a big quality of life improvement!
Just a note on tarpaulin it has alpha support for the llvm instrumentation https://github.com/xd009642/tarpaulin/issues/549, but it also plans in future of using probe-rs and providing options for embedded device coverage. Some way of reducing -Cinstrument-coverage to limit it to certain crates is definitely desired though. Without it the binary size is pretty large and it stops it being suitable to use in embedded (otherwise we could get embedded coverage using minicov.
As well as these changes making the coverage more applicable to more domains, it would benefit crates like cargo-llvm-cov and tarpaulin and enable coverage in more specific environments. Plus making coverage work for 99% of users without needing a 3rd party tool would be a big UX improvement.
As a new user to rust I want to say that this in my opinion would be a great step in the right direction. It would help make rust adoption simpler as there would be a clear way to check code coverage without needing to check what creates provide this functionality and potentially need to deep dive into them to choose one.
Bumping this. The Rust community has always heavily invested into its tooling. This is a feature found in many other ecosystems. It’d be great to see an ergonomic cargo flag as a solution.
You can actually specify which crates to test when giving the
cargo testcommand in a workspace. I think this RFC should talk about this scenario right now instead of leaving it as a future possibility.
I'll add this in as opposed to leaving it as a future possibility.
Good update. But I still see some references to "top-level crate" which is a bit confusing with the new changes.
Good update. But I still see some references to "top-level crate" which is a bit confusing with the new changes.
I've removed the references to top-level crate. Thanks for the feedback!
I'm very new to rust and not sure if I missed it somewhere higher up but wanted to give my opinion on back-end support from my humble point of view.
Even if in the initial working version we only have LLVM support that is still better than no built-in support. As a new user for me it would clearly identify what is at least a generally considered a good way to do coverage testing without doing a lot of research on the topic. I got the impression this change was mostly targeted at beginners, and I think beginners often don't mind working with defaults to get results to get started. Like using LLVM or whatever other backend is required to get started.
That is not to say it should be included in Future Possibilities. However in the meanwhile, you could use other backends when debugging and only need to use LLVM if you want to test coverage.
@epage thank you for taking the time to review this PR, it is greatly appreciated. I've made the updates you requested and added doctests to the initial iteration of this RFC.
Thank you for your response @nrc
It should be a platform for supporting all things, but only include default support for core functionality. I don't think code coverage makes that mark
I don't necessarily agree with this statement. Code coverage is an essential part of the development lifecycle. This only boldens the argument for writing tests and ensuring tests capture core functionality of any project. The Rust compiler also has a mechanism for gathering coverage data in two ways. Instrumentation based coverage is supported in stable and a gcc coverage implementation is in unstable. Having stable support for instrumentation-based coverage in rustc should ensure that Cargo should not have any risks of this feature becoming "outdated". Also with any feature, having an agreed upon RFC would allow any Rust developer to be able to find the default suggested way of collecting coverage metrics in a Rust project via Cargo. Cargo could support code coverage in the same manner it supports rustdoc, making it very easy for a Rust developer to build their documentation and open it in a browser. This RFC is proposing leveraging the support rustc currently provides along with the components provided by the rust toolchain, i.e. llvm-tools-preview.
I don't necessarily agree with this statement. Code coverage is an essential part of the development lifecycle.
So, personally, I strongly disagree with this. I think code coverage is a flaky heuristic masquerading as a metric and it encourages useless boilerplate testing at the expense of testing any edge cases which don't show up as branches. BUT, that isn't really important. I do think it is sometimes useful and I do think some people consider it an essential part of their development. However, my understanding of the wider dev community (which is what is important here) is that there is no consensus on it being essential and it isn't in the same tier as unit testing, benchmarking, or documentation. I think (and the Cargo team might have a different opinion) that to qualify for core support in Cargo, something must either have almost unanimous support in the community (like testing) or be something that we are specifically pushing as an advantage of Rust (e.g., Rustdoc). (or possibly be something which is important and fundamentally cannot be supported as a plugin). I don't think coverage meets these requirements, but perhaps it has become a lot more widely accepted recently?
Anyway, I don't think being a plugin is a bad thing! Some really great tools are Cargo plugins rather than having built-in support (Rustfmt, Clippy, cargo audit, etc.). I would love for Rust to have best-in-class support for code coverage (as a plugin) and I'd support changes to Cargo to facilitate that.
The Rust compiler also has a mechanism for gathering coverage data in two ways. Instrumentation based coverage is supported in stable and a gcc coverage implementation is in unstable
It actually kinda worries me that there are already two implementations here. It seems like that means it is more likely for there to be more in the future. They are also closely tied to the compiler backend, so support with a different backend (e.g., Cranelift) or with a different compiler is likely to be different or non-existent. That seems like another good reason for this to be a plugin to Cargo rather than built-in.
If I'm understanding plugins correctly then Tarpaulin is already a coverage plugin for cargo (among others) https://lib.rs/crates/cargo-tarpaulin
I found it in the list of plugins for cargo https://lib.rs/development-tools/cargo-plugins
The only concern I have is that it comes back to being a bit confusing for new users which one they should pick
If I'm understanding plugins correctly then Tarpaulin is already a coverage plugin for cargo (among others) https://lib.rs/crates/cargo-tarpaulin
Yes, Tarpaulin is a plugin along with llvm-cov.
The only concern I have is that it comes back to being a bit confusing for new users which one they should pick
I agree with your sentiment which is why I introduced this RFC as adding this support directly in Cargo to help alleviate some of those concerns. @nrc makes some good points around how plugins can be a good thing in terms of handling code coverage for Rust projects, but it is also clear that Cargo is lacking some features that would allow these plugins to have the full picture around which crates need to be instrumented.
@nrc
As per our discussion, I pushed out a separate RFC to support a generic mechanism for setting rustc flags via Cargo. This allows setting any Rust compiler flag, not just code coverage flags, and enables them only on the current crate being built. Dependencies including transitive dependencies will not have the flags applied. This new feature will make it easier to enable rustc flags regardless of the cargo command being run.
To be honest, I see code coverage as being to cargo test as Clippy (and especially clippy::restriction) is to cargo build.
Yes, it can be abused and misinterpreted, but so can lints, and both are a pain to implement and maintain without support from the compiler.
@ssokolow I agree with this sentiment, thus the reason for opening this RFC. In my opinion having support for code coverage integrated directly into Cargo would be extremely helpful for a large group of Rust developers. I understand that there is some disagreement in how beneficial code coverage can be but having builtin stable support for it so those Rust developers that find it extremely essential in elevating code quality are able to easily use it.
One quick note — I would want to make sure this knows to not clobber between coverage and non-coverage artifacts so that the last two calls in this sequence are no-ops:
$ cargo test --no-run
$ cargo test --coverage --no-run
$ cargo test --no-run
$ cargo test --coverage --no-run
One quick note — I would want to make sure this knows to not clobber between coverage and non-coverage artifacts so that the last two calls in this sequence are no-ops:
$ cargo test --no-run $ cargo test --coverage --no-run $ cargo test --no-run $ cargo test --coverage --no-run
That would not be the case. The --coverage flag alters the rustflags enabled for a given crate by forcing cargo to re-compile the root crate to ensure its libraries are instrumented. This has a direct change in the artifacts produced so for the example above, the last two calls would both cause the top-level crate and any of its dependencies to be compiled again since the invocations to rustc will have a different set of flags enabled.
I understand that that's the case if implemented directly as a way to modify rustflags. But I'd like to have that not be the case with --coverage. If we truly want "native code coverage support in Cargo", then that should (imo) include teaching Cargo to distinguish between the coverage and non-coverage artifacts so that it doesn't let them clobber each other.
Ahh I understand your comment now. That is very interesting, my only worry about this adding additional artifacts for each crate that is being instrumented and potentially having 'coverage artifacts' for each target being compiled which can quickly expand the number of artifacts for a simple build. This might be as simple as adding a separate target/out directory when the --coverage flag is enabled.
I do see the benefit of this approach and will certainly add it to the RFC as an Alternative as well as an Unanswered question. I would love to get input from the cargo team on this as well and see what their thought on this approach would be. In the meantime, I'll look into what exactly this will mean for cargo and what changes would be necessary.
Thanks for pointing out this alternative implementation :)
Just posting an update: The Cargo team discussed this RFC a bit. I think in the short term we would like to see more experimentation done as third-party commands. We may consider more first-class support in the future, as it would make it more accessible. However, it seems like there is quite a bit of space to explore.
It sounds like one of the biggest concerns expressed in this RFC with the current third-party approach (such as cargo llvm-cov) is the inability to limit the coverage from being collected from dependencies. We are interested in looking at adding more control over how RUSTFLAGS can be set (such as #3310). In the short term, you can try a hack using profiles, something like:
[profile.codecov]
inherits = "dev"
rustflags = ["-Cinstrument-coverage"]
[profile.codecov.package."*"]
rustflags = []
I understand that is not ideal, and due to a limitation of the current implementation this can't be used via .cargo/config.toml. But it is at least something to experiment with.
Another thing that would be helpful is to include more of a survey of code coverage in other ecosystems. What lessons can we learn from them?
@ehuss Thanks for the detailed response!
Yes, that hack should be sufficient enough to use as a workaround for the main issue of using third-party commands to handle collecting code coverage for a specific crate. My biggest concern is that this is not as ergonomic as I would have hoped for collecting code coverage as it adds more onus on the user of the third-party command to set these profile specific rustflags rather than making it ergonomic for the third-party command to handle this.
I agree that having a better way of setting RUSTFLAGS on a crate-by-crate basis would alleviate this scenario and enhance how third-party commands collect coverage metrics and would make it simpler for a plugin to set the specific RUSTFLAG needed to collect coverage for a given crate.
I have started this conversation here.
However, it seems like there is quite a bit of space to explore.
Could you elaborate on what you mean here? It would be great to get this documented to see the full scope of hurdles that would be blocking first-class support for handling code coverage within Cargo.
I'm going to propose to postpone this RFC. The Cargo team agrees that we would like to see first-party support for coverage support and recognizes that the current options are not great. However, we have concerns about how this ties directly into the llvm tools, and the uncertainty on how that works with various scenarios (different LLVM versions, different codegen backends, different approaches of coverage instrumentation, etc.). Primarily, there isn't anyone on the team who has the capacity at this time to champion this feature.
As a compromise, we may be interested in possibly seeing some unstable experimentation done in Cargo, where we can explore options for the solution. This would be with the intent that an RFC would follow up before stabilization. If anyone is interested in working on that, feel free to reach out to the Cargo team on #t-cargo on Zulip, or on the issue https://github.com/rust-lang/cargo/issues/13040, or come to Office hours. We would probably like to see it broken into smaller pieces, such as adding an option to have Cargo pass the -Cinstrument-coverage flag, but not do anything else. Or maybe more investigation into better RUSTFLAGS support (like https://github.com/rust-lang/rfcs/pull/3310).
@rfcbot fcp postpone
Team member @ehuss has proposed to postpone this. The next step is review by the rest of the tagged team members:
- [x] @Eh2406
- [ ] @Muscraft
- [x] @arlosi
- [x] @ehuss
- [x] @epage
- [ ] @joshtriplett
- [x] @weihanglo
No concerns currently listed.
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!
See this document for info about what commands tagged team members can give me.
I think it would make sense to add a flag that lets you only generate the profraw files and stop there. This way you could manually merge the coverage of multiple executions of cargo test which you may be invoking for testing different configurations (different features, different targets, etc).
The final comment period, with a disposition to postpone, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This is now postponed.