Do not generate coverage records for compile-time-only functions

[fs-rachel]

These functions (which are generally, though not exclusively, intrinsics) should always
resolve to a definite value at compile time, and so should never be called at runtime.
We ensure that these functions never reach codegen using a check in compute_symbol_name
in compiler/rustc_symbol_mangling/src/lib.rs.

However, when generating coverage, prepare_covfun_records_for_unused_functions will
detect them as unused functions (since every call has been replaced by a constant by
this point) and try to generate dummy coverage records. This will trip the above check;
to avoid that, skip generating coverage for such functions.

This is necessary to correctly generate coverage data for core.

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

[rustbot]

r? @mu001999

rustbot has assigned @mu001999.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir, mir-opt
• compiler, mir, mir-opt expanded to 75 candidates
• Random selection from 18 candidates

[mu001999]

@rustbot reroll

[fmease]

Could you add a regression test and link to the corresponding GH issue if there's one (I think I saw one report earlier but I'm on mobile rn)¹? Thanks!

Footnotes

1. Ah, I was mistaken. I saw https://github.com/rust-lang/rust/issues/161770 which is only related. ↩

[Zalathar]

I'm not familiar with intrinsics, but if something is not supposed to ever exist at runtime then it makes sense to not consider it “unused”.

[Zalathar]

Instead of doing two query-cache lookups for tcx.def_kind(def_id) (above and here), we should call it once and stash the result in a local variable.

View changes since the review

[Zalathar]

Style-wise, I think this condition would be a little clearer as:

if matches!(def_kind, DefKind::Fn | DefKind::AssocFn)
    && matches!(tcx.constness(def_id), Constness::Const { always: true })
{
    return false;
}

[fs-rachel]

That does look a lot nicer, I've incorporated that in the new version of the PR.

Though ideally it should match the check in compute_symbol_name, which IMHO looks a lot less intuitive with that change

Current:

    if let DefKind::Fn | DefKind::AssocFn = def_kind {
        debug_assert!(tcx.constness(instance.def_id()) != hir::Constness::Const { always: true });
    }

Applying that change and running ./x fmt:

    if matches!(def_kind, DefKind::Fn | DefKind::AssocFn) {
        debug_assert!(!matches!(
            tcx.constness(instance.def_id()),
            hir::Constness::Const { always: true }
        ));
    }

What are your thoughts if I split the difference like this?

    if matches!(def_kind, DefKind::Fn | DefKind::AssocFn) {
        debug_assert!(tcx.constness(instance.def_id()) != hir::Constness::Const { always: true });
    }

[fs-rachel]

Could you add a regression test [...] Thanks!

Done! I was worried it would be difficult because actually generating coverage for core is non-trivial (our fork has a whole pile of infrastructure for it). But on looking at the existing tests, I realized you can just whack the relevant attribute on a random function, so I did that instead.

[fs-rachel]

Also I rewrote the comment + commit message to hopefully be clearer about how this all fits together

[Zalathar]

Wow, that clippy suggestion is awful. I guess we can ignore the lint at the function level.

[fs-rachel]

Realized the test only needs //@ build-pass, not //@ run-pass

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.