coverage of async function bodies should match non-async #84323

richkadel · 2021-04-18T22:09:53Z

This fixes some missing coverage within async function bodies.

Commit 1 demonstrates the problem in the fixed issue, and commit 2 corrects it.

Fixes: #83985

The initial commit demonstrates the issue, but the fix is not yet implemented. Once corrected... Fixes: rust-lang#83985

rust-highfive · 2021-04-18T22:09:56Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

richkadel · 2021-04-18T22:10:35Z

r? @tmandry
cc: @wesleywiser

The body_span was assumed to be in the Span root context, but this was not the case for async function bodies.

richkadel · 2021-04-18T23:31:03Z

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.partial_eq.txt

@@ -2,7 +2,7 @@
    2|       |// structure of this test.
    3|       |
    4|      2|#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
-                       ^0            ^0      ^0 ^0  ^1       ^1 ^0^0


Note that this fix also resolves the mystery (by removing them) of the double counters with some derived traits.

Well, I have to take this back. After reviewing the implementation of original_sp(), I believe preemptively forcing the body to the root context would have changed the behavior of original_sp() on other source spans in a lossy way. Dropping the "extra" coverage counters here was a happy side effect of what was probably not a good change, and my new commit no longer has that side effect, but is probably more correct.

richkadel · 2021-04-18T23:32:24Z

src/test/run-make-fulldeps/coverage/async.rs

@@ -1,6 +1,6 @@
 #![allow(unused_assignments, dead_code)]

-// compile-flags: --edition=2018 -C opt-level=1 # fix in rustc_mir/monomorphize/partitioning/mod.rs


This comment is no longer relevant.

wesleywiser

LGTM but I'll let Tyler review as well

Aaron1011 · 2021-04-19T17:12:33Z

compiler/rustc_mir/src/transform/coverage/spans.rs

