Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Remove or migrate all unstable values of -Cinstrument-coverage #122226

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Mar 9, 2024

(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 Change the documented implicit value of -C instrument-coverage to =yes #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.

@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Mar 9, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Mar 9, 2024

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
Copy link
Contributor Author

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.

@nnethercote
Copy link
Contributor

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?

@Zalathar
Copy link
Contributor Author

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.

@nnethercote
Copy link
Contributor

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.

@Zalathar
Copy link
Contributor Author

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
Copy link
Contributor Author

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 Zalathar force-pushed the zcoverage-options branch 2 times, most recently from e82af0b to 8cf4f94 Compare March 12, 2024 01:49
@Zalathar
Copy link
Contributor Author

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 Zalathar changed the title coverage: Migrate unstable options to new flag -Zcoverage-options coverage: Remove or migrate all unstable values of -Cinstrument-coverage Mar 12, 2024
@Zalathar Zalathar force-pushed the zcoverage-options branch 4 times, most recently from e373b16 to 228d15b Compare March 12, 2024 05:46
@nnethercote
Copy link
Contributor

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

@bors
Copy link
Contributor

bors commented Mar 12, 2024

✌️ @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

This new nightly-only flag can be used to toggle fine-grained flags that
control the details of coverage instrumentation.

Currently the only supported flag value is `branch` (or `no-branch`), which is
a placeholder for upcoming support for branch coverage. Other flag values can
be added in the future, to prototype proposed new behaviour, or to enable
special non-default behaviour.
@Zalathar
Copy link
Contributor Author

@bors r=@nnethercote

@bors
Copy link
Contributor

bors commented Mar 13, 2024

📌 Commit 3407fcc has been approved by nnethercote

It is now in the queue for this repository.

@bors 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 Mar 13, 2024
@Zalathar
Copy link
Contributor Author

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…hercote

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

(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 rust-lang#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 rust-lang#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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#121820 (pattern analysis: Store field indices in `DeconstructedPat` to avoid virtual wildcards)
 - rust-lang#121908 (match lowering: don't collect test alternatives ahead of time)
 - rust-lang#122203 (Add `intrinsic_name` to get plain intrinsic name)
 - rust-lang#122226 (coverage: Remove or migrate all unstable values of `-Cinstrument-coverage`)
 - rust-lang#122255 (Use `min_exhaustive_patterns` in core & std)
 - rust-lang#122360 ( Don't Create `ParamCandidate` When Obligation Contains Errors )
 - rust-lang#122375 (CFI: Break tests into smaller files)
 - rust-lang#122383 (Enable PR tracking review assignment for rust-lang/rust)
 - rust-lang#122386 (Move `Once` implementations to `sys`)
 - rust-lang#122400 (Fix ICE in diagnostics for parenthesized type arguments)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#121820 (pattern analysis: Store field indices in `DeconstructedPat` to avoid virtual wildcards)
 - rust-lang#121908 (match lowering: don't collect test alternatives ahead of time)
 - rust-lang#122203 (Add `intrinsic_name` to get plain intrinsic name)
 - rust-lang#122226 (coverage: Remove or migrate all unstable values of `-Cinstrument-coverage`)
 - rust-lang#122255 (Use `min_exhaustive_patterns` in core & std)
 - rust-lang#122360 ( Don't Create `ParamCandidate` When Obligation Contains Errors )
 - rust-lang#122383 (Enable PR tracking review assignment for rust-lang/rust)
 - rust-lang#122386 (Move `Once` implementations to `sys`)
 - rust-lang#122400 (Fix ICE in diagnostics for parenthesized type arguments)
 - rust-lang#122410 (rustdoc: do not preload fonts when browsing locally)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8b9ef3b into rust-lang:master Mar 13, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
Rollup merge of rust-lang#122226 - Zalathar:zcoverage-options, r=nnethercote

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

(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 rust-lang#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 rust-lang#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 Zalathar deleted the zcoverage-options branch March 13, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants