rust icon indicating copy to clipboard operation
rust copied to clipboard

coverage: Remove or migrate all unstable values of `-Cinstrument-coverage`

Open Zalathar opened this issue 5 months ago • 14 comments

(This PR was substantially overhauled from its original version, which migrated all of the existing unstable values intact.)

This PR takes the three nightly-only values that are currently accepted by -Cinstrument-coverage, completely removes two of them (except-unused-functions and except-unused-generics), and migrates the third (branch) over to a newly-introduced unstable flag -Zcoverage-options.

I have a few motivations for wanting to do this:

  • It's unclear whether anyone actually uses the except-unused-* values, so this serves as an opportunity to either remove them, or prompt existing users to object to their removal.
  • After #117199, the stable values of -Cinstrument-coverage treat it as a boolean-valued flag, so having nightly-only extra values feels out-of-place.
    • Nightly-only values also require extra ad-hoc code to make sure they aren't accidentally exposed to stable users.
  • The new system allows multiple different settings to be toggled independently, which isn't possible in the current single-value system.
  • The new system makes it easier to introduce new behaviour behind an unstable toggle, and then gather nightly-user feedback before possibly making it the default behaviour for all users.
  • The new system also gives us a convenient place to put relatively-narrow options that won't ever be the default, but that nightly users might still want access to.
  • It's likely that we will eventually want to give stable users more fine-grained control over coverage instrumentation. The new flag serves as a prototype of what that stable UI might eventually look like.

The branch option is a placeholder that currently does nothing. It will be used by #122322 to opt into branch coverage instrumentation.


I see -Zcoverage-options as something that will exist more-or-less indefinitely, though individual sub-options might come and go as appropriate. I think there will always be some demand for nightly-only toggles, so I don't see -Zcoverage-options itself ever being stable, though we might eventually stabilize something similar to it.

Zalathar avatar Mar 09 '24 07:03 Zalathar

r? @nnethercote

rustbot has assigned @nnethercote. 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

rustbot avatar Mar 09 '24 07:03 rustbot

Not sure what level of process this change needs. It only affects nightly-only functionality, but there might be people with opinions on this new direction for coverage options.

Zalathar avatar Mar 09 '24 07:03 Zalathar

Ideally I would prefer this to land before #122322, so that we don't land branch coverage under one nightly flag and then immediately move it to a different nightly flag. But if there is any pushback on this PR, then it would be OK to land the branch coverage implementation under the current flag.

Zalathar avatar Mar 11 '24 03:03 Zalathar

I think moving the unstable options from -Cinstrument-coverage to -Zcoverage is a good idea.

The unused-functions and unused-generics options were part of the original -Zinstrument-coverage implementation. I'm not sure how many users they have (if any), but I don't think there has ever been any serious attempt to stabilize them. Currently they're no trouble to support, so I don't have any particular reason to try to remove them entirely.

You say "no trouble" but I see a non-trivial amount of code to support them, including multiple test output files. If they're not needed, I would recommend removing them. (I'm generally in favour of pruning the -Z flag space.) What do you think?

nnethercote avatar Mar 12 '24 00:03 nnethercote

My original thinking was that I genuinely don't know whether people use the except-unused flags, and I don't have a specific reason to want to remove them, so I might as well keep them around.

But now that I think about it, I'm coming around to the idea that instead of asking existing users to pass a different unstable flag, we should just remove the flags and ask existing users (if any) to come complain about it.

The actual implementation is only a couple of lines each (plus plumbing), so removing that code now (and potentially adding it back later) should be fairly straightforward.

Zalathar avatar Mar 12 '24 00:03 Zalathar

I don't think removing them needs an MCP, but opening a Zulip thread might be nice, just to ask if anyone has any use for them.

nnethercote avatar Mar 12 '24 01:03 nnethercote

While implementing the removal, I did stumble across one of the original motivations for except-unused-generics: #79651.

If a generic function is unused in its own crate, but is used from other crates, we can end up with an “unused” record for a dummy instantiation of that function in the library crate, which then shows up in coverage reports even though all of its actual instantiations are used.

(I don't think the flag is a good way to solve that problem, so I'm still eager to remove it; I just wanted to mention this finding.)

Zalathar avatar Mar 12 '24 01:03 Zalathar

I don't think removing them needs an MCP, but opening a Zulip thread might be nice, just to ask if anyone has any use for them.

https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Removing.20unstable.20values.20of.20.60-Cinstrument-coverage.60

Zalathar avatar Mar 12 '24 01:03 Zalathar

I've completely overhauled this PR to first remove all unstable values of -Cinstrument-coverage, and then introduce a -Zcoverage-options flag that only supports the placeholders branch and no-branch.

Zalathar avatar Mar 12 '24 01:03 Zalathar

Nice to see the unused options gone, and all the code that supports them.

It's now a little strange to add -Zcoverage=branch in this PR without it actually doing anything, but with #122322 pending I think it's ok.

Just the one tiny nit to fix, and then:

@bors delegate=Zalathar

nnethercote avatar Mar 12 '24 21:03 nnethercote

:v: @Zalathar, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

bors avatar Mar 12 '24 21:03 bors

@bors r=@nnethercote

Zalathar avatar Mar 13 '24 00:03 Zalathar

:pushpin: Commit 3407fcc12ee1ace7267611c91b579f6f5cfcb01b has been approved by nnethercote

It is now in the queue for this repository.

bors avatar Mar 13 '24 00:03 bors

One thing I want to note explicitly is that the name of the flag is currently -Zcoverage-options (not -Zcoverage).

The last chance to bikeshed this flag name before it affects (nightly) users will be in #122322.

Of course, since it's an unstable flag we're still free to change it at any time after that, too.

Zalathar avatar Mar 13 '24 00:03 Zalathar