Adds feature-gated #[no_coverage] function attribute, to fix derived Eq 0 coverage issue #83601

[richkadel]

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

[richkadel]

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

[richkadel]

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

Shouldn't AssumeUsed make this unnecessary?

[richkadel]

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]

Looks great modulo comments.
@bors delegate+

[bors]

✌️ @richkadel can now approve this pull request

[tmandry]

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]

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]

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]

@bors r=tmandry

[bors]

📌 Commit 3a5df48 has been approved by tmandry

[tmandry]

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]

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

[bors]

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