Change the documented implicit value of `-C instrument-coverage` to `=yes` #117199

Zalathar · 2023-10-26T03:27:03Z

The option-value parser for -Cinstrument-coverage= currently accepts the following stable values:

all (implicit value of plain -Cinstrument-coverage)
yes, y, on, true (undocumented aliases for all)
off (default; same as not specifying -Cinstrument-coverage)
no, n, false, 0 (undocumented aliases for off)

I'd like to rearrange and re-document the stable values as follows:

no (default; same as not specifying -Cinstrument-coverage)
n, off, false (documented aliases for no)
0 (undocumented alias for no)
yes (implicit value of plain -Cinstrument-coverage)
y, on, true (documented aliases for yes)
all (documented as currently an alias for yes that may change; discouraged but not deprecated)

The main changes being:

Documented default value changes from off to no
Documented implicit value changes from all to yes
Other boolean aliases (n, off, false, y, on, true) are explicitly documented
all remains currently an alias for yes, but is explicitly documented as being able to change in the future
0 remains an undocumented but stable alias for no
The actual behaviour of coverage instrumentation does not change

Why?

The choice of all as the implicit value only really makes sense in the context of the unstable except-unused-functions and except-unused-generics values. That arrangement was fine for an unstable flag, but it's confusing for a stable flag whose only other stable value is off, and will only become more confusing if we eventually want to stabilize other fine-grained coverage option values.

(Currently I'm not aware of any plans to stabilize other coverage option values, but that's why I think now is a fine time to make this change, well before anyone actually has to care about it.)

For example, if we ever add support for opt-in instrumentation of things that are not instrumented by -Cinstrument-coverage by default, it will be very strange for the all value to not actually instrument all things that we know how to instrument.

Compatibility impact

Because this is not a functional change, there is no immediate compatibility impact. However, changing the documented semantics of all opens up the possibility of future changes that could be considered retroactively breaking.

I don't think this is going to be a big deal in practice, for a few reasons:

The exact behaviour of coverage instrumentation is allowed to change, so changing the behaviour of all is not a stability-breaking change, as long as it still exists and does something reasonable.
-Cinstrument-coverage is mainly used by tools or scripts that can be easily updated if necessary. It's unusual for users to pass the flag directly, because processing the profiler output is complicated enough that tools/scripts tend to be necessary anyway.
Most people who are using coverage are probably relying on -Cinstrument-coverage rather than explicitly passing -Cinstrument-coverage=all, so the number of users actually affected by this change is likely to be low, and plausibly zero.

rustbot · 2023-10-26T03:27:11Z

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

Zalathar · 2023-10-26T03:27:28Z

@rustbot label +A-code-coverage

Zalathar · 2023-10-26T03:30:44Z

This changes the documented meaning of -Cinstrument-coverage=all (though the actual behaviour is currently unaffected), so it might require more process than just a PR.

Zalathar · 2023-10-26T03:57:46Z

The choice between yes/no and on/off as the canonical pair of values is mostly arbitrary, so I went with the pair that is more visually distinct. (From what I can tell, that pair also seems to preferred in some other cases?)

The value of `-Cinstrument-coverage=` doesn't need to be `Option` (Extracted from rust-lang#117199, since this is a purely internal cleanup that can land independently.) Not using this flag is identical to passing `-Cinstrument-coverage=off`, so there's no need to distinguish between `None` and `Some(Off)`.

Rollup merge of rust-lang#117207 - Zalathar:no-option, r=compiler-errors The value of `-Cinstrument-coverage=` doesn't need to be `Option` (Extracted from rust-lang#117199, since this is a purely internal cleanup that can land independently.) Not using this flag is identical to passing `-Cinstrument-coverage=off`, so there's no need to distinguish between `None` and `Some(Off)`.

b-naber · 2023-11-17T19:25:25Z

The change seems reasonable to me. I'm not completely sure whether this needs an MCP though. You write that '-Cinstrument-coverage is not really intended as a directly user-facing feature;', but strictly speaking it still is a user-facing feature, isn't it?

Zalathar · 2023-11-18T00:07:29Z

Yes, ultimately it is a user-facing feature.

