Skip to content

Commit f837028

Browse files
ZalatharZalathar
authored
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.
2 parents 7278554 + 682f744 commit f837028

File tree

10 files changed

+396
-42
lines changed

10 files changed

+396
-42
lines changed

‎compiler/rustc_attr_data_structures/src/attributes.rs

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -110,18 +110,10 @@ pub enum DeprecatedSince {
110110
Err,
111111
}
112112

113-
#[derive(
114-
Copy,
115-
Debug,
116-
Eq,
117-
PartialEq,
118-
Encodable,
119-
Decodable,
120-
Clone,
121-
HashStable_Generic,
122-
PrintAttribute
123-
)]
124-
pub enum CoverageStatus {
113+
/// Successfully-parsed value of a `#[coverage(..)]` attribute.
114+
#[derive(Copy, Debug, Eq, PartialEq, Encodable, Decodable, Clone)]
115+
#[derive(HashStable_Generic, PrintAttribute)]
116+
pub enum CoverageAttrKind {
125117
On,
126118
Off,
127119
}
@@ -304,8 +296,8 @@ pub enum AttributeKind {
304296
/// Represents `#[const_trait]`.
305297
ConstTrait(Span),
306298

307-
/// Represents `#[coverage]`.
308-
Coverage(Span, CoverageStatus),
299+
/// Represents `#[coverage(..)]`.
300+
Coverage(Span, CoverageAttrKind),
309301

310302
///Represents `#[rustc_deny_explicit_impl]`.
311303
DenyExplicitImpl(Span),

‎compiler/rustc_attr_parsing/src/attributes/codegen_attrs.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_attr_data_structures::{AttributeKind, CoverageStatus, OptimizeAttr, UsedBy};
1+
use rustc_attr_data_structures::{AttributeKind, CoverageAttrKind, OptimizeAttr, UsedBy};
22
use rustc_feature::{AttributeTemplate, template};
33
use rustc_session::parse::feature_err;
44
use rustc_span::{Span, Symbol, sym};
@@ -78,16 +78,16 @@ impl<S: Stage> SingleAttributeParser<S> for CoverageParser {
7878
return None;
7979
};
8080

81-
let status = match arg.path().word_sym() {
82-
Some(sym::off) => CoverageStatus::Off,
83-
Some(sym::on) => CoverageStatus::On,
81+
let kind = match arg.path().word_sym() {
82+
Some(sym::off) => CoverageAttrKind::Off,
83+
Some(sym::on) => CoverageAttrKind::On,
8484
None | Some(_) => {
8585
fail_incorrect_argument(arg.span());
8686
return None;
8787
}
8888
};
8989

90-
Some(AttributeKind::Coverage(cx.attr_span, status))
90+
Some(AttributeKind::Coverage(cx.attr_span, kind))
9191
}
9292
}
9393

‎compiler/rustc_mir_transform/src/coverage/query.rs

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_attr_data_structures::{AttributeKind, CoverageStatus, find_attr};
1+
use rustc_attr_data_structures::{AttributeKind, CoverageAttrKind, find_attr};
22
use rustc_index::bit_set::DenseBitSet;
33
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
44
use rustc_middle::mir::coverage::{BasicCoverageBlock, CoverageIdsInfo, CoverageKind, MappingKind};
@@ -32,16 +32,6 @@ fn is_eligible_for_coverage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
3232
return false;
3333
}
3434

