Skip to content

Commit e9df0b6

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of #116974 - Zalathar:signature-spans, r=oli-obk,cjgillot
coverage: Fix inconsistent handling of function signature spans While doing some more cleanup of `spans`, I noticed a strange inconsistency in how function signatures are handled. Normally the function signature span is treated as though it were executable as part of the start of the function, but in some cases the signature span disappears entirely from coverage, for no obvious reason. This is caused by the fact that spans created by `CoverageSpan::for_fn_sig` don't add the span to their `merged_spans` field (unlike normal statement/terminator spans). In cases where the span-processing code looks at those merged spans, it thinks the signature span is no longer visible and deletes it. Adding the signature span to `merged_spans` resolves the inconsistency. (Prior to #116409 this wouldn't have been possible, because there was no case in the old `CoverageStatement` enum representing a signature. Now that `merged_spans` is just a list of spans, that's no longer an obstacle.)
2 parents b703519 + 319693a commit e9df0b6

File tree

9 files changed

+233
-67
lines changed

9 files changed

+233
-67
lines changed

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

+4-29
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use std::cell::OnceCell;
22

33
use rustc_data_structures::graph::WithNumNodes;
44
use rustc_index::IndexVec;
5-
use rustc_middle::mir::{self, AggregateKind, Rvalue, Statement, StatementKind};
5+
use rustc_middle::mir;
66
use rustc_span::{BytePos, ExpnKind, MacroKind, Span, Symbol, DUMMY_SP};
77