All I mean by that comment is that the flag is mainly used by tools like cargo-llvm-cov. People can pass it directly to the compiler if they want, but doing so is unusual.

Zalathar · 2023-11-18T00:09:03Z

If we’re not sure about the process, I’m not opposed to doing an MCP just to be safe. I’ve already done most of the hard work of writing up the proposal, and there’s no particular urgency.

b-naber · 2023-11-18T00:12:28Z

Yes, user-facing changes do require an MCP: https://rustc-dev-guide.rust-lang.org/contributing.html?highlight=Mcp#major-changes

Zalathar · 2023-11-18T01:06:24Z

MCP filed as rust-lang/compiler-team#690.

Zalathar · 2024-01-31T01:31:46Z

The MCP has been seconded, so I've rebased this PR to make sure it's still in good shape.

fmease · 2024-03-06T06:37:16Z

#120690
r? compiler

…=yes`

Zalathar · 2024-03-06T06:52:29Z

Might as well rebase again; no changes.

Nadrieril · 2024-03-06T13:42:49Z

r? @oli-obk since you seconded the MCP

Fwiw the implementation looks good to me

oli-obk · 2024-03-06T15:19:37Z

@bors r=oli-obk,Nadrieril

bors · 2024-03-06T15:19:39Z

📌 Commit 9f287dd has been approved by oli-obk,Nadrieril

It is now in the queue for this repository.

…=oli-obk,Nadrieril Change the documented implicit value of `-C instrument-coverage` to `=yes` The option-value parser for `-Cinstrument-coverage=` currently accepts the following stable values: - `all` (implicit value of plain `-Cinstrument-coverage`) - `yes`, `y`, `on`, `true` (undocumented aliases for `all`) - `off` (default; same as not specifying `-Cinstrument-coverage`) - `no`, `n`, `false`, `0` (undocumented aliases for `off`) I'd like to rearrange and re-document the stable values as follows: - `no` (default; same as not specifying `-Cinstrument-coverage`) - `n`, `off`, `false` (documented aliases for `no`) - `0` (undocumented alias for `no`) - `yes` (implicit value of plain `-Cinstrument-coverage`) - `y`, `on`, `true` (documented aliases for `yes`) - `all` (documented as *currently* an alias for `yes` that may change; discouraged but not deprecated) The main changes being: - Documented default value changes from `off` to `no` - Documented implicit value changes from `all` to `yes` - Other boolean aliases (`n`, `off`, `false`, `y`, `on`, `true`) are explicitly documented - `all` remains currently an alias for `yes`, but is explicitly documented as being able to change in the future - `0` remains an undocumented but stable alias for `no` - The actual behaviour of coverage instrumentation does not change # Why? The choice of `all` as the implicit value only really makes sense in the context of the unstable `except-unused-functions` and `except-unused-generics` values. That arrangement was fine for an unstable flag, but it's confusing for a stable flag whose only other stable value is `off`, and will only become more confusing if we eventually want to stabilize other fine-grained coverage option values. (Currently I'm not aware of any plans to stabilize other coverage option values, but that's why I think now is a fine time to make this change, well before anyone actually has to care about it.) For example, if we ever add support for opt-in instrumentation of things that are *not* instrumented by `-Cinstrument-coverage` by default, it will be very strange for the `all` value to not actually instrument all things that we know how to instrument. # Compatibility impact Because this is not a functional change, there is no immediate compatibility impact. However, changing the documented semantics of `all` opens up the possibility of future changes that could be considered retroactively breaking. I don't think this is going to be a big deal in practice, for a few reasons: - The exact behaviour of coverage instrumentation is allowed to change, so changing the behaviour of `all` is not a *stability-breaking* change, as long as it still exists and does something reasonable. - `-Cinstrument-coverage` is mainly used by tools or scripts that can be easily updated if necessary. It's unusual for users to pass the flag directly, because processing the profiler output is complicated enough that tools/scripts tend to be necessary anyway. - Most people who are using coverage are probably relying on `-Cinstrument-coverage` rather than explicitly passing `-Cinstrument-coverage=all`, so the number of users actually affected by this change is likely to be low, and plausibly zero.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113518 (bootstrap/libtest: print test name eagerly on failure even with `verbose-tests=false` / `--quiet`) - rust-lang#117199 (Change the documented implicit value of `-C instrument-coverage` to `=yes`) - rust-lang#121190 (avoid overlapping privacy suggestion for single nested imports) - rust-lang#121382 (Rework `untranslatable_diagnostic` lint) - rust-lang#121833 (Suggest correct path in include_bytes!) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#122038 (Fix linting paths with qself in `unused_qualifications`) - rust-lang#122051 (cleanup: remove zero-offset GEP) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113518 (bootstrap/libtest: print test name eagerly on failure even with `verbose-tests=false` / `--quiet`) - rust-lang#117199 (Change the documented implicit value of `-C instrument-coverage` to `=yes`) - rust-lang#121190 (avoid overlapping privacy suggestion for single nested imports) - rust-lang#121382 (Rework `untranslatable_diagnostic` lint) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#122038 (Fix linting paths with qself in `unused_qualifications`) - rust-lang#122051 (cleanup: remove zero-offset GEP) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117199 - Zalathar:instrument-coverage-on, r=oli-obk,Nadrieril Change the documented implicit value of `-C instrument-coverage` to `=yes` The option-value parser for `-Cinstrument-coverage=` currently accepts the following stable values: - `all` (implicit value of plain `-Cinstrument-coverage`) - `yes`, `y`, `on`, `true` (undocumented aliases for `all`) - `off` (default; same as not specifying `-Cinstrument-coverage`) - `no`, `n`, `false`, `0` (undocumented aliases for `off`) I'd like to rearrange and re-document the stable values as follows: - `no` (default; same as not specifying `-Cinstrument-coverage`) - `n`, `off`, `false` (documented aliases for `no`) - `0` (undocumented alias for `no`) - `yes` (implicit value of plain `-Cinstrument-coverage`) - `y`, `on`, `true` (documented aliases for `yes`) - `all` (documented as *currently* an alias for `yes` that may change; discouraged but not deprecated) The main changes being: - Documented default value changes from `off` to `no` - Documented implicit value changes from `all` to `yes` - Other boolean aliases (`n`, `off`, `false`, `y`, `on`, `true`) are explicitly documented - `all` remains currently an alias for `yes`, but is explicitly documented as being able to change in the future - `0` remains an undocumented but stable alias for `no` - The actual behaviour of coverage instrumentation does not change # Why? The choice of `all` as the implicit value only really makes sense in the context of the unstable `except-unused-functions` and `except-unused-generics` values. That arrangement was fine for an unstable flag, but it's confusing for a stable flag whose only other stable value is `off`, and will only become more confusing if we eventually want to stabilize other fine-grained coverage option values. (Currently I'm not aware of any plans to stabilize other coverage option values, but that's why I think now is a fine time to make this change, well before anyone actually has to care about it.) For example, if we ever add support for opt-in instrumentation of things that are *not* instrumented by `-Cinstrument-coverage` by default, it will be very strange for the `all` value to not actually instrument all things that we know how to instrument. # Compatibility impact Because this is not a functional change, there is no immediate compatibility impact. However, changing the documented semantics of `all` opens up the possibility of future changes that could be considered retroactively breaking. I don't think this is going to be a big deal in practice, for a few reasons: - The exact behaviour of coverage instrumentation is allowed to change, so changing the behaviour of `all` is not a *stability-breaking* change, as long as it still exists and does something reasonable. - `-Cinstrument-coverage` is mainly used by tools or scripts that can be easily updated if necessary. It's unusual for users to pass the flag directly, because processing the profiler output is complicated enough that tools/scripts tend to be necessary anyway. - Most people who are using coverage are probably relying on `-Cinstrument-coverage` rather than explicitly passing `-Cinstrument-coverage=all`, so the number of users actually affected by this change is likely to be low, and plausibly zero.

…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.

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.

rustbot assigned b-naber Oct 26, 2023

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. labels Oct 26, 2023

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Oct 26, 2023

Zalathar changed the title ~~Change the documented implicit value of -C instrument-coverage to =on~~ Change the documented implicit value of -C instrument-coverage to =yes Oct 26, 2023

Zalathar force-pushed the instrument-coverage-on branch from 7de0e6a to 40d8911 Compare October 26, 2023 03:52

Swatinem approved these changes Oct 26, 2023

View reviewed changes

Zalathar mentioned this pull request Oct 26, 2023

The value of -Cinstrument-coverage= doesn't need to be Option #117207

Merged

This comment was marked as outdated.

Sign in to view

Zalathar force-pushed the instrument-coverage-on branch from 40d8911 to 39b9cb4 Compare October 27, 2023 06:17

Zalathar force-pushed the instrument-coverage-on branch from 39b9cb4 to 7a4ce83 Compare November 8, 2023 06:22

Zalathar mentioned this pull request Nov 18, 2023

Semantics of -Cinstrument-coverage=all rust-lang/compiler-team#690

Closed

3 tasks

apiraino added the S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. label Dec 28, 2023

Zalathar force-pushed the instrument-coverage-on branch from 7a4ce83 to b2c67f6 Compare January 31, 2024 01:31

Zalathar force-pushed the instrument-coverage-on branch from b2c67f6 to 474d660 Compare February 14, 2024 00:29

Dylan-DPC removed the S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. label Feb 16, 2024

Zalathar force-pushed the instrument-coverage-on branch from 474d660 to 1130e46 Compare February 23, 2024 02:49

rustbot assigned Nadrieril and unassigned b-naber Mar 6, 2024

Change the documented implicit value of -C instrument-coverage to `…

9f287dd

…=yes`

Zalathar force-pushed the instrument-coverage-on branch from 1130e46 to 9f287dd Compare March 6, 2024 06:52

rustbot assigned oli-obk and unassigned Nadrieril Mar 6, 2024

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 6, 2024

matthiaskrgr mentioned this pull request Mar 6, 2024

Rollup of 8 pull requests #122097

Closed

matthiaskrgr mentioned this pull request Mar 6, 2024

Rollup of 7 pull requests #122111

Merged

bors merged commit 4d9cdd6 into rust-lang:master Mar 6, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Mar 6, 2024

Zalathar deleted the instrument-coverage-on branch March 7, 2024 00:26

Zalathar mentioned this pull request Mar 9, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the documented implicit value of `-C instrument-coverage` to `=yes` #117199

Change the documented implicit value of `-C instrument-coverage` to `=yes` #117199

Zalathar commented Oct 26, 2023 •

edited

Loading

rustbot commented Oct 26, 2023

Zalathar commented Oct 26, 2023

Zalathar commented Oct 26, 2023

Zalathar commented Oct 26, 2023

This comment was marked as outdated.

b-naber commented Nov 17, 2023 •

edited

Loading

Zalathar commented Nov 18, 2023

Zalathar commented Nov 18, 2023

b-naber commented Nov 18, 2023

Zalathar commented Nov 18, 2023

Zalathar commented Jan 31, 2024

fmease commented Mar 6, 2024

Zalathar commented Mar 6, 2024

Nadrieril commented Mar 6, 2024

oli-obk commented Mar 6, 2024

bors commented Mar 6, 2024

Change the documented implicit value of -C instrument-coverage to =yes #117199

Change the documented implicit value of -C instrument-coverage to =yes #117199

Conversation

Zalathar commented Oct 26, 2023 • edited Loading

Why?

Compatibility impact

rustbot commented Oct 26, 2023

Zalathar commented Oct 26, 2023

Zalathar commented Oct 26, 2023

Zalathar commented Oct 26, 2023

This comment was marked as outdated.

b-naber commented Nov 17, 2023 • edited Loading

Zalathar commented Nov 18, 2023

Zalathar commented Nov 18, 2023

b-naber commented Nov 18, 2023

Zalathar commented Nov 18, 2023

Zalathar commented Jan 31, 2024

fmease commented Mar 6, 2024

Zalathar commented Mar 6, 2024

Nadrieril commented Mar 6, 2024

oli-obk commented Mar 6, 2024

bors commented Mar 6, 2024

Change the documented implicit value of `-C instrument-coverage` to `=yes` #117199

Change the documented implicit value of `-C instrument-coverage` to `=yes` #117199

Zalathar commented Oct 26, 2023 •

edited

Loading

b-naber commented Nov 17, 2023 •

edited

Loading