coverage: Don't instrument #[automatically_derived] functions#120185
Merged
bors merged 1 commit intorust-lang:masterfrom Jan 24, 2024
Merged
coverage: Don't instrument #[automatically_derived] functions#120185bors merged 1 commit intorust-lang:masterfrom
#[automatically_derived] functions#120185bors merged 1 commit intorust-lang:masterfrom
Conversation
Collaborator
|
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
Collaborator
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
rustbot
added
the
A-code-coverage
Swatinem
approved these changes
Jan 21, 2024
Contributor
Swatinem
left a comment
Swatinem
left a comment
There was a problem hiding this comment.
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.
This was referenced Jan 21, 2024
Contributor
|
@bors r+ rollup If it becomes an issue we can make it configurable |
Collaborator
bors
added
S-waiting-on-bors
TaKO8Ki
added a commit
to TaKO8Ki/rust
that referenced
this pull request
Jan 21, 2024
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.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 21, 2024
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
Member
Author
|
Ugh, failed in rollup because I forgot to bless the separate |
Zalathar
force-pushed
the
auto-derived
branch
from
7263a76 to
5dda729
Compare
Zalathar
force-pushed
the
auto-derived
branch
from
5dda729 to
41dcba8
Compare
Member
Author
rustbot
added
the
S-waiting-on-review
Member
|
@bors r=oli-obk |
Collaborator
bors
removed
the
S-waiting-on-review
Member
|
@bors r- |
bors
added
S-waiting-on-author
Member
Author
|
@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. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2024
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
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2024
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
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2024
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.
wcampbell0x2a
added a commit
to sharksforarms/deku
that referenced
this pull request
Oct 27, 2024
* Add automatically_derived to all impl functions in deku-derive * Motivation: rust-lang/rust#120185
wcampbell0x2a
added a commit
to sharksforarms/deku
that referenced
this pull request
Oct 29, 2024
* Add automatically_derived to all impl functions in deku-derive * Motivation: rust-lang/rust#120185
saiintbrisson
added a commit
to saiintbrisson/scale-info
that referenced
this pull request
Feb 14, 2025
As per rust-lang/rust#120185, functions with their enclosing impl body marked as `#[automatically_derived]` won't be featured in coverage reports.
niklasad1
pushed a commit
to paritytech/scale-info
that referenced
this pull request
Feb 19, 2025
* chore: mark TypeInfo derived impl as automatically_derived As per rust-lang/rust#120185, functions with their enclosing impl body marked as `#[automatically_derived]` won't be featured in coverage reports. * chore: elide lifetimes * Update tests.rs paritytech/parity-scale-codec/pull/653 disallowed duplicate indexes for variants. * fix: ui tests were outdated
guzman-raphael
added a commit
to guzman-raphael/getset
that referenced
this pull request
Apr 16, 2025
bors
added a commit
that referenced
this pull request
Jul 29, 2025
coverage: Treat `#[automatically_derived]` as `#[coverage(off)]` One of the contributing factors behind #141577 (comment) was the presence of derive-macro-generated code containing nested closures. Coverage instrumentation already has a heuristic for skipping code marked with `#[automatically_derived]` (#120185), because derived code is usually not worth instrumenting, and also has a tendency to trigger vexing edge-case bugs in coverage instrumentation or coverage codegen. However, the existing heuristic only applied to the associated items directly within an auto-derived impl block, and had no effect on closures or nested items within those associated items. This PR therefore extends the search for `#[coverage(..)]` attributes to also treat `#[automatically_derived]` as an implied `#[coverage(off)]` for the purposes of coverage instrumentation. --- This change doesn’t rule out an entire category of bugs, because it only affects code that actually uses the auto-derived attribute. But it should reduce the overall chance of edge-case macro span bugs being observed in the wild.
bors
added a commit
that referenced
this pull request
Jul 29, 2025
coverage: Treat `#[automatically_derived]` as `#[coverage(off)]` One of the contributing factors behind #141577 (comment) was the presence of derive-macro-generated code containing nested closures. Coverage instrumentation already has a heuristic for skipping code marked with `#[automatically_derived]` (#120185), because derived code is usually not worth instrumenting, and also has a tendency to trigger vexing edge-case bugs in coverage instrumentation or coverage codegen. However, the existing heuristic only applied to the associated items directly within an auto-derived impl block, and had no effect on closures or nested items within those associated items. This PR therefore extends the search for `#[coverage(..)]` attributes to also treat `#[automatically_derived]` as an implied `#[coverage(off)]` for the purposes of coverage instrumentation. --- This change doesn’t rule out an entire category of bugs, because it only affects code that actually uses the auto-derived attribute. But it should reduce the overall chance of edge-case macro span bugs being observed in the wild.
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Jul 29, 2025
…errors coverage: Treat `#[automatically_derived]` as `#[coverage(off)]` One of the contributing factors behind rust-lang#141577 (comment) was the presence of derive-macro-generated code containing nested closures. Coverage instrumentation already has a heuristic for skipping code marked with `#[automatically_derived]` (rust-lang#120185), because derived code is usually not worth instrumenting, and also has a tendency to trigger vexing edge-case bugs in coverage instrumentation or coverage codegen. However, the existing heuristic only applied to the associated items directly within an auto-derived impl block, and had no effect on closures or nested items within those associated items. This PR therefore extends the search for `#[coverage(..)]` attributes to also treat `#[automatically_derived]` as an implied `#[coverage(off)]` for the purposes of coverage instrumentation. --- This change doesn’t rule out an entire category of bugs, because it only affects code that actually uses the auto-derived attribute. But it should reduce the overall chance of edge-case macro span bugs being observed in the wild.
rust-timer
added a commit
that referenced
this pull request
Jul 29, 2025
Rollup merge of #144560 - Zalathar:auto-derived, r=compiler-errors coverage: Treat `#[automatically_derived]` as `#[coverage(off)]` One of the contributing factors behind #141577 (comment) was the presence of derive-macro-generated code containing nested closures. Coverage instrumentation already has a heuristic for skipping code marked with `#[automatically_derived]` (#120185), because derived code is usually not worth instrumenting, and also has a tendency to trigger vexing edge-case bugs in coverage instrumentation or coverage codegen. However, the existing heuristic only applied to the associated items directly within an auto-derived impl block, and had no effect on closures or nested items within those associated items. This PR therefore extends the search for `#[coverage(..)]` attributes to also treat `#[automatically_derived]` as an implied `#[coverage(off)]` for the purposes of coverage instrumentation. --- This change doesn’t rule out an entire category of bugs, because it only affects code that actually uses the auto-derived attribute. But it should reduce the overall chance of edge-case macro span bugs being observed in the wild.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-coverageis 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.