rust-embedded / heapless

Heapless, `static` friendly data structures
Apache License 2.0
1.51k stars 178 forks source link

Const assertions are not guaranteed to cause compile failures #351

Open rjsberry opened 1 year ago

rjsberry commented 1 year ago

In sealed.rs, there are a number of associated constants which are used in const fns to try and trigger compile failures when const generics don't match particular requirements.

These const fns contain constant expressions, which may (but are not guaranteed to) evaluate at compile time. AFAICT from https://doc.rust-lang.org/reference/const_eval.html while this works now it might stop working in the future and instead cause a runtime panic.

Running cargo check on an example which we know won't build:

% cat examples/bad_indexmap.rs  
fn main() {
    // needs N to be a power of 2
    let _map = heapless::FnvIndexMap::<u8, u8, 3>::new();
}

% cargo check --example bad_indexmap
    Checking heapless v0.8.0 (~/Code/heapless)
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s

% cargo build --example bad_indexmap
   Compiling heapless v0.8.0 (~/Code/heapless)
error[E0080]: evaluation of `heapless::sealed::Assert::<3, 0>::POWER_OF_TWO` failed
  --> ~/Code/heapless/src/sealed.rs:57:37
   |
57 |     pub const POWER_OF_TWO: usize = 0 - (L & (L - 1));
   |                                     ^^^^^^^^^^^^^^^^^ attempt to compute `0_usize - 2_usize`, which would overflow

note: the above error was encountered while instantiating `fn heapless::sealed::power_of_two::<3>`
   --> ~/Code/heapless/src/indexmap.rs:541:9
    |
541 |         crate::sealed::power_of_two::<N>();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0080`.
error: could not compile `heapless` due to previous error

We know this is a bit of a hack evidenced by the comments: // Const assert hack. It would be good to try and add coverage for these compile fails in the cfail test suite so that we can detect if/when this stops being a compile-time error. Unfortunately the trybuild crate is using cargo check under the hood and would not be able to hit this.

burrbull commented 1 year ago

This is not actually related to the issue. This line can be rewritten like:

pub const POWER_OF_TWO: () = assert!(L.is_power_of_two());
newAM commented 10 months ago

This is not actually related to the issue. This line can be rewritten like:

pub const POWER_OF_TWO: () = assert!(L.is_power_of_two());

I tried this out for MpMcQueue, but it did not assert at compile time.

    /// ```compile_fail
    /// use heapless::mpmc::MpMcQueue;
    /// let q: MpMcQueue<i32, 1> = MpMcQueue::new();
    /// ```
    const _ASSERT1: () = assert!(N > 1);
    /// ```compile_fail
    /// use heapless::mpmc::MpMcQueue;
    /// let q: MpMcQueue<i32, 3> = MpMcQueue::new();
    /// ```
    const _ASSERT2: () = assert!(N.is_power_of_two());
---- src/mpmc.rs - mpmc::MpMcQueue<T,N>::_ASSERT1 (line 141) stdout ----
Test compiled successfully, but it's marked `compile_fail`.
---- src/mpmc.rs - mpmc::MpMcQueue<T,N>::_ASSERT2 (line 146) stdout ----
Test compiled successfully, but it's marked `compile_fail`.

failures:
    src/mpmc.rs - mpmc::MpMcQueue<T,N>::_ASSERT1 (line 141)
    src/mpmc.rs - mpmc::MpMcQueue<T,N>::_ASSERT2 (line 146)

I'm surprised this compiles successfully :thinking:

Edit: They needed to be used, see #403

YuhanLiin commented 9 months ago

From https://doc.rust-lang.org/reference/const_eval.html:

In const contexts, these are the only allowed expressions, and are always evaluated at compile time.

As long as we always evaluate the const expressions in const context (so in const variables, for example), it will always evaluate at compile time. We can also use the static_assertions crate so we don't have to roll this ourselves.