Should the static be defined in the `hit!` rather than `check!`?

[nagisa]

Currently we define the #[no_mangle] static in the check! macro.

This means that the test like this:

fn safe_divide(dividend: u32, divisor: u32) -> u32 {
    if divisor == 0 {
        cov_mark::hit!(save_divide_zero);
        return 0;
    }
    dividend / divisor
}

#[test]
fn test_safe_divide_by_zero() {
    cov_mark::check!(save_divide_zero);
    assert_eq!(safe_divide(92, 0), 0);
}

#[test]
fn test_safe_divide_by_zero2() {
    cov_mark::check!(save_divide_zero);
    assert_eq!(safe_divide(92, 0), 0);
}

will fail with an error like this:

error: symbol `save_divide_zero` is already defined
  --> tests/smoke.rs:17:5
   |
17 |     cov_mark::check!(save_divide_zero);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

because we’re defining the same symbol in the same crate twice.

Ideologically it seems to me that the users will want this kind of check for their hit! macros (to ensure they're not duplicate), but they might want to use the check! macro in multiple tests.

Would it make sense to move the definition of the counter static to the hit! macro instead?

[matklad]

I am unsure about this one.

The current scheme has the advantage that, if you accidentally remove the test, you get a compilation error, and, if you accidentally remove the code, you get a runtime test failure.

If we move the static definition to hit, then removing the test would no longer lead to an error -- the marks could become orphan.

I personally feel that such automated checking of the "set" of marks is useful. You can, of course, imagine some kind of external tool which does this, but making linker do this feels more usable.

Could we convince the linker to treat definitions as inline? IE, just require it to merge all equivalent marks?

[nagisa]

Alas, this is not the linker that's complaining about the duplicate definitions, but rustc itself. During the collection of monomorphizations it will ensure that it only collects one definition with the same symbol. This logic exists to prevent another kind of unsoundness that comes from LLVM where it would just "concatenate" code for two different definitions.

I guess we could look at figuring out an API to offload the decision to the user?

[matklad]

I guess we could look at figuring out an API to offload the decision to the user?

Yeah, it would be fine by me!

[akiross]

I see value in having declaration, hit! and check! as 3 separated calls: this allows to use the same symbol in multiple places, both hits and checks.

If declaration happens in #[cfg(test)], then you get broken compilation if you move the code around (same thing as missing check!). This is not as robust as the current solution, but could be a compromise for improved flexibility.

Moving the declaration from checks to hits will, I believe, allows for multiple checks, but not for multiple hits of the same symbol: the opposed, but similar, case.

Here one use case where it could be useful:

• a function call appears in 2 different places of the code
• it might happen in place 1 or 2 depending on a condition, but it must happen exactly once,
• you want to ensure that it happens once in different conditions.

[akiross]

(Naturally I'm not suggesting to break the existing code, but to define 3 primitives D, H, C that can be used as an alternative: hit = H, and check = D + C.)

[akiross]

I agree that having a more complex API is not worth it, if we can keep it simpler and give less room for error to the user. Here a use case in which having define! could be used: this is actually a toy version of a real example, so let's see if we can use cov-mark as-is, without additions.

As I was saying above, the use case is that an event might be called in two different places.

With defines

struct Context(bool);
impl Context {
    fn new() -> Self { Context(false) }
    fn open(&mut self) {
        cov_mark::hit!(opening);
        self.0 = true;
    }

    fn close(&mut self) {
        cov_mark::hit!(closing);
        println!("Closing manually");
        self.0 = false;
    }
}

impl std::ops::Drop for Context {
    fn drop(&mut self) {
        if self.0 {
            cov_mark::hit!(closing);
            println!("Closing automatically");
            self.0 = false;
        }
        println!("Context is closed");
    }
}

fn main() {
    println!("Hello, world!");
}

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

    cov_mark::define!(opening);
    cov_mark::define!(closing);

    #[test]
    fn test_noop() {
        cov_mark::check_count!(defined opening, 0);
        cov_mark::check_count!(defined closing, 0);
        let _c = Context::new();
    }

    #[test]
    fn test_manual() {
        cov_mark::check_count!(defined opening, 1);
        cov_mark::check_count!(defined closing, 1);
        let mut c = Context::new();
        c.open();
        c.close();
    }

    #[test]
    fn test_automatic() {
        cov_mark::check_count!(defined opening, 1);
        cov_mark::check_count!(defined closing, 1);
        let mut c = Context::new();
        c.open();
    }
}

Personal Pros:

• I like that there is "a name for each code branch", and that you can refer to it where needed.
• The mapping between "counting events" and "asserting properties on those events" seems natural and easy to read.
• It's easy to assert something not happening, using the same paths shared by other tests.

Without defines

I came up with this: it might be a bad solution (as the one above), so please correct me if I'm mistaken.

struct Context(bool);
impl Context {
    fn new() -> Self { Context(false) }
    fn open(&mut self) {
        cov_mark::hit!(opening_manual);
        cov_mark::hit!(opening_automatic);
        self.0 = true;
    }

    fn close(&mut self) {
        cov_mark::hit!(closing_manual);
        println!("Closing manually");
        self.0 = false;
    }
}

impl std::ops::Drop for Context {
    fn drop(&mut self) {
        if self.0 {
            cov_mark::hit!(closing_automatic);
            println!("Closing automatically");
            self.0 = false;
        }
        println!("Context is closed");
    }
}

fn main() {
    println!("Hello, world!");
}

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

    #[test]
    fn test_noop() {
        // Is this testing the absence of events?
        let _c = Context::new();
    }

    #[test]
    fn test_manual() {
        cov_mark::check!(opening_manual);
        cov_mark::check!(closing_manual);
        let mut c = Context::new();
        c.open();
        c.close();
    }

    #[test]
    fn test_automatic() {
        cov_mark::check!(opening_automatic);
        cov_mark::check!(closing_automatic);
        let mut c = Context::new();
        c.open();
    }
}

Personal Pros:

• I like how symbols are different, so it's easy to understand where they are used.

Personal Cons:

• The no-op test loses it's meaning, and while counts could be used, they add other hits and symbols.

Let me know what you think :)