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.

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.

* Add automatically_derived to all impl functions in deku-derive * Motivation: rust-lang/rust#120185

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

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label 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

daxpedda mentioned this pull request Aug 18, 2024

Use #[automatically_derived] instead of #[coverage(off)] rustwasm/wasm-bindgen#4078

Merged

BlueGreenMagick mentioned this pull request Aug 30, 2024

Put #[automatically_derived] on impls siefkenj/tsify#44

Merged

oxalica mentioned this pull request Sep 22, 2024

Mark #[automatically_derived] for generated impls dtolnay/thiserror#322

Merged

wcampbell0x2a added a commit to sharksforarms/deku that referenced this pull request Oct 27, 2024

Mark auto derived functions

affa54c

* Add automatically_derived to all impl functions in deku-derive * Motivation: rust-lang/rust#120185

wcampbell0x2a mentioned this pull request Oct 27, 2024

Mark auto derived functions sharksforarms/deku#495

Merged

wcampbell0x2a added a commit to sharksforarms/deku that referenced this pull request Oct 29, 2024

Mark auto derived functions (#495)

b3be1e3

* Add automatically_derived to all impl functions in deku-derive * Motivation: rust-lang/rust#120185

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

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

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