88
use super::graph::{BasicCoverageBlock, CoverageGraph, START_BCB};
@@ -70,29 +70,15 @@ struct CoverageSpan {
7070

7171
impl CoverageSpan {
7272
pub fn for_fn_sig(fn_sig_span: Span) -> Self {
73-
Self {
74-
span: fn_sig_span,
75-
expn_span: fn_sig_span,
76-
current_macro_or_none: Default::default(),
77-
bcb: START_BCB,
78-
merged_spans: vec![],
79-
is_closure: false,
80-
}
73+
Self::new(fn_sig_span, fn_sig_span, START_BCB, false)
8174
}
8275

83-
pub fn for_statement(
84-
statement: &Statement<'_>,
76+
pub(super) fn new(
8577
span: Span,
8678
expn_span: Span,
8779
bcb: BasicCoverageBlock,
80+
is_closure: bool,
8881
) -> Self {
89-
let is_closure = match statement.kind {
90-
StatementKind::Assign(box (_, Rvalue::Aggregate(box ref kind, _))) => {
91-
matches!(kind, AggregateKind::Closure(_, _) | AggregateKind::Coroutine(_, _, _))
92-
}
93-
_ => false,
94-
};
95-
9682
Self {
9783
span,
9884
expn_span,
@@ -103,17 +89,6 @@ impl CoverageSpan {
10389
}
10490
}
10591

106-
pub fn for_terminator(span: Span, expn_span: Span, bcb: BasicCoverageBlock) -> Self {
107-
Self {
108-
span,
109-
expn_span,
110-
current_macro_or_none: Default::default(),
111-
bcb,
112-
merged_spans: vec![span],
113-
is_closure: false,
114-
}
115-
}
116-
11792
pub fn merge_from(&mut self, mut other: CoverageSpan) {
11893
debug_assert!(self.is_mergeable(&other));
11994
self.span = self.span.to(other.span);

‎compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

+37-28
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
use rustc_data_structures::captures::Captures;
12
use rustc_middle::mir::{
2-
self, FakeReadCause, Statement, StatementKind, Terminator, TerminatorKind,
3+
self, AggregateKind, FakeReadCause, Rvalue, Statement, StatementKind, Terminator,
4+
TerminatorKind,
35
};
46
use rustc_span::Span;
57

@@ -12,7 +14,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
1214
body_span: Span,
1315
basic_coverage_blocks: &CoverageGraph,
1416
) -> Vec<CoverageSpan> {
15-
let mut initial_spans = Vec::<CoverageSpan>::with_capacity(mir_body.basic_blocks.len() * 2);
17+
let mut initial_spans = Vec::with_capacity(mir_body.basic_blocks.len() * 2);
1618
for (bcb, bcb_data) in basic_coverage_blocks.iter_enumerated() {
1719
initial_spans.extend(bcb_to_initial_coverage_spans(mir_body, body_span, bcb, bcb_data));
1820
}
@@ -50,34 +52,41 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
5052
// for each `Statement` and `Terminator`. (Note that subsequent stages of coverage analysis will
5153
// merge some `CoverageSpan`s, at which point a `CoverageSpan` may represent multiple
5254
// `Statement`s and/or `Terminator`s.)
53-
fn bcb_to_initial_coverage_spans(
54-
mir_body: &mir::Body<'_>,
55+
fn bcb_to_initial_coverage_spans<'a, 'tcx>(
56+
mir_body: &'a mir::Body<'tcx>,
5557
body_span: Span,
5658
bcb: BasicCoverageBlock,
57-
bcb_data: &BasicCoverageBlockData,
58-
) -> Vec<CoverageSpan> {
59-
bcb_data
60-
.basic_blocks
61-
.iter()
62-
.flat_map(|&bb| {
63-
let data = &mir_body[bb];
64-
data.statements
65-
.iter()
66-
.filter_map(move |statement| {
67-
filtered_statement_span(statement).map(|span| {
68-
CoverageSpan::for_statement(
69-
statement,
70-
function_source_span(span, body_span),
71-
span,
72-
bcb,
73-
)
74-
})
75-
})
76-
.chain(filtered_terminator_span(data.terminator()).map(|span| {
77-
CoverageSpan::for_terminator(function_source_span(span, body_span), span, bcb)
78-
}))
79-
})
80-
.collect()
59+
bcb_data: &'a BasicCoverageBlockData,
60+
) -> impl Iterator<Item = CoverageSpan> + Captures<'a> + Captures<'tcx> {
61+
bcb_data.basic_blocks.iter().flat_map(move |&bb| {
62+
let data = &mir_body[bb];
63+
64+
let statement_spans = data.statements.iter().filter_map(move |statement| {
65+
let expn_span = filtered_statement_span(statement)?;
66+
let span = function_source_span(expn_span, body_span);
67+
68+
Some(CoverageSpan::new(span, expn_span, bcb, is_closure(statement)))
69+
});
70+
71+
let terminator_span = Some(data.terminator()).into_iter().filter_map(move |terminator| {
72+
let expn_span = filtered_terminator_span(terminator)?;
73+
let span = function_source_span(expn_span, body_span);
74+
75+
Some(CoverageSpan::new(span, expn_span, bcb, false))
76+
});
77+
78+
statement_spans.chain(terminator_span)
79+
})
80+
}
81+
82+
fn is_closure(statement: &Statement<'_>) -> bool {
83+
match statement.kind {
84+
StatementKind::Assign(box (_, Rvalue::Aggregate(box ref agg_kind, _))) => match agg_kind {
85+
AggregateKind::Closure(_, _) | AggregateKind::Coroutine(_, _, _) => true,
86+
_ => false,
87+
},
88+
_ => false,
89+
}
8190
}
8291

8392
/// If the MIR `Statement` has a span contributive to computing coverage spans,

‎tests/coverage-map/fn_sig_into_try.cov-map

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
Function name: fn_sig_into_try::a
2+
Raw bytes (9): 0x[01, 01, 00, 01, 01, 0a, 01, 04, 02]
3+
Number of files: 1
4+
- file 0 => global file 1
5+
Number of expressions: 0
6+
Number of file 0 mappings: 1
7+
- Code(Counter(0)) at (prev + 10, 1) to (start + 4, 2)
8+
9+
Function name: fn_sig_into_try::b
10+
Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 10, 01, 02, 0f, 00, 02, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
11+
Number of files: 1
12+
- file 0 => global file 1
13+
Number of expressions: 2
14+
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
15+
- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
16+
Number of file 0 mappings: 4
17+
- Code(Counter(0)) at (prev + 16, 1) to (start + 2, 15)
18+
- Code(Zero) at (prev + 2, 15) to (start + 0, 16)
19+
- Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
20+
= (c0 - c1)
21+
- Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
22+
= (c1 + (c0 - c1))
23+
24+
Function name: fn_sig_into_try::c
25+
Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 16, 01, 02, 17, 00, 02, 17, 00, 18, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
26+
Number of files: 1
27+
- file 0 => global file 1
28+
Number of expressions: 2
29+
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
30+
- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
31+
Number of file 0 mappings: 4
32+
- Code(Counter(0)) at (prev + 22, 1) to (start + 2, 23)
33+
- Code(Zero) at (prev + 2, 23) to (start + 0, 24)
34+
- Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
35+
= (c0 - c1)
36+
- Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
37+
= (c1 + (c0 - c1))
38+
39+
Function name: fn_sig_into_try::d
40+
Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 1c, 01, 03, 0f, 00, 03, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
41+
Number of files: 1
42+
- file 0 => global file 1
43+
Number of expressions: 2
44+
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
45+
- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
46+
Number of file 0 mappings: 4
47+
- Code(Counter(0)) at (prev + 28, 1) to (start + 3, 15)
48+
- Code(Zero) at (prev + 3, 15) to (start + 0, 16)
49+
- Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
50+
= (c0 - c1)
51+
- Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
52+
= (c1 + (c0 - c1))
53+

‎tests/coverage-map/fn_sig_into_try.rs

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
#![feature(coverage_attribute)]
2+
// compile-flags: --edition=2021
3+
4+
// Regression test for inconsistent handling of function signature spans that
5+
// are followed by code using the `?` operator.
6+
//
7+
// For each of these similar functions, the line containing the function
8+
// signature should be handled in the same way.
9+
10+
fn a() -> Option<i32>
11+
{
12+
Some(7i32);
13+
Some(0)
14+
}
15+
16+
fn b() -> Option<i32>
17+
{
18+
Some(7i32)?;
19+
Some(0)
20+
}
21+
22+
fn c() -> Option<i32>
23+
{
24+
let _ = Some(7i32)?;
25+
Some(0)
26+
}
27+
28+
fn d() -> Option<i32>
29+
{
30+
let _: () = ();
31+
Some(7i32)?;
32+
Some(0)
33+
}
34+
35+
#[coverage(off)]
36+
fn main() {
37+
a();
38+
b();
39+
c();
40+
d();
41+
}

‎tests/coverage-map/status-quo/inline-dead.cov-map

+8-6
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,15 @@ Number of file 0 mappings: 2
3131
- Code(Counter(0)) at (prev + 7, 6) to (start + 2, 2)
3232

3333
Function name: inline_dead::main::{closure#0}
34-
Raw bytes (16): 0x[01, 01, 01, 01, 05, 02, 00, 09, 0d, 00, 0e, 03, 02, 05, 00, 06]
34+
Raw bytes (23): 0x[01, 01, 02, 09, 06, 01, 05, 03, 01, 07, 17, 00, 18, 00, 02, 0d, 00, 0e, 03, 02, 05, 00, 06]
3535
Number of files: 1
3636
- file 0 => global file 1
37-
Number of expressions: 1
38-
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
39-
Number of file 0 mappings: 2
40-
- Code(Zero) at (prev + 9, 13) to (start + 0, 14)
37+
Number of expressions: 2
38+
- expression 0 operands: lhs = Counter(2), rhs = Expression(1, Sub)
39+
- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
40+
Number of file 0 mappings: 3
41+
- Code(Counter(0)) at (prev + 7, 23) to (start + 0, 24)
42+
- Code(Zero) at (prev + 2, 13) to (start + 0, 14)
4143
- Code(Expression(0, Add)) at (prev + 2, 5) to (start + 0, 6)
42-
= (c0 + c1)
44+
= (c2 + (c0 - c1))
4345

‎tests/coverage-map/status-quo/issue-84561.cov-map

+3-3
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,15 @@ Number of file 0 mappings: 1
77
- Code(Counter(0)) at (prev + 4, 10) to (start + 0, 19)
88

99
Function name: <issue_84561::Foo as core::fmt::Debug>::fmt
10-
Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 89, 01, 09, 00, 25, 05, 00, 25, 00, 26, 02, 01, 09, 00, 0f, 07, 01, 05, 00, 06]
10+
Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 88, 01, 05, 01, 25, 05, 01, 25, 00, 26, 02, 01, 09, 00, 0f, 07, 01, 05, 00, 06]
1111
Number of files: 1
1212
- file 0 => global file 1
1313
Number of expressions: 2
1414
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
1515
- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
1616
Number of file 0 mappings: 4
17-
- Code(Counter(0)) at (prev + 137, 9) to (start + 0, 37)
18-
- Code(Counter(1)) at (prev + 0, 37) to (start + 0, 38)
17+
- Code(Counter(0)) at (prev + 136, 5) to (start + 1, 37)
18+
- Code(Counter(1)) at (prev + 1, 37) to (start + 0, 38)
1919
- Code(Expression(0, Sub)) at (prev + 1, 9) to (start + 0, 15)
2020
= (c0 - c1)
2121
- Code(Expression(1, Add)) at (prev + 1, 5) to (start + 0, 6)

‎tests/run-coverage/fn_sig_into_try.coverage

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
LL| |#![feature(coverage_attribute)]
2+
LL| |// compile-flags: --edition=2021
3+
LL| |
4+
LL| |// Regression test for inconsistent handling of function signature spans that
5+
LL| |// are followed by code using the `?` operator.
6+
LL| |//
7+
LL| |// For each of these similar functions, the line containing the function
8+
LL| |// signature should be handled in the same way.
9+
LL| |
10+
LL| 1|fn a() -> Option<i32>
11+
LL| 1|{
12+
LL| 1| Some(7i32);
13+
LL| 1| Some(0)
14+
LL| 1|}
15+
LL| |
16+
LL| 1|fn b() -> Option<i32>
17+
LL| 1|{
18+
LL| 1| Some(7i32)?;
19+
^0
20+
LL| 1| Some(0)
21+
LL| 1|}
22+
LL| |
23+
LL| 1|fn c() -> Option<i32>
24+
LL| 1|{
25+
LL| 1| let _ = Some(7i32)?;
26+
^0
27+
LL| 1| Some(0)
28+
LL| 1|}
29+
LL| |
30+
LL| 1|fn d() -> Option<i32>
31+
LL| 1|{
32+
LL| 1| let _: () = ();
33+
LL| 1| Some(7i32)?;
34+
^0
35+
LL| 1| Some(0)
36+
LL| 1|}
37+
LL| |
38+
LL| |#[coverage(off)]
39+
LL| |fn main() {
40+
LL| | a();
41+
LL| | b();
42+
LL| | c();
43+
LL| | d();
44+
LL| |}
45+

‎tests/run-coverage/fn_sig_into_try.rs

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
#![feature(coverage_attribute)]
2+
// compile-flags: --edition=2021
3+
4+
// Regression test for inconsistent handling of function signature spans that
5+
// are followed by code using the `?` operator.
6+
//
7+
// For each of these similar functions, the line containing the function
8+
// signature should be handled in the same way.
9+
10+
fn a() -> Option<i32>
11+
{
12+
Some(7i32);
13+
Some(0)
14+
}
15+
16+
fn b() -> Option<i32>
17+
{
18+
Some(7i32)?;
19+
Some(0)
20+
}
21+
22+
fn c() -> Option<i32>
23+
{
24+
let _ = Some(7i32)?;
25+
Some(0)
26+
}
27+
28+
fn d() -> Option<i32>
29+
{
30+
let _: () = ();
31+
Some(7i32)?;
32+
Some(0)
33+
}
34+
35+
#[coverage(off)]
36+
fn main() {
37+
a();
38+
b();
39+
c();
40+
d();
41+
}

‎tests/run-coverage/issue-84561.coverage

+1-1
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@
135135
LL| 0|}
136136
LL| |
137137
LL| |impl std::fmt::Debug for Foo {
138-
LL| | fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
138+
LL| 7| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
139139
LL| 7| write!(f, "try and succeed")?;
140140
^0
141141
LL| 7| Ok(())

0 commit comments

Comments
 (0)