35-
// Don't instrument functions with `#[automatically_derived]` on their
36-
// enclosing impl block, on the assumption that most users won't care about
37-
// coverage for derived impls.
38-
if let Some(impl_of) = tcx.impl_of_assoc(def_id.to_def_id())
39-
&& tcx.is_automatically_derived(impl_of)
40-
{
41-
trace!("InstrumentCoverage skipped for {def_id:?} (automatically derived)");
42-
return false;
43-
}
44-
4535
if tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NAKED) {
4636
trace!("InstrumentCoverage skipped for {def_id:?} (`#[naked]`)");
4737
return false;
@@ -57,20 +47,32 @@ fn is_eligible_for_coverage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
5747

5848
/// Query implementation for `coverage_attr_on`.
5949
fn coverage_attr_on(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
60-
// Check for annotations directly on this def.
61-
if let Some(coverage_status) =
62-
find_attr!(tcx.get_all_attrs(def_id), AttributeKind::Coverage(_, status) => status)
50+
// Check for a `#[coverage(..)]` attribute on this def.
51+
if let Some(kind) =
52+
find_attr!(tcx.get_all_attrs(def_id), AttributeKind::Coverage(_sp, kind) => kind)
6353
{
64-
*coverage_status == CoverageStatus::On
65-
} else {
66-
match tcx.opt_local_parent(def_id) {
67-
// Check the parent def (and so on recursively) until we find an
68-
// enclosing attribute or reach the crate root.
69-
Some(parent) => tcx.coverage_attr_on(parent),
70-
// We reached the crate root without seeing a coverage attribute, so
71-
// allow coverage instrumentation by default.
72-
None => true,
54+
match kind {
55+
CoverageAttrKind::On => return true,
56+
CoverageAttrKind::Off => return false,
7357
}
58+
};
59+
60+
// Treat `#[automatically_derived]` as an implied `#[coverage(off)]`, on
61+
// the assumption that most users won't want coverage for derived impls.
62+
//
63+
// This affects not just the associated items of an impl block, but also
64+
// any closures and other nested functions within those associated items.
65+
if tcx.is_automatically_derived(def_id.to_def_id()) {
66+
return false;
67+
}
68+
69+
// Check the parent def (and so on recursively) until we find an
70+
// enclosing attribute or reach the crate root.
71+
match tcx.opt_local_parent(def_id) {
72+
Some(parent) => tcx.coverage_attr_on(parent),
73+
// We reached the crate root without seeing a coverage attribute, so
74+
// allow coverage instrumentation by default.
75+
None => true,
7476
}
7577
}
7678

‎tests/coverage/auto-derived.auto.cov-map

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::inner_fn_on
2+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 1f, 09, 00, 19, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 23, 01, 01, 09, 00, 0a]
3+
Number of files: 1
4+
- file 0 => $DIR/auto-derived.rs
5+
Number of expressions: 0
6+
Number of file 0 mappings: 4
7+
- Code(Counter(0)) at (prev + 31, 9) to (start + 0, 25)
8+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
9+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 35)
10+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
11+
Highest counter ID seen: c0
12+
13+
Function name: auto_derived::main
14+
Raw bytes (19): 0x[01, 01, 00, 03, 01, 33, 01, 00, 0a, 01, 01, 05, 00, 1a, 01, 01, 01, 00, 02]
15+
Number of files: 1
16+
- file 0 => $DIR/auto-derived.rs
17+
Number of expressions: 0
18+
Number of file 0 mappings: 3
19+
- Code(Counter(0)) at (prev + 51, 1) to (start + 0, 10)
20+
- Code(Counter(0)) at (prev + 1, 5) to (start + 0, 26)
21+
- Code(Counter(0)) at (prev + 1, 1) to (start + 0, 2)
22+
Highest counter ID seen: c0
23+

‎tests/coverage/auto-derived.auto.coverage

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
LL| |#![feature(coverage_attribute)]
2+
LL| |//@ edition: 2024
3+
LL| |//@ revisions: base auto on
4+
LL| |
5+
LL| |// Tests for how `#[automatically_derived]` affects coverage instrumentation.
6+
LL| |//
7+
LL| |// The actual behaviour is an implementation detail, so this test mostly exists
8+
LL| |// to show when that behaviour has been accidentally or deliberately changed.
9+
LL| |//
10+
LL| |// Revision guide:
11+
LL| |// - base: Test baseline instrumentation behaviour without `#[automatically_derived]`
12+
LL| |// - auto: Test how `#[automatically_derived]` affects instrumentation
13+
LL| |// - on: Test interaction between auto-derived and `#[coverage(on)]`
14+
LL| |
15+
LL| |struct MyStruct;
16+
LL| |
17+
LL| |trait MyTrait {
18+
LL| | fn my_assoc_fn();
19+
LL| |}
20+
LL| |
21+
LL| |#[cfg_attr(auto, automatically_derived)]
22+
LL| |#[cfg_attr(on, automatically_derived)]
23+
LL| |#[cfg_attr(on, coverage(on))]
24+
LL| |impl MyTrait for MyStruct {
25+
LL| | fn my_assoc_fn() {
26+
LL| | fn inner_fn() {
27+
LL| | say("in inner fn");
28+
LL| | }
29+
LL| |
30+
LL| | #[coverage(on)]
31+
LL| 1| fn inner_fn_on() {
32+
LL| 1| say("in inner fn (on)");
33+
LL| 1| }
34+
LL| |
35+
LL| | let closure = || {
36+
LL| | say("in closure");
37+
LL| | };
38+
LL| |
39+
LL| | closure();
40+
LL| | inner_fn();
41+
LL| | inner_fn_on();
42+
LL| | }
43+
LL| |}
44+
LL| |
45+
LL| |#[coverage(off)]
46+
LL| |#[inline(never)]
47+
LL| |fn say(s: &str) {
48+
LL| | println!("{s}");
49+
LL| |}
50+
LL| |
51+
LL| 1|fn main() {
52+
LL| 1| MyStruct::my_assoc_fn();
53+
LL| 1|}
54+

