-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
r? @nnethercote rustbot has assigned @nnethercote. Use r? to explicitly pick a reviewer |
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. |
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. |
I think moving the unstable options from
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 |
My original thinking was that I genuinely don't know whether people use the 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. |
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. |
While implementing the removal, I did stumble across one of the original motivations for 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.) |
|
e82af0b
to
8cf4f94
Compare
I've completely overhauled this PR to first remove all unstable values of |
-Zcoverage-options
-Cinstrument-coverage
e373b16
to
228d15b
Compare
✌️ @Zalathar, you can now approve this pull request! If @nnethercote told you to " |
228d15b
to
f266b8a
Compare
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.
f266b8a
to
3407fcc
Compare
One thing I want to note explicitly is that the name of the flag is currently 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. |
…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.
…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
…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
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.
(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
andexcept-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:
except-unused-*
values, so this serves as an opportunity to either remove them, or prompt existing users to object to their removal.-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.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.