taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
933 stars 57 forks source link

Show missing lines: trouble with multiple derive macros, multiple functions in the same file #181

Closed vmiklos closed 2 years ago

vmiklos commented 2 years ago

Sigh, I'm afraid this is a 3rd issue in this area, I didn't expect this will be so tricky... :-)

Given this rust file:

$ cat src/lib.rs 
pub type BoxResult<T> = Result<T, Box<dyn std::error::Error>>;

pub fn foo() -> BoxResult<()> {
    Ok(())
}

pub fn bar() -> BoxResult<()> {
    Ok(foo()?)
}

#[derive(serde::Deserialize)]
struct RelationDict {
}

pub fn baz() -> i32 { 42 }

pub fn blah() -> i32 { 42 }

#[cfg(test)]
mod tests {
    use super::*;
    #[test]
    fn it_works() {
        bar().unwrap();

        let j = serde_json::json!({});
        let _: RelationDict = serde_json::from_value(j).unwrap();
    }
}

and this project definition:

$ cat Cargo.toml 
[package]
name = "t"
version = "0.0.0"
edition = "2021"
[dependencies]
serde = { version = "1.0.137", features = ["derive"] }
serde_json = "1.0.81"
[workspace]

Expected output is that baz() and blah() is not covered.

Actual output:

$ ~/git/cargo-llvm-cov/target/debug/cargo-llvm-cov llvm-cov -q --show-missing-lines
...
Uncovered Lines:
/home/vmiklos/git/t/src/lib.rs: 11

11 is the #[derive(serde::Deserialize)] line.

I believe there are 2 issues here:

1) This line:

https://github.com/taiki-e/cargo-llvm-cov/blob/1e717e8c1287f9d6170c5da5e32c1fd6eeca99e4/src/json.rs#L94

is naive: we iterate over a list of functions there, but then insert into a map where the key is the filename. This means we only show missing lines for the last function (with missing lines) in a file.

Once, this is fixed, we'll see missing lines for baz() and blah() as well.

2) The other problem is that a derive macro may define several functions inside the same line, so we can have two functions, which are expanded from a macro from the same line: one covered and one not covered. llvm-cov itself considers the #[derive(serde::Deserialize)] covered, so we should probably check for lines which are both covered in one function and are uncovered in some other functions and consider them covered, too.

Does this look like a reasonable plan? Thanks.

I have some PoC code at https://github.com/vmiklos/cargo-llvm-cov/commits/show-missing-lines3 that does this, but wanted to get some feedback before I invest time into writing up some proper commit message, tests, etc.

The positive side is that my osm-gimmisn project with 7-8k lines of code and ~complete line coverage now doesn't show any false output with --show-missing-lines (when llvm-cov itself reports 100% line coverage). So perhaps this would be the last "false alarm in show-missing-lines output" issue for a while.

taiki-e commented 2 years ago

Thanks for investigating this! Your patch looks good to me.