‎tests/coverage/auto-derived.base.cov-map

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn
2+
Raw bytes (34): 0x[01, 01, 00, 06, 01, 19, 05, 00, 15, 01, 0a, 0d, 00, 14, 01, 04, 09, 00, 12, 01, 01, 09, 00, 11, 01, 01, 09, 00, 14, 01, 01, 05, 00, 06]
3+
Number of files: 1
4+
- file 0 => $DIR/auto-derived.rs
5+
Number of expressions: 0
6+
Number of file 0 mappings: 6
7+
- Code(Counter(0)) at (prev + 25, 5) to (start + 0, 21)
8+
- Code(Counter(0)) at (prev + 10, 13) to (start + 0, 20)
9+
- Code(Counter(0)) at (prev + 4, 9) to (start + 0, 18)
10+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 17)
11+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 20)
12+
- Code(Counter(0)) at (prev + 1, 5) to (start + 0, 6)
13+
Highest counter ID seen: c0
14+
15+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::inner_fn
16+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 1a, 09, 00, 16, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 1e, 01, 01, 09, 00, 0a]
17+
Number of files: 1
18+
- file 0 => $DIR/auto-derived.rs
19+
Number of expressions: 0
20+
Number of file 0 mappings: 4
21+
- Code(Counter(0)) at (prev + 26, 9) to (start + 0, 22)
22+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
23+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 30)
24+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
25+
Highest counter ID seen: c0
26+
27+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::inner_fn_on
28+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 1f, 09, 00, 19, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 23, 01, 01, 09, 00, 0a]
29+
Number of files: 1
30+
- file 0 => $DIR/auto-derived.rs
31+
Number of expressions: 0
32+
Number of file 0 mappings: 4
33+
- Code(Counter(0)) at (prev + 31, 9) to (start + 0, 25)
34+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
35+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 35)
36+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
37+
Highest counter ID seen: c0
38+
39+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::{closure#0}
40+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 23, 1a, 00, 1b, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 1d, 01, 01, 09, 00, 0a]
41+
Number of files: 1
42+
- file 0 => $DIR/auto-derived.rs
43+
Number of expressions: 0
44+
Number of file 0 mappings: 4
45+
- Code(Counter(0)) at (prev + 35, 26) to (start + 0, 27)
46+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
47+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 29)
48+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
49+
Highest counter ID seen: c0
50+
51+
Function name: auto_derived::main
52+
Raw bytes (19): 0x[01, 01, 00, 03, 01, 33, 01, 00, 0a, 01, 01, 05, 00, 1a, 01, 01, 01, 00, 02]
53+
Number of files: 1
54+
- file 0 => $DIR/auto-derived.rs
55+
Number of expressions: 0
56+
Number of file 0 mappings: 3
57+
- Code(Counter(0)) at (prev + 51, 1) to (start + 0, 10)
58+
- Code(Counter(0)) at (prev + 1, 5) to (start + 0, 26)
59+
- Code(Counter(0)) at (prev + 1, 1) to (start + 0, 2)
60+
Highest counter ID seen: c0
61+

‎tests/coverage/auto-derived.base.coverage

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
LL| |#![feature(coverage_attribute)]
2+
LL| |//@ edition: 2024
3+
LL| |//@ revisions: base auto on
4+
LL| |
5+
LL| |// Tests for how `#[automatically_derived]` affects coverage instrumentation.
6+
LL| |//
7+
LL| |// The actual behaviour is an implementation detail, so this test mostly exists
8+
LL| |// to show when that behaviour has been accidentally or deliberately changed.
9+
LL| |//
10+
LL| |// Revision guide:
11+
LL| |// - base: Test baseline instrumentation behaviour without `#[automatically_derived]`
12+
LL| |// - auto: Test how `#[automatically_derived]` affects instrumentation
13+
LL| |// - on: Test interaction between auto-derived and `#[coverage(on)]`
14+
LL| |
15+
LL| |struct MyStruct;
16+
LL| |
17+
LL| |trait MyTrait {
18+
LL| | fn my_assoc_fn();
19+
LL| |}
20+
LL| |
21+
LL| |#[cfg_attr(auto, automatically_derived)]
22+
LL| |#[cfg_attr(on, automatically_derived)]
23+
LL| |#[cfg_attr(on, coverage(on))]
24+
LL| |impl MyTrait for MyStruct {
25+
LL| 1| fn my_assoc_fn() {
26+
LL| 1| fn inner_fn() {
27+
LL| 1| say("in inner fn");
28+
LL| 1| }
29+
LL| |
30+
LL| | #[coverage(on)]
31+
LL| 1| fn inner_fn_on() {
32+
LL| 1| say("in inner fn (on)");
33+
LL| 1| }
34+
LL| |
35+
LL| 1| let closure = || {
36+
LL| 1| say("in closure");
37+
LL| 1| };
38+
LL| |
39+
LL| 1| closure();
40+
LL| 1| inner_fn();
41+
LL| 1| inner_fn_on();
42+
LL| 1| }
43+
LL| |}
44+
LL| |
45+
LL| |#[coverage(off)]
46+
LL| |#[inline(never)]
47+
LL| |fn say(s: &str) {
48+
LL| | println!("{s}");
49+
LL| |}
50+
LL| |
51+
LL| 1|fn main() {
52+
LL| 1| MyStruct::my_assoc_fn();
53+
LL| 1|}
54+

