Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverage instruments closure bodies in macros (not the macro body) #84897

Merged
merged 1 commit into from
May 7, 2021

Conversation

richkadel
Copy link
Contributor

Fixes: #84884

This solution might be considered a compromise, but I think it is the
better choice.

The results in the closure.rs test correctly resolve all test cases
broken as described in #84884.

One test pattern (in both closure_macro.rs and
closure_macro_async.rs) was also affected, and removes coverage
statistics for the lines inside the closure, because the closure
includes a macro. (The coverage remains at the callsite of the macro, so
we lose some detail, but there isn't a perfect choice with macros.

Often macro implementations are split across the macro and the callsite,
and there doesn't appear to be a single "right choice" for which body
should be covered. For the current implementation, we can't do both.

The callsite is most likely to be the preferred site for coverage.

r? @tmandry
cc: @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2021
@richkadel
Copy link
Contributor Author

@tmandry - This is also just 1 commit, but shows three because it depends on #84797 (due to the test relocation to run-make.

@richkadel
Copy link
Contributor Author

I do have a new idea to explore, but still believe this PR is the right intermediate solution.

So, just FYI, in a future PR I think this may work:

When collecting spans in spans.rs, I currently only add spans that are within the body_span. If a Statement/Terminator span is an expansion span, I use original_sp() to try to find a pre-expansion span (for example, a span within the macro call itself) if that span is in the body span. (If not, it just returns the body span, which generally just makes the span irrelevant). And this works well for coverage within the function body, wherever the body starts from (which can be in a macro).

I think I can improve on this by returning ALL spans for every statement/terminator, that is, return the given span, it's parent span if any, and all other ancestor spans.

Then when sorting the initial_spans, I will include the ctxt in the sort criteria (to keep them separate), and I would have to adjust span comparisons to take ctxt into account (I should only compare span.lo() or span.hi() when two spans are in the same ctxt).

With that change, I should be able to still add all resolved (selected and/or merged) spans to the same refined_spans collection and add the Coverage statements as before.

Some debugging features (e.g., spanview) may assume all spans are in the same file and contiguous function block, so there will probably be some cleanup needed to support this.

I think the coverage map generation code might also assume all spans for a single function are associated with a single file, and if so, I'll need to make some adjustments to support multiple files for a given function.

But generally this seems doable, and seems like we would get even more coverage of macros, plus coverage of macro callers if the body starts in a macro. (It shouldn't matter anymore either way.)

Fixes: rust-lang#84884

This solution might be considered a compromise, but I think it is the
better choice.

The results in the `closure.rs` test correctly resolve all test cases
broken as described in rust-lang#84884.

One test pattern (in both `closure_macro.rs` and
`closure_macro_async.rs`) was also affected, and removes coverage
statistics for the lines inside the closure, because the closure
includes a macro. (The coverage remains at the callsite of the macro, so
we lose some detail, but there isn't a perfect choice with macros.

Often macro implementations are split across the macro and the callsite,
and there doesn't appear to be a single "right choice" for which body
should be covered. For the current implementation, we can't do both.

The callsite is most likely to be the preferred site for coverage.

I applied this fix to all `MacroKinds`, not just `Bang`.

I'm trying to resolve an issue of lost coverage in a
`MacroKind::Attr`-based, function-scoped macro. Instead of only
searching for a body_span that is "not a function-like macro" (that is,
MacroKind::Bang), I'm expanding this to all `MacroKind`s. Maybe I should
expand this to `ExpnKind::Desugaring` and `ExpnKind::AstPass` (or
subsets, depending on their sub-kinds) as well, but I'm not sure that's
a good idea.

I'd like to add a test of the `Attr` macro on functions, but I need time
to figure out how to constract a good, simple example without external
crate dependencies. For the moment, all tests still work as expected (no
change), this new commit shouldn't have a negative affect, and more
importantly, I believe it will have a positive effect. I will try to
confirm this.
@tmandry
Copy link
Member

tmandry commented May 6, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2021

📌 Commit cb70221 has been approved by tmandry

@bors 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 May 6, 2021
@tmandry
Copy link
Member

tmandry commented May 6, 2021

@bors rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#84779 (Add support for --test-args to cargotest)
 - rust-lang#84781 (Don't check bootstrap artifacts by default)
 - rust-lang#84787 (bump deps)
 - rust-lang#84815 (Update coverage docs and command line help)
 - rust-lang#84875 (Removes unneeded check of `#[no_coverage]` in mapgen)
 - rust-lang#84897 (Coverage instruments closure bodies in macros (not the macro body))
 - rust-lang#84911 (Retry clang+llvm download)
 - rust-lang#84972 (CTFE inbounds-error-messages tweak)
 - rust-lang#84990 (Sort rustdoc-gui tests)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 343a094 into rust-lang:master May 7, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-Z instrument-coverage shows missing coverage for unbraced closures invoking only a macro
5 participants