Coverage behavioral changes on #[derive] lines

[reaperhulk]

We are seeing changes in coverage behavior in 1.83, 1.84-1.90, and beta/nightly. This code:

use asn1::Asn1Read;

#[derive(Asn1Read)]
struct ThreeIntegers {
    first: u64,
    second: u64,
    third: u64,
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_parse_and_serialize() {
        // This represents SEQUENCE { INTEGER 1, INTEGER 2, INTEGER 3 }
        let der_data = b"\x30\x09\x02\x01\x01\x02\x01\x02\x02\x01\x03";

        // Parse the DER data
        let parsed: ThreeIntegers = asn1::parse_single(der_data)
            .expect("Failed to parse DER data");
        assert_eq!(parsed.first, 1);
        assert_eq!(parsed.second, 2);
        assert_eq!(parsed.third, 3);
    }
}

with this Cargo.toml

[package]
name = "rust-asn1-project"
version = "0.1.0"
edition = "2021"

[dependencies]
asn1 = "0.22.0"

has coverage in 1.83.0 when run with cargo llvm-cov --html (which of course requires llvm-tools-preview).

But in 1.84-1.90 it shows as uncovered.

And in beta and nightly this now doesn't show as a line at all:

Since this has changed multiple times now (covered -> uncovered -> not a line at all) I'm filing this issue to verify that the beta/nightly behavior is expected and won't change again in the future.

[workingjubilee]

Huh. The last one is fairly weird. I don't understand what maps coverage well, but it suggests a subtle change that affected macro-expanded code. @Zalathar?

[Zalathar]

For the behaviour change in 1.91 (beta), the most likely cause is #145643.

A change to derive instrumentation was not intended, though in hindsight it's not entirely surprising.

[alex]

Do you have a view on which behavior is the right one? (For our purposes, it'd be nicest to regard that line as covered :-))

Either way, it seems like it'd be ideal if a test was added to ensure this doesn't regress. Cheers and thanks!

[Zalathar]

From an implementation perspective, the code that enabled the old behaviour was basically unmaintainable. And proc-macros have been a huge headache for coverage instrumentation in general.

So on that basis, I'm very strongly inclined to accept the new behaviour as the new status quo.

(Hypothetically speaking, I'd say the ideal behaviour would be to not instrument derives by default, and have some way to opt in for people who do want that. But right now we're pretty far away from that being feasible.)

[alex]

👍 new behavior works for us! Thanks much!

[workingjubilee]

@Zalathar Do you happen to know what caused the 1.84 change that caused it to switch from instrumented-covered to instrumented-uncovered, as opposed to the switch to non-instrumented?

Ignoring derives during instrumentation is definitely at least better than choosing a seemingly-arbitrary one based on what must have been very fragile logic.

I think given that the question has been answered, then perhaps a coverage test should exist now that verifies that derives are non-instrumented? Then we can close this issue as resolved.

[Zalathar]

I can't point to a specific PR for the 1.84 change, but there were some changes to coverage span processing during that period, so it's not surprising that one of them would have caused such a change.

(E.g. Notice that in the 1.83 screenshot, the covered span is missing the d from Asn1Read. There are subtle and fragile interactions between pieces of code that unexpand to the same source span, and derives cause lots of pieces of code to unexpand to the same or similar span.)

[Zalathar]

We do have one coverage test that makes use of proc-macros, including derive proc-macros (#144616), but curiously in that case it was not instrumented even before #145643 landed in 1.91.

(Perhaps because that proc-macro doesn't use any spans from the source program.)