xd009642 / tarpaulin

A code coverage tool for Rust projects
https://crates.io/crates/cargo-tarpaulin
Apache License 2.0
2.5k stars 180 forks source link

Sequences of statements that end in unreachable() are excluded. #833

Closed snue closed 3 years ago

snue commented 3 years ago

Describe the bug Tarpaulin tries to be smart about lines excluded from coverage numbers. Unfortunately this assumption here about sequences of statements is wrong:

        // in a list of statements, if any of them is unreachable, the whole list is
        // unreachable

Actually, each of the prior statements in the list may return prematurely. These can be arbitrarily complex constructs. I encountered this in a function that does a lot of checks according to a manual, and puts unreachable() afterwards to express that all cases shall be handled. This prevents me from seeing whether all the cases in the function were exercised by tests.

To Reproduce Here is a pretty minimal reproducer. The whole function will be excluded from coverage.

fn excluded_from_coverage(option: bool) -> bool {
  if option {
    return true;
  }

  if !option {
    return false;
  }

  unreachable!();
}

Please note: There is actually a unit test that asserts this (imho wrong) behavior.

To fix this correctly, you probably need to distinguish statements that can return.

Expected behavior The unreachable() line itself may be excluded from coverage, but lines preceding it are valid code sequences that should receive coverage annotation.

This is related to the common #351 ticket, but I decided to open a separate ticket as this one has a pretty straight forward explanation for the root cause.

xd009642 commented 3 years ago

Ah good catch!, So the unit test is correct as there's no early return. But I felt early return should just abort any form of reachability analysis. So I've done a PR to fix this (also taking into account try blocks and the try operator). If you want to check that to see if it sorts your issue that would be helpful (assuming your real code might have more edge cases I haven't thought of that aren't in the MRE). But I'm confident it should so I'll merge it late tomorrow or Monday if there are no issues reported.

snue commented 3 years ago

So the unit test is correct as there's no early return. But I felt early return should just abort any form of reachability analysis.

Yes that sounds like a viable approach. Thanks.

I tested #834 but it doesn't seem to fix the issue yet. For my reproducer from above the function is still excluded. I also tried to add it as an integration test, which confirms it is not working as expected yet.

xd009642 commented 3 years ago

Ah yeah looks like it doesn't actually work with the nested scope so for the test I add the ? in the top level scope actually sorts all the others. I'll add your specific example to the test and sort that this evening

xd009642 commented 3 years ago

Okay I cracked it, I was being a big idiot. Also, it made me aware of another edge case - loops i.e.

fn forever() {
    loop {
        do_stuff();
    }
    unreachable!();
}

So I've also got that sorted. If you want to try it again feel free, but now your MRE is also in the tests as well so it's definitely good for that case :grin:

snue commented 3 years ago

The latest version in #834 works for me! Thanks a lot. This handles the example and my actual code correctly now.

xd009642 commented 3 years ago

Cool just merged it, so you can get it by installing the version on the develop branch now :+1: