coverage: Don't instrument `#[automatically_derived]` functions #120185

Zalathar · 2024-01-21T04:43:52Z

This PR makes the coverage instrumentor detect and skip functions that have #[automatically_derived] on their enclosing impl block.

Most notably, this means that methods generated by built-in derives (e.g. Clone, Debug, PartialEq) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports.

This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. #105055, #84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default.

It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default.

(Also note that while -Cinstrument-coverage is a stable feature, the exact details of coverage instrumentation are allowed to change. So we can make this change; the main question is whether we should.)

Fixes #105055.

rustbot · 2024-01-21T04:44:00Z

r? @b-naber

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

rustbot · 2024-01-21T04:44:02Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Swatinem

I agree that skipping these is a much better default behavior.

If users want these derives to be instrumented, they can implement them manually. Which btw is trivial as rust-analyzer will auto-generate all of the inner derive code automatically.

oli-obk · 2024-01-21T11:04:42Z

@bors r rollup

If it becomes an issue we can make it configurable

bors · 2024-01-21T11:04:45Z

📌 Commit 7263a76 has been approved by oli-obk

It is now in the queue for this repository.

coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

Rollup of 12 pull requests Successful merges: - rust-lang#118639 (Undeprecate lint `unstable_features` and make use of it in the compiler) - rust-lang#118714 ( Explanation that fields are being used when deriving `(Partial)Ord` on enums) - rust-lang#119801 (Fix deallocation with wrong allocator in (A)Rc::from_box_in) - rust-lang#119948 (Make `unsafe_op_in_unsafe_fn` migrated in edition 2024) - rust-lang#119999 (remote-test: use u64 to represent file size) - rust-lang#120058 (bootstrap: improvements for compiler builds) - rust-lang#120160 (Manually implement derived `NonZero` traits.) - rust-lang#120177 (Remove duplicate dependencies for rustc) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120194 (Shorten `#[must_use]` Diagnostic Message for `Option::is_none`) - rust-lang#120200 (Correct the anchor of an URL in an error message) - rust-lang#120203 (Replace `#!/bin/bash` with `#!/usr/bin/env bash` in rust-installer tests) r? `@ghost` `@rustbot` modify labels: rollup

Zalathar · 2024-01-22T01:12:43Z

Ugh, failed in rollup because I forgot to bless the separate coverage-run-rustdoc suite (again).

Zalathar · 2024-01-22T01:21:45Z

Rebased and re-blessed (diff); no functional changes since last review.

@rustbot ready

saethlin · 2024-01-22T03:01:29Z

@bors r=oli-obk

bors · 2024-01-22T03:01:31Z

📌 Commit 41dcba8 has been approved by oli-obk

It is now in the queue for this repository.

matthiaskrgr · 2024-01-22T06:53:54Z

@bors r-
may have failed in #120209 (comment) ?

Zalathar · 2024-01-22T07:08:26Z

@matthiaskrgr Yes, I noticed this earlier today and have already pushed a fix, so this PR is ready to merge again in its current state.

wesleywiser · 2024-01-22T18:59:30Z

@bors r

bors · 2024-01-22T18:59:33Z

📌 Commit 41dcba8 has been approved by wesleywiser

It is now in the queue for this repository.

coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119433 (rc,sync: Do not create references to uninitialized values) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119766 (Split tait and impl trait in assoc items logic) - rust-lang#120160 (Manually implement derived `NonZero` traits.) - rust-lang#120177 (Remove duplicate dependencies for rustc) - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120201 (Bump some deps with syn 1.0 dependencies) - rust-lang#120246 (Re-add estebank to review rotation) r? `@ghost` `@rustbot` modify labels: rollup

coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

Rollup of 13 pull requests Successful merges: - rust-lang#119028 (Add more weirdness to weird-exprs.rs) - rust-lang#119805 (Suggest array::from_fn for array initialization) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120177 (Remove duplicate dependencies for rustc) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120188 (compiler: update freebsd and netbsd base specs.) - rust-lang#120201 (Bump some deps with syn 1.0 dependencies) - rust-lang#120215 (Update some deps with `bitflags` v1 dependencies) - rust-lang#120244 (Use `Self` in `NonZero*` implementations.) - rust-lang#120246 (Re-add estebank to review rotation) - rust-lang#120252 (rename `RawTy` to `LoweredTy`) - rust-lang#120255 (correct my mailmap entry) - rust-lang#120270 (A bunch of random modifications) r? `@ghost` `@rustbot` modify labels: rollup

coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

Rollup of 7 pull requests Successful merges: - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120277 (Normalize field types before checking validity) - rust-lang#120285 (Remove extra # from url in suggestion) r? `@ghost` `@rustbot` modify labels: rollup

coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

Rollup of 7 pull requests Successful merges: - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120165 (Switch `NonZero` alias direction.) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120285 (Remove extra # from url in suggestion) r? `@ghost` `@rustbot` modify labels: rollup

coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

Rollup of 10 pull requests Successful merges: - rust-lang#114764 ([style edition 2024] Combine all delimited exprs as last argument) - rust-lang#118326 (Add `NonZero*::count_ones`) - rust-lang#118636 (Add the unstable option to reduce the binary size of dynamic library…) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120284 (privacy: Refactor top-level visiting in `TypePrivacyVisitor`) - rust-lang#120285 (Remove extra # from url in suggestion) - rust-lang#120299 (Add mw to review rotation and add some owner assignments) Failed merges: - rust-lang#120124 (Split assembly tests for ELF and MachO) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 9 pull requests Successful merges: - rust-lang#114764 ([style edition 2024] Combine all delimited exprs as last argument) - rust-lang#118326 (Add `NonZero*::count_ones`) - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions) - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target) - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions) - rust-lang#120265 (Remove no-system-llvm) - rust-lang#120284 (privacy: Refactor top-level visiting in `TypePrivacyVisitor`) - rust-lang#120285 (Remove extra # from url in suggestion) - rust-lang#120299 (Add mw to review rotation and add some owner assignments) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120185 - Zalathar:auto-derived, r=wesleywiser coverage: Don't instrument `#[automatically_derived]` functions This PR makes the coverage instrumentor detect and skip functions that have [`#[automatically_derived]`](https://doc.rust-lang.org/reference/attributes/derive.html#the-automatically_derived-attribute) on their enclosing impl block. Most notably, this means that methods generated by built-in derives (e.g. `Clone`, `Debug`, `PartialEq`) are now ignored by coverage instrumentation, and won't appear as executed or not-executed in coverage reports. This is a noticeable change in user-visible behaviour, but overall I think it's a net improvement. For example, we've had a few user requests for this sort of change (e.g. rust-lang#105055, rust-lang#84605 (comment)), and I believe it's the behaviour that most users will expect/prefer by default. It's possible to imagine situations where users would want to instrument these derived implementations, but I think it's OK to treat that as an opportunity to consider adding more fine-grained option flags to control the details of coverage instrumentation, while leaving this new behaviour as the default. (Also note that while `-Cinstrument-coverage` is a stable feature, the exact details of coverage instrumentation are allowed to change. So we *can* make this change; the main question is whether we *should*.) Fixes rust-lang#105055.

rustbot assigned b-naber Jan 21, 2024

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 Jan 21, 2024

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Jan 21, 2024

Zalathar mentioned this pull request Jan 21, 2024

Tracking issue for function attribute #[coverage] #84605

Open

Swatinem approved these changes Jan 21, 2024

View reviewed changes

This was referenced Jan 21, 2024

Clone is required in a bound by Copy, however Clone is marked as uncovered taiki-e/cargo-llvm-cov#193

Closed

Disable coverage for derived traits taiki-e/cargo-llvm-cov#222

Closed

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 Jan 21, 2024

TaKO8Ki mentioned this pull request Jan 21, 2024

Rollup of 12 pull requests #120209

Closed

Zalathar force-pushed the auto-derived branch from 7263a76 to 5dda729 Compare January 22, 2024 01:17

coverage: Don't instrument #[automatically_derived] functions

41dcba8

Zalathar force-pushed the auto-derived branch from 5dda729 to 41dcba8 Compare January 22, 2024 01:21

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2024

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2024

wesleywiser approved these changes Jan 22, 2024

View reviewed changes

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 Jan 22, 2024

matthiaskrgr mentioned this pull request Jan 22, 2024

Rollup of 9 pull requests #120249

Closed

fmease mentioned this pull request Jan 23, 2024

Rollup of 13 pull requests #120271

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 7 pull requests #120289

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 7 pull requests #120290

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 10 pull requests #120303

Closed

fmease mentioned this pull request Jan 24, 2024

Rollup of 9 pull requests #120309

Merged

bors merged commit 8bd126c into rust-lang:master Jan 24, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 24, 2024

Zalathar deleted the auto-derived branch January 24, 2024 23:04

davidhewitt mentioned this pull request Mar 23, 2024

PyO3 v0.21 beta pyfunction annotation coverage PyO3/pyo3#3989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Don't instrument `#[automatically_derived]` functions #120185

coverage: Don't instrument `#[automatically_derived]` functions #120185

Zalathar commented Jan 21, 2024 •

edited

Loading

rustbot commented Jan 21, 2024

rustbot commented Jan 21, 2024

Swatinem left a comment

oli-obk commented Jan 21, 2024

bors commented Jan 21, 2024

Zalathar commented Jan 22, 2024

Zalathar commented Jan 22, 2024 •

edited

Loading

saethlin commented Jan 22, 2024

bors commented Jan 22, 2024

matthiaskrgr commented Jan 22, 2024

Zalathar commented Jan 22, 2024

wesleywiser commented Jan 22, 2024

bors commented Jan 22, 2024

coverage: Don't instrument #[automatically_derived] functions #120185

coverage: Don't instrument #[automatically_derived] functions #120185

Conversation

Zalathar commented Jan 21, 2024 • edited Loading

rustbot commented Jan 21, 2024

rustbot commented Jan 21, 2024

Swatinem left a comment

Choose a reason for hiding this comment

oli-obk commented Jan 21, 2024

bors commented Jan 21, 2024

Zalathar commented Jan 22, 2024

Zalathar commented Jan 22, 2024 • edited Loading

saethlin commented Jan 22, 2024

bors commented Jan 22, 2024

matthiaskrgr commented Jan 22, 2024

Zalathar commented Jan 22, 2024

wesleywiser commented Jan 22, 2024

bors commented Jan 22, 2024

coverage: Don't instrument `#[automatically_derived]` functions #120185

coverage: Don't instrument `#[automatically_derived]` functions #120185

Zalathar commented Jan 21, 2024 •

edited

Loading

Zalathar commented Jan 22, 2024 •

edited

Loading