Adds feature-gated `#[no_coverage]` function attribute, to fix derived Eq `0` coverage issue #83601 by richkadel · Pull Request #84562 · rust-lang/rust

richkadel · 2021-04-25T20:45:53Z

Derived Eq no longer shows uncovered

The Eq trait has a special hidden function. MIR InstrumentCoverage
would add this function to the coverage map, but it is never called, so
the Eq trait would always appear uncovered.

Fixes: #83601

The fix required creating a new function attribute no_coverage to mark
functions that should be ignored by InstrumentCoverage and the
coverage mapgen (during codegen).

Adding a no_coverage feature gate with tracking issue #84605.

r? @tmandry
cc: @wesleywiser

library/core/src/cmp.rs

richkadel · 2021-04-27T03:19:39Z

I doubt this will fix #82853, but I think #84582 will fix that one.

compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs

The Eq trait has a special hidden function. MIR `InstrumentCoverage` would add this function to the coverage map, but it is never called, so the `Eq` trait would always appear uncovered. Fixes: rust-lang#83601 The fix required creating a new function attribute `no_coverage` to mark functions that should be ignored by `InstrumentCoverage` and the coverage `mapgen` (during codegen). While testing, I also noticed two other issues: * spanview debug file output ICEd on a function with no body. The workaround for this is included in this PR. * `assert_*!()` macro coverage can appear covered if followed by another `assert_*!()` macro. Normally they appear uncovered. I submitted a new Issue rust-lang#84561, and added a coverage test to demonstrate this issue.

richkadel · 2021-04-28T00:17:46Z

@nikomatsakis - Does this implementation work for you?

Is there anything else needed before this can be approved and merged (now that feature gating is supported)?

tmandry

I'm thinking we should go ahead and add this attribute to all the builtin derives (in either this change or a future one). I don't see much point in making sure they have coverage; they're already well tested in the compiler.

tmandry · 2021-04-28T00:42:25Z

compiler/rustc_typeck/src/collect.rs

+                if list.iter().any(|nested_meta_item| nested_meta_item.has_name(sym::no_coverage)) {
+                    tcx.sess.mark_attr_used(attr);
+                    no_coverage_feature_enabled = true;
+                }


Shouldn't AssumeUsed make this unnecessary?

No, it appears to be necessary. I tried removing it, and when compiling rustc I now get the error:

error: unused attribute --> library/core/src/cmp.rs:277:32 | 277 | #[cfg_attr(not(bootstrap), feature(no_coverage))] | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D unused-attributes` implied by `-D warnings`

tmandry · 2021-04-28T00:47:53Z

Looks great modulo comments.
@bors delegate+

bors · 2021-04-28T00:47:55Z

✌️ @richkadel can now approve this pull request

tmandry · 2021-04-28T01:01:45Z

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 · 2021-04-28T01:25:44Z

I'm thinking we should go ahead and add this attribute to all the builtin derives (in either this change or a future one). I don't see much point in making sure they have coverage; they're already well tested in the compiler.

Hmm... Maybe I could be convinced, but I think we should discuss this, and for now I think it should be left out of this PR.

The function with no_coverage, in Eq is a function that truly is never meant to be called, so it shouldn't contribute to coverage profiling by definition.

But cmp(), eq(), ne(), and partial_cmp() are meant to be called, and coverage results will show, for a user-defined type, whether they've exercised those functions or not (and hopefully, by exercising them, they've validated the expected behavior, preferably via a test.)

I think it's worth discussing, but skipping coverage reporting that can provide valid feedback to the user, for builtin traits, feels like a judgement call we would be making.

richkadel · 2021-04-28T01:38:22Z

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.

richkadel · 2021-04-28T03:13:07Z

@bors r=tmandry

bors · 2021-04-28T03:13:09Z

📌 Commit 3a5df48 has been approved by tmandry

tmandry · 2021-04-28T03:13:49Z

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

bors · 2021-04-28T13:05:21Z

⌛ Testing commit 3a5df48 with merge 20040fa...

bors · 2021-04-28T15:46:20Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 20040fa to master...

rust-highfive assigned tmandry Apr 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2021

jyn514 reviewed Apr 26, 2021

View reviewed changes

library/core/src/cmp.rs Outdated Show resolved Hide resolved

jyn514 added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Apr 26, 2021

richkadel force-pushed the issue-83601 branch from 265eca0 to e977835 Compare April 27, 2021 04:32

richkadel commented Apr 27, 2021

View reviewed changes

compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs Outdated Show resolved Hide resolved

Rich Kadel added 2 commits April 27, 2021 11:11

adds feature gating of no_coverage at either crate- or function-level

3a5df48

richkadel force-pushed the issue-83601 branch from e977835 to 3a5df48 Compare April 28, 2021 00:14

richkadel changed the title ~~Derived Eq no longer shows uncovered (fixes issue 83601)~~ Adds #[no_coverage] option for functions, to fix derived Eq 0 coverage issue #83601 Apr 28, 2021

richkadel changed the title ~~Adds #[no_coverage] option for functions, to fix derived Eq 0 coverage issue #83601~~ Adds feature-gated #[no_coverage] function attribute, to fix derived Eq 0 coverage issue #83601 Apr 28, 2021

tmandry reviewed Apr 28, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2021

bors merged commit 20040fa into rust-lang:master Apr 28, 2021

rustbot added this to the 1.53.0 milestone Apr 28, 2021

Uh oh!

Conversation

richkadel commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

richkadel commented Apr 27, 2021

Uh oh!

Uh oh!

richkadel commented Apr 28, 2021

Uh oh!

tmandry left a comment

Choose a reason for hiding this comment

Uh oh!

tmandry Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

richkadel Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

tmandry commented Apr 28, 2021

Uh oh!

bors commented Apr 28, 2021

Uh oh!

tmandry commented Apr 28, 2021

Uh oh!

richkadel commented Apr 28, 2021

Uh oh!

richkadel commented Apr 28, 2021

Uh oh!

richkadel commented Apr 28, 2021

Uh oh!

bors commented Apr 28, 2021

Uh oh!

tmandry commented Apr 28, 2021

Uh oh!

bors commented Apr 28, 2021

Uh oh!

bors commented Apr 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

richkadel commented Apr 25, 2021 •

edited

Loading