@@ -246,8 +246,8 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
    ) -> Vec<CoverageSpan> {
        let mut coverage_spans = CoverageSpans {
            mir_body,
-            fn_sig_span,
-            body_span,
+            fn_sig_span: fn_sig_span.with_ctxt(SyntaxContext::root()),


It doesn't look like the SyntaxContext is adjusted in any other places (e.g. in bcb_to_initial_coverage_spans). Is there something special about async desugarings that makes this necessary? What happens if the body of a function is generated from a macro (and therefore has a non-root SyntaxContext)?

See function_source_span(), which does adjust the SyntaxtContext for all other spans.

All of the function body spans are extrapolated to their rustc_span::source_map::original_sp.

They are assumed to then be in the context of the body.

The bug, fixed by this PR, was that I assumed the body would always be in the root. Now all contexts are normalized to root.

This allows me to use Span::to(), which, otherwise, doesn't always return the expected result, if the contexts are different (even though they are in the same source).

The end result (in tests and in many practical use cases) provide the evidence that this approach works, except for the bug fixed by this PR, which is now obvious.

I'm taking a closer look at this though, and trying an alternative that may be relevant to the concern you raised.

I just uploaded a modified solution (see the new commit). This still forces all spans (fn_sig, body, and the MIR sources) to the same ctxt, so Span::to() should still work, but it does not force everything to the root ctxt anymore. Instead, the body_span's ctxt is now the common ctxt.

In the previous commit, I normalized the body_span's ctxt to the root, but that would have changed the behavior or original_sp, when using the body_span as the enclosing span. That was the wrong change, and I'm glad to have had the opportunity to review this.

richkadel · 2021-04-19T20:08:56Z

Overall, with this last commit, this is definitely a net-positive fix, especially for macro expansion. Before this PR, I see anomalies like entire macro sources shown with all lines counted (including unexecutable lines), with the same count. With this PR, only the executable lines are counted, even for functions defined within macros. So the bad data is gone, but the good data is still retained.

For another example, you can see how many lines that were not counted and should have been are not correctly counted. This is due to the same issue that appeared in async functions, prompting this PR:

Before this PR:

  677|       |
  678|       |            /// Inserts the specified flags in-place.
  679|       |            #[inline]
  ------------------
  | <clap::app::settings::Flags>::insert:
  |  680|      3|            pub fn insert(&mut self, other: $BitFlags) {
  ------------------
  681|       |                self.bits |= other.bits;
  682|       |            }
  683|       |
  684|       |            /// Removes the specified flags in-place.
  685|       |            #[inline]
  686|     11|            pub fn remove(&mut self, other: $BitFlags) {
  ------------------
  | <clap::app::settings::Flags>::remove:
  |  686|      1|            pub fn remove(&mut self, other: $BitFlags) {
  ------------------
  | <clap::args::settings::Flags>::remove:
  |  686|     10|            pub fn remove(&mut self, other: $BitFlags) {
  ------------------
  687|       |                self.bits &= !other.bits;
  688|       |            }

With this PR, the result is now much more accurate:

  677|       |
  678|       |            /// Inserts the specified flags in-place.
  679|       |            #[inline]
  680|      7|            pub fn insert(&mut self, other: $BitFlags) {
  681|      7|                self.bits |= other.bits;
  682|      7|            }
  ------------------
  | <clap::args::settings::Flags>::insert:
  |  680|      4|            pub fn insert(&mut self, other: $BitFlags) {
  |  681|      4|                self.bits |= other.bits;
  |  682|      4|            }
  ------------------
  | <clap::app::settings::Flags>::insert:
  |  680|      3|            pub fn insert(&mut self, other: $BitFlags) {
  |  681|      3|                self.bits |= other.bits;
  |  682|      3|            }
  ------------------
  683|       |
  684|       |            /// Removes the specified flags in-place.
  685|       |            #[inline]
  686|     11|            pub fn remove(&mut self, other: $BitFlags) {
  687|     11|                self.bits &= !other.bits;
  688|     11|            }
  ------------------
  | <clap::args::settings::Flags>::remove:
  |  686|     10|            pub fn remove(&mut self, other: $BitFlags) {
  |  687|     10|                self.bits &= !other.bits;
  |  688|     10|            }
  ------------------
  | <clap::app::settings::Flags>::remove:
  |  686|      1|            pub fn remove(&mut self, other: $BitFlags) {
  |  687|      1|                self.bits &= !other.bits;
  |  688|      1|            }

tmandry · 2021-04-20T00:32:48Z

This looks great!

@bors r+

bors · 2021-04-20T00:32:49Z

📌 Commit 5d8d67f has been approved by tmandry

bors · 2021-04-20T08:33:56Z

⌛ Testing commit 5d8d67f with merge 6af1e63...

bors · 2021-04-20T11:19:26Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 6af1e63 to master...

sdroege · 2021-04-22T07:18:56Z

This seems to cause an ICE: #84421

DRAFT: coverage of async function bodies should match non-async

3ece606

The initial commit demonstrates the issue, but the fix is not yet implemented. Once corrected... Fixes: rust-lang#83985

rust-highfive assigned Mark-Simulacrum Apr 18, 2021

rust-highfive added the S-waiting-on-review label Apr 18, 2021

rust-highfive assigned tmandry and unassigned Mark-Simulacrum Apr 18, 2021

richkadel marked this pull request as draft April 18, 2021 22:11

Fixes the issue with uncovered source in async function bodies

1893721

The body_span was assumed to be in the Span root context, but this was not the case for async function bodies.

richkadel commented Apr 18, 2021

View reviewed changes

richkadel changed the title ~~DRAFT: coverage of async function bodies should match non-async~~ coverage of async function bodies should match non-async Apr 18, 2021

richkadel marked this pull request as ready for review April 18, 2021 23:33

wesleywiser approved these changes Apr 19, 2021

View reviewed changes

richkadel mentioned this pull request Apr 19, 2021

Source-code based coverage is failing to identify some executable lines #83985

Closed

Aaron1011 reviewed Apr 19, 2021

View reviewed changes

compute fn_sig span from body call_site, and use body ctxt, not root

5d8d67f

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 20, 2021

bors added the merged-by-bors label Apr 20, 2021

bors merged commit 6af1e63 into rust-lang:master Apr 20, 2021

rustbot added this to the 1.53.0 milestone Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coverage of async function bodies should match non-async #84323

coverage of async function bodies should match non-async #84323

richkadel commented Apr 18, 2021 •

edited

Loading

rust-highfive commented Apr 18, 2021

richkadel commented Apr 18, 2021

richkadel Apr 18, 2021

richkadel Apr 19, 2021

richkadel Apr 18, 2021

wesleywiser left a comment

Aaron1011 Apr 19, 2021

richkadel Apr 19, 2021

richkadel Apr 19, 2021

richkadel Apr 19, 2021

richkadel commented Apr 19, 2021

tmandry commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

sdroege commented Apr 22, 2021

		@@ -1,6 +1,6 @@
		#![allow(unused_assignments, dead_code)]

		// compile-flags: --edition=2018 -C opt-level=1 # fix in rustc_mir/monomorphize/partitioning/mod.rs

coverage of async function bodies should match non-async #84323

coverage of async function bodies should match non-async #84323

Conversation

richkadel commented Apr 18, 2021 • edited Loading

rust-highfive commented Apr 18, 2021

richkadel commented Apr 18, 2021

richkadel Apr 18, 2021

Choose a reason for hiding this comment

richkadel Apr 19, 2021

Choose a reason for hiding this comment

richkadel Apr 18, 2021

Choose a reason for hiding this comment

wesleywiser left a comment

Choose a reason for hiding this comment

Aaron1011 Apr 19, 2021

Choose a reason for hiding this comment

richkadel Apr 19, 2021

Choose a reason for hiding this comment

richkadel Apr 19, 2021

Choose a reason for hiding this comment

richkadel Apr 19, 2021

Choose a reason for hiding this comment

richkadel commented Apr 19, 2021

tmandry commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

sdroege commented Apr 22, 2021

richkadel commented Apr 18, 2021 •

edited

Loading