‎tests/coverage/auto-derived.on.cov-map

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn
2+
Raw bytes (34): 0x[01, 01, 00, 06, 01, 19, 05, 00, 15, 01, 0a, 0d, 00, 14, 01, 04, 09, 00, 12, 01, 01, 09, 00, 11, 01, 01, 09, 00, 14, 01, 01, 05, 00, 06]
3+
Number of files: 1
4+
- file 0 => $DIR/auto-derived.rs
5+
Number of expressions: 0
6+
Number of file 0 mappings: 6
7+
- Code(Counter(0)) at (prev + 25, 5) to (start + 0, 21)
8+
- Code(Counter(0)) at (prev + 10, 13) to (start + 0, 20)
9+
- Code(Counter(0)) at (prev + 4, 9) to (start + 0, 18)
10+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 17)
11+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 20)
12+
- Code(Counter(0)) at (prev + 1, 5) to (start + 0, 6)
13+
Highest counter ID seen: c0
14+
15+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::inner_fn
16+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 1a, 09, 00, 16, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 1e, 01, 01, 09, 00, 0a]
17+
Number of files: 1
18+
- file 0 => $DIR/auto-derived.rs
19+
Number of expressions: 0
20+
Number of file 0 mappings: 4
21+
- Code(Counter(0)) at (prev + 26, 9) to (start + 0, 22)
22+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
23+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 30)
24+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
25+
Highest counter ID seen: c0
26+
27+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::inner_fn_on
28+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 1f, 09, 00, 19, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 23, 01, 01, 09, 00, 0a]
29+
Number of files: 1
30+
- file 0 => $DIR/auto-derived.rs
31+
Number of expressions: 0
32+
Number of file 0 mappings: 4
33+
- Code(Counter(0)) at (prev + 31, 9) to (start + 0, 25)
34+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
35+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 35)
36+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
37+
Highest counter ID seen: c0
38+
39+
Function name: <auto_derived::MyStruct as auto_derived::MyTrait>::my_assoc_fn::{closure#0}
40+
Raw bytes (24): 0x[01, 01, 00, 04, 01, 23, 1a, 00, 1b, 01, 01, 0d, 00, 10, 01, 00, 11, 00, 1d, 01, 01, 09, 00, 0a]
41+
Number of files: 1
42+
- file 0 => $DIR/auto-derived.rs
43+
Number of expressions: 0
44+
Number of file 0 mappings: 4
45+
- Code(Counter(0)) at (prev + 35, 26) to (start + 0, 27)
46+
- Code(Counter(0)) at (prev + 1, 13) to (start + 0, 16)
47+
- Code(Counter(0)) at (prev + 0, 17) to (start + 0, 29)
48+
- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 10)
49+
Highest counter ID seen: c0
50+
51+
Function name: auto_derived::main
52+
Raw bytes (19): 0x[01, 01, 00, 03, 01, 33, 01, 00, 0a, 01, 01, 05, 00, 1a, 01, 01, 01, 00, 02]
53+
Number of files: 1
54+
- file 0 => $DIR/auto-derived.rs
55+
Number of expressions: 0
56+
Number of file 0 mappings: 3
57+
- Code(Counter(0)) at (prev + 51, 1) to (start + 0, 10)
58+
- Code(Counter(0)) at (prev + 1, 5) to (start + 0, 26)
59+
- Code(Counter(0)) at (prev + 1, 1) to (start + 0, 2)
60+
Highest counter ID seen: c0
61+

0 commit comments

Comments
 (0)