rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
95.13k stars 12.27k forks source link

Constant evaluation cannot have code coverage instrumentation #124732

Open clarfonthey opened 2 months ago

clarfonthey commented 2 months ago

This is a hard problem to solve, but I figured I'd open an issue for this since it's an interesting problem that could potentially have a few solutions.

Problem

Constant evaluation and const fn are particularly powerful for moving computations that would normally have to happen at runtime, into compile time. However, moving computations into constant evaluation has the side effect of making them no longer show up under code coverage, which means that a lot of these computations would have to be duplicated in tests for them to show up.

As more and more features get stabilised in const context, this discrepancy will just get larger. Soon, we'll be able to have entire mini programs run at compiler time, which will be completely absent from code coverage. The solution could be solved by macros, but it feels like it would be nice to have the compiler do this.

Example

In the future this could potentially be populated with simpler examples, but, I might as well share the exact example I'm dealing with right now. Full code can be seen here: https://codeberg.org/clarfonthey/discrete-spring

Essentially, a "discrete spring" here represents the state of a pixelated spring which can only be stretched in whole-number increments, meaning that some segments of the spring will be stretched at different amounts from others. The state of such a spring allows a uniform "tension" applied to the spring, then an inner range which has a separate "tension" applied to introduce the non-uniformity:

pub const fn new(outer: (RangeTo<NonZeroU8>, u8), mut inner: (Range<u8>, u8)) -> Self;

In order to ensure that springs representing the same actual state are equal in value (for comparisons & hashing), I "normalize" the state by converting the inner range to 0..0 with the same tension as the outer range if it has the same tension or is empty. This happens in the constructor and after any operation.

To test the normalization code, I decided to create a bunch of sample spring states and compared their values in the test, but because I wanted to share them across tests, I constructed them in constants. This caused the normalization code to be entirely absent from code coverage, since it happened at compile time. Since the normalization after various operations isn't the same as the normalization in the constructor, the tests for those also didn't affect the constructor's code coverage.

(I chose to normalize values in the constructor instead of returning Result or having an unchecked method to keep things simple. Plus, if things are constructed at compile time, this will never affect runtime performance.)

As I said, this particular example, the code coverage being absent for just constructor validation is a pretty minor issue. But as the operations allowed in const evaluation get expanded, there will be a lot of nontrivial cases that soon get omitted from code coverage, requiring the user to test it separately.

The "Solution"

The way to truly account for this is to generate a test that contains the code for the constant evaluation, so that when tests run, the code is covered. I'll go over a suboptimal macro solution, and the solution that would require compiler help.

Workaround with macros

One potential workaround is to create a macro that will generate tests for constants automatically. Here's an example of this, with a playground link: link

macro_rules! test_const {
    ($($vis:vis const $CONST:ident: $T:ty = $E:expr;)*) => {
        $(
            $vis const $CONST: $T = $E;
            #[cfg(test)]
            mod $CONST {
                use super::*;
                #[test]
                fn $CONST() {
                    ::core::hint::black_box($E);
                }
            }
        )*     
    }
}

const fn fib(n: usize) -> usize {
    if n <= 1 {
        1
    } else {
        fib(n - 1) + fib(n - 2)
    }
}

test_const! {
    const TRIVIAL: usize = fib(1);
    const COMPLEX: usize = fib(5);
}

However, there is one issue with this that would require compiler help: the const_eval_select intrinsic. While this code operates the same at runtime and constant time, in general this is not true, and coverage information may differ. While I think it's not worth fretting over minute details of execution that could result from optimization, the fact that certain branches of code could not run at all due to the way const evaluation works feels like something worth accounting for. This will also be even more true once we consider const allocation and other features that will probably exist in the future.

Compiler version

A compiler version of this would effectively do what the macro is doing, with a potentially nicer generation of the tests, converting the below:

const X: T = f();
const Y: U = g();

into the below code:

#[test]
fn eval_X() {
    let X: T = f();
    black_box(X);
}

#[test]
fn eval_Y() {
    let Y: T = g();
    black_box(Y);
}

Although it would require some changes to still apply intrinsics like const_eval_select properly as if the code were running in constant time. It would be still impossible to ensure that generated code runs exactly like the constant evaluation would, but it would certainly be "close enough" for ensuring the basics like if certain methods and branches have been called in constant code.

clarfonthey commented 2 months ago

@rustbot label +A-code-coverage

RalfJung commented 2 months ago

Could you update the issue description to start by explaining the problem -- in the usual way, "here's my code, here's what I did, here's what happened, here's what should have happened"? Currently it starts by saying "the problem is hard" without saying what the problem is. I was confused for a while here. Of course there's no coverage instrumentation, it's compile-time code after all...

clarfonthey commented 2 months ago

Right, I was avoiding providing explicit code examples because I felt that it might get a bit too in the weeds, but I'll try and add one to make it a bit easier to understand.

clarfonthey commented 2 months ago

Updated the description with hopefully a better example. Keep in mind that as I said, I don't think that this feature is worth having now given my current example, but as more stuff gets stabilised for const evaluation, it feels like this would be more and more desired.

RalfJung commented 2 months ago

const fn can be called at compiletime and runtime, so I am not entirely sure why there would be code duplication.

To test the normalization code, I decided to create a bunch of sample spring states and compared their values in the test, but because I wanted to share them across tests, I constructed them in constants. This caused the normalization code to be entirely absent from code coverage, since it happened at compile time. Since the normalization after various operations isn't the same as the normalization in the constructor, the tests for those also didn't affect the constructor's code coverage.

There is another way to share them: write functions of type const fn f() -> T. Then you can call these functions at runtime and will get coverage information.

Arguably that's still a work-around, but the overhead compared to using consts in your tests seems rather small.

clarfonthey commented 2 months ago

const fn can be called at compiletime and runtime, so I am not entirely sure why there would be code duplication.

There is another way to share them: write functions of type const fn f() -> T. Then you can call these functions at runtime and will get coverage information.

Arguably that's still a work-around, but the overhead compared to using consts in your tests seems rather small.

Oh, like I said, I completely agree that this would mostly be overkill with the current state of const evaluation. It mostly just prompted the idea for me since I know that const eval will become substantially more advanced in the future.

RalfJung commented 2 months ago

Yes it will, but the approach of "just" having your test call the const fn at runtime will still work. So it seems to me like if coverage matters, you can keep obtaining coverage that way.

oli-obk commented 1 month ago

if it's not too much complexity in the mir interpreter, could the interpreter collect the coverage data during evaluation and then have it exposed at the use runtime sites of the constant? As long as most of the complexity is in the coverage logic and not the interpreter, that seems maintainable to me.

clarfonthey commented 1 month ago

Coverage instrumentation in miri in general feels like something that would be useful to have, although I'm not sure if that's already possible or not. Since the compiled code already links back to the original source, I would assume that the MIR also has access to the original source as well for storing that info.