coverage: Fix inconsistent handling of function signature spans #116974

Zalathar · 2023-10-20T12:49:27Z

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.)

rustbot · 2023-10-20T12:49:36Z

r? @cjgillot

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

rustbot · 2023-10-20T12:49:39Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2023-10-20T12:50:08Z

@rustbot label +A-code-coverage

Zalathar · 2023-10-20T12:56:23Z

tests/run-coverage/fn_sig_into_try.coverage

+   LL|      1|fn a() -> Option<i32>
+   LL|      1|{
+   LL|      1|    Some(7i32);
+   LL|      1|    Some(0)
+   LL|      1|}
+   LL|       |
+   LL|       |fn b() -> Option<i32>
+   LL|       |{
+   LL|      1|    Some(7i32)?;
+                            ^0
+   LL|      1|    Some(0)
+   LL|      1|}
+   LL|       |
+   LL|       |fn c() -> Option<i32>
+   LL|       |{
+   LL|      1|    let _ = Some(7i32)?;
+                                    ^0
+   LL|      1|    Some(0)
+   LL|      1|}
+   LL|       |
+   LL|      1|fn d() -> Option<i32>
+   LL|      1|{
+   LL|      1|    let _: () = ();
+   LL|      1|    Some(7i32)?;
+                            ^0
+   LL|      1|    Some(0)
+   LL|      1|}


This shows the current inconsistency. All four functions should have the same coverage for their signatures, but instead b and c treat them as not executable, due to the indirect influence of the nearby ?.

oli-obk · 2023-10-20T15:41:28Z

@bors r+ rollup

r? @oli-obk

bors · 2023-10-20T15:41:31Z

📌 Commit 2a191e2 has been approved by oli-obk

It is now in the queue for this repository.

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 rust-lang#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.)

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#116911 (Suggest relaxing implicit `type Assoc: Sized;` bound) - rust-lang#116961 (Typo suggestion to change bindings with leading underscore) - rust-lang#116964 (Add stable Instance::body() and RustcInternal trait) - rust-lang#116992 (Mention the syntax for `use` on `mod foo;` if `foo` doesn't exist) - rust-lang#116995 (Point at assoc fn definition on type param divergence) Failed merges: - rust-lang#116974 (coverage: Fix inconsistent handling of function signature spans) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2023-10-21T00:13:13Z

☔ The latest upstream changes (presumably #116958) made this pull request unmergeable. Please resolve the merge conflicts.

…gnatures

Zalathar · 2023-10-21T00:57:48Z

Resolved conflict with generator/coroutine rename.

Zalathar · 2023-10-21T05:16:16Z

@rustbot ready

cjgillot · 2023-10-21T06:51:21Z

@bors r=oli-obk,cjgillot

bors · 2023-10-21T06:51:23Z

📌 Commit 319693a has been approved by oli-obk,cjgillot

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#114521 (std: freebsd build update.) - rust-lang#116911 (Suggest relaxing implicit `type Assoc: Sized;` bound) - rust-lang#116917 (coverage: Simplify the injection of coverage statements) - rust-lang#116961 (Typo suggestion to change bindings with leading underscore) - rust-lang#116964 (Add stable Instance::body() and RustcInternal trait) - rust-lang#116974 (coverage: Fix inconsistent handling of function signature spans) - rust-lang#116990 (Mention `into_iter` on borrow errors suggestions when appropriate) - rust-lang#116995 (Point at assoc fn definition on type param divergence) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#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 rust-lang#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.)

rustbot assigned cjgillot Oct 20, 2023

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 Oct 20, 2023

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Oct 20, 2023

Zalathar commented Oct 20, 2023

View reviewed changes

rustbot assigned oli-obk Oct 20, 2023

rustbot unassigned cjgillot Oct 20, 2023

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 Oct 20, 2023

matthiaskrgr mentioned this pull request Oct 20, 2023

Rollup of 4 pull requests #116994

Closed

matthiaskrgr mentioned this pull request Oct 20, 2023

Rollup of 5 pull requests #117002

Closed

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 Oct 21, 2023

Zalathar added 4 commits October 21, 2023 11:53

coverage: Add a test showing the inconsistent handling of function si…

dca4295

…gnatures

coverage: Handle fn signature spans more consistently near ?

a17ff82

coverage: Don't create an intermediate vec for each BCB's initial spans

e164944

coverage: Simplify initial creation of coverage spans

319693a

Zalathar force-pushed the signature-spans branch from 2a191e2 to 319693a Compare October 21, 2023 00:57

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 Oct 21, 2023

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 Oct 21, 2023

matthiaskrgr mentioned this pull request Oct 21, 2023

Rollup of 8 pull requests #117013

Merged

bors merged commit e9df0b6 into rust-lang:master Oct 21, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 21, 2023

Zalathar deleted the signature-spans branch October 21, 2023 11:05

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage: Fix inconsistent handling of function signature spans #116974

coverage: Fix inconsistent handling of function signature spans #116974

Zalathar commented Oct 20, 2023 •

edited

Loading

rustbot commented Oct 20, 2023

rustbot commented Oct 20, 2023

Zalathar commented Oct 20, 2023

Zalathar Oct 20, 2023

oli-obk commented Oct 20, 2023

bors commented Oct 20, 2023

bors commented Oct 21, 2023

Zalathar commented Oct 21, 2023

Zalathar commented Oct 21, 2023

cjgillot commented Oct 21, 2023

bors commented Oct 21, 2023

coverage: Fix inconsistent handling of function signature spans #116974

coverage: Fix inconsistent handling of function signature spans #116974

Conversation

Zalathar commented Oct 20, 2023 • edited Loading

rustbot commented Oct 20, 2023

rustbot commented Oct 20, 2023

Zalathar commented Oct 20, 2023

Zalathar Oct 20, 2023

Choose a reason for hiding this comment

oli-obk commented Oct 20, 2023

bors commented Oct 20, 2023

bors commented Oct 21, 2023

Zalathar commented Oct 21, 2023

Zalathar commented Oct 21, 2023

cjgillot commented Oct 21, 2023

bors commented Oct 21, 2023

Zalathar commented Oct 20, 2023 •

edited

Loading