rust: improve `static_assert!` #269

ojeda · 2021-05-13T19:16:14Z

Now static_assert! allows for several forms:

Boolean assertion: expr.
Set membership assertion: (expr) is in {a0, ..., aN}.
Interval membership assertion: (expr) is in [min, max].
Fits-in-type assertion: (expr) fits in type.

These are all static_assert!s instead of being split into
a separate macro (e.g. fits_in!) because we want to ensure
they are only within/as a static_assert! (e.g. for the i128
trick).

The documentation is also improved for the boolean assertion case.

Signed-off-by: Miguel Ojeda ojeda@kernel.org

ojeda · 2021-05-13T19:46:37Z

This is an RFC/experiment. Let me know what you think. It is also likely I am missing a better way, so please feel free to submit another alternative.

There are two main points to discuss here.

First of all, whether this makes sense at all. I added the interval/set cases later (since the fits in could use the interval one), but I did this for the fits in one, for things like #266 (comment).

I tried a few alternatives without macros, but AFAIK there is no C++ consteval equivalent in Rust (i.e. to avoid the function being used at all at runtime), so while it was possible to provide something like fits_in::<i16>(x as i128) or fits_in_i16(x as i128), u32_fits_in_i16(x), x.fits_in_i16(), etc. people could still use it as a normal function. Some of those alternatives also required macros, copy-pasting or nightly features (such as impl const) anyhow. And we do not have overloading support, which makes some of them uglier.

So I ended up going with a macro, and thinking that since we enforce this to be always a static_assert! (i.e. instead of providing independent fits_in! macros), then we could allow people to use i128 without too much worry in this context, e.g. for things like the MAX_ERRNO use case which needs a negation and therefore one needs a cast anyway (since it is a u32):

static_assert!((-(bindings::MAX_ERRNO as i128)) fits in i16);

The second part of the experiment is going with custom syntax, all in static_assert!:

static_assert!((expr) fits in i16);
static_assert!((expr) is     in {FOO, BAR, BAZ});
static_assert!((expr) is not in {FOO, BAR, BAZ});

instead of going for different macros:

static_assert_fits_in!(expr, i16);
static_assert_is_in_set!    (expr, {FOO, BAR, BAZ});
static_assert_is_not_in_set!(expr, {FOO, BAR, BAZ});

I do not know which is best. Part of me just wanted to see how it looked ;)

Now `static_assert!` allows for several forms: - Boolean assertion: `expr`. - Set membership assertion: `(expr) is in {a0, ..., aN}`. - Interval membership assertion: `(expr) is in [min, max]`. - Fits-in-type assertion: `(expr) fits in type`. These are all `static_assert!`s instead of being split into a separate macro (e.g. `fits_in!`) because we want to ensure they are only within/as a `static_assert!` (e.g. for the `i128` trick). The documentation is also improved for the boolean assertion case. Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

TheSven73 · 2021-05-13T22:46:49Z

I tried this out on #268 and it's a great solution ! I wish I knew enough about Rust macros to help review.

One small nit though: when the static assertion fails, the build error is very cryptic. It's not immediately obvious what has happened.

const MY_ERRNO: u32 = 40000;
static_assert!((-(MY_ERRNO as i128)) fits in i16);

error: any use of this value will cause an error
   --> rust/kernel/static_assert.rs:129:23
    |
129 |         const _: () = [()][!($condition) as usize];
    |         --------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
    |                       |
    |                       index out of bounds: the length is 1 but the index is 1
    | 
   ::: rust/kernel/error.rs:113:1
    |
113 | static_assert!((-(MY_ERRNO as i128)) fits in i16);
    | -------------------------------------------------- in this macro invocation
    |
    = note: `#[deny(const_err)]` on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

soruh · 2021-05-13T23:10:21Z

rust/kernel/static_assert.rs

+    };
+    (($expression:expr) is not in [$min:expr, $max:expr]) => {
+        static_assert!(!(($expression) >= ($min) && ($expression) <= ($max)));
+    };


It would probably be better to use regular Rust range syntax (and range.contains(...)) here. I think it's generally useful to have macros resemble normal syntax as closely as possible as it makes the macros easier to learn. Is there a reason we can't / don't want to do that here?

soruh · 2021-05-13T23:17:04Z

One small nit though: when the static assertion fails, the build error is very cryptic. It's not immediately obvious what has happened.

I don't know if there is much that can be done to address that as this is somewhat abusing the compiler safety checks to produce the error. One small way to improve this would be to replace the const _: () = with const _static_assertion_failed: () = which would still have "out of bounds access" as the message but would at least give some hints as to what happened / where to look for more information.

soruh · 2021-05-13T23:26:37Z

Ah of course this would probably make messages better when it becomes stable.

soruh · 2021-05-13T23:57:07Z

rust/kernel/static_assert.rs

+    };
+    (($expression:expr) is not in {$($a:expr),+}) => {
+        static_assert!(!($(($expression) == ($a))||*));
+    };


While I do like maths notation, I'd probably make these use the square brackets as that is generally how you represent a collection in Rust. This isn't as relevant as the "in [ range ]" one though as this one can't be misunderstood when read in in isolation.

soruh · 2021-05-14T00:00:00Z

rust/kernel/static_assert.rs

+static_assert!((-2) is not in [-1, 2]);
+static_assert!((-1) is     in [-1, 2]);
+static_assert!(( 0) is     in [-1, 2]);
+static_assert!(( 1) is     in [-1, 2]);


Re: using ranges here:
Reading this line in isolation and without prior knowledge of this macro I would probably expect this to fail as "1 is neither -1 nor 2"

soruh · 2021-05-14T00:06:22Z

The required parathesis around the expressions seem unfortunate but are probably unavoidable.

In general I like this quite a lot though btw. and the macros themselves (apart from my syntax nits :) ) look correct to me.

soruh · 2021-05-14T00:07:17Z

rust/kernel/static_assert.rs

    ($condition:expr) => {
        // Based on the latest one in `rustc`'s one before it was [removed].
        //
        // [removed]: https://github.com/rust-lang/rust/commit/c2dad1c6b9f9636198d7c561b47a2974f5103f6d
        #[allow(dead_code)]
        const _: () = [()][!($condition) as usize];
    };
+
+    // Set membership assertion: `(expr) is in {a0, ..., aN}`.
+    (($expression:expr) is in {$($a:expr),+}) => {


These could have a $(,)? after the ,+ to allow for trailing commas.

ojeda · 2021-05-17T22:14:41Z

@soruh Answering here to several of the comments in one.

I agree that inventing new syntax is not ideal, in particular if it is close to the "real" one.

On the ranges syntax (.., ..=...), we cannot use those after an expression of course, so we cannot make it exactly the same syntax.

On the is in [] notation, I agree it can be misleading for someone from Rust. One easy solution is to be a bit more explicit with:

is in set
is in interval

Then we could also use [] for the set too (although it is nice to have a different symbol to spot it quicker, but I am fine both ways).

Concerning the parenthesis around (expr), there are other alternatives I tried (adapted to add interval to them):

(expr) is in interval [a, b]   # Current one.

expr => is in interval [a, b]
expr, is in interval [a, b]    # Looked a bit like arguments to `static_assert!`.
expr; is in interval [a, b]

[a, b] interval contains expr
interval [a, b] contains expr

The last ones are closer to Rust, but then if we want to match fits in reversing the order in that one too (using something like can hold), we have a problem with the type, i.e. we need something like:

type => can hold expr

An advantage of the => notation is that it works for both cases (i.e. both types and expressions):

42 => fits in i8
42 => is in set [21, 42]

i8 => can hold 42

It could be called the "asserting something about" operator when used inside our static_assert! macro. :)

I am not particularly in favor of any particular way, so I am happy to hear opinions!

I also tried to support open intervals etc., but we cannot use () for that, so we need something else instead of the math notation.

ojeda · 2021-05-17T22:44:24Z

One small nit though: when the static assertion fails, the build error is very cryptic. It's not immediately obvious what has happened.

Yeah, that is a problem already for the current static_assert!. Let's put it in an issue... #274

wedsonaf · 2021-05-19T00:27:40Z

rust/kernel/static_assert.rs

    ($condition:expr) => {
        // Based on the latest one in `rustc`'s one before it was [removed].
        //
        // [removed]: https://github.com/rust-lang/rust/commit/c2dad1c6b9f9636198d7c561b47a2974f5103f6d
        #[allow(dead_code)]
        const _: () = [()][!($condition) as usize];
    };
+
+    // Set membership assertion: `(expr) is in {a0, ..., aN}`.
+    (($expression:expr) is in {$($a:expr),+}) => {


Do we need the parenthesis around the expression?

Yeah, expressions can only be followed by some tokens. See my comments above.

wedsonaf · 2021-05-19T00:37:41Z

rust/kernel/static_assert.rs

+///   - Boolean assertion: `expr`.
+///   - Set membership assertion: `(expr) is in {a0, ..., aN}`.
+///   - Interval membership assertion: `(expr) is in [min, max]`.
+///   - Fits-in-type assertion: `(expr) fits in type`.


When I saw this, the first thing I thought was whether a type could be 'transmuted' into another. For example, whether *const T fits in usize. Could we support something like this?

So a comparison of the sizes? Or something more?

I wonder if it is best to leave that to utilities on their own (e.g. like transmute checks for the size).

wedsonaf · 2021-05-19T00:39:42Z

rust/kernel/static_assert.rs

+/// There are several forms of this macro:
+///
+///   - Boolean assertion: `expr`.
+///   - Set membership assertion: `(expr) is in {a0, ..., aN}`.


Pascal has an in operator. If we want to follow precedent, perhaps we could drop is and just have expr in {a0, ..., aN}.

Yeah, Python too (as well as the not in). I am happy either way!

nbdd0121 · 2021-05-19T04:44:36Z

Here's a version of static_assert! that accepts the syntax suggested by @soruh. I haven't added fits-in-type assertion.

macro_rules! static_assert {
    (@normal_assert $invert:literal ($condition:expr)) => {
        const _: () = [()][(($condition) == $invert) as usize];
    };
    (@set_assert $invert:literal ($expr:expr) [$($a:expr),+]) => {
        static_assert!(@normal_assert $invert ($(($expr) == ($a))||*));
    };
    (@range_assert $invert:literal ($expr:expr) ($min:expr) ($max:expr)) => {
        static_assert!(@normal_assert $invert (($expr) >= ($min) && ($expr) < ($max)));
    };
    (@range_from_assert $invert:literal ($expr:expr) ($min:expr)) => {
        static_assert!(@normal_assert $invert (($expr) >= ($min)));
    };
    (@range_inclusive_assert $invert:literal ($expr:expr) ($min:expr) ($max:expr)) => {
        static_assert!(@normal_assert $invert (($expr) >= ($min) && ($expr) <= ($max)));
    };
    (@range_to_assert $invert:literal ($expr:expr) ($max:expr)) => {
        static_assert!(@normal_assert $invert (($expr) < ($max)));
    };
    (@range_to_inclusive_assert $invert:literal ($expr:expr) ($max:expr)) => {
        static_assert!(@normal_assert $invert (($expr) <= ($max)));
    };
    (@parse_expr ($($head:tt)*)) => {
        static_assert!(@normal_assert false ($($head)*));
    };
    (@parse_expr ($($head:tt)*) in $($tail:tt)*) => {
        static_assert!(@parse_range false ($($head)*) () $($tail)*);
    };
    (@parse_expr ($($head:tt)*) not in $($tail:tt)*) => {
        static_assert!(@parse_range true ($($head)*) () $($tail)*);
    };
    (@parse_expr ($($head:tt)*) $current:tt $($tail:tt)*) => {
        static_assert!(@parse_expr ($($head)* $current) $($tail)*);
    };
    (@parse_range $invert:literal ($expr:expr) () [$($tail:tt)*]) => {
        static_assert!(@set_assert $invert ($expr) [$($tail)*]);
    };
    (@parse_range $invert:literal ($expr:expr) ($($head:tt)+) ..) => {
        static_assert!(@range_from_assert $invert ($expr) ($($head)*));
    };
    (@parse_range $invert:literal ($expr:expr) ($($head:tt)+) .. $($tail:tt)+) => {
        static_assert!(@range_assert $invert ($expr) ($($head)*) ($($tail)*));
    };
    (@parse_range $invert:literal ($expr:expr) ($($head:tt)+) ..= $($tail:tt)+) => {
        static_assert!(@range_inclusive_assert $invert ($expr) ($($head)*) ($($tail)*));
    };
    (@parse_range $invert:literal ($expr:expr) () .. $($tail:tt)+) => {
        static_assert!(@range_to_assert $invert  ($expr) ($($tail)*));
    };
    (@parse_range $invert:literal ($expr:expr) () ..= $($tail:tt)+) => {
        static_assert!(@range_to_inclusive_assert $invert ($expr) ($($tail)*));
    };
    (@parse_range $invert:literal ($expr:expr) ($($head:tt)*) $current:tt $($tail:tt)*) => {
        static_assert!(@parse_range $invert ($expr) ($($head)* $current) $($tail)*);
    };
    (@ $($tail:tt)*) => { compile_error!("wrong syntax"); };
    ($($tail:tt)*) => { static_assert!(@parse_expr () $($tail)*); }
}

static_assert!(1 == 1);
static_assert!(2 in [1, 2]);
static_assert!(2 in 1 .. 100);
static_assert!(2 in 0..= 100);
static_assert!(2 in ..= 100);
static_assert!(2 in .. 100);
static_assert!(2 in 0..);
static_assert!(2 in [1, 2]);
static_assert!(2 not in 3 .. 100);
static_assert!(2 not in 3..= 100);
static_assert!(2 not in ..= 1);
static_assert!(2 not in .. 2);
static_assert!(2 not in 3..);
static_assert!(2 not in [1, 3]);

ojeda · 2021-05-19T08:05:33Z

@nbdd0121 Heh, taking tokens individually is indeed very cool. It defeats a bit the beauty of the "by example" part of macros by example that I wanted to show with this file, but of course opens up more possibilities.

The obvious concern is how it will be received when people take a look at this file. But given we already have proc macros around, I guess it is fair to say we already crossed that bridge a long time ago... :P

If we go with the token tree approach, then I would focus on telling people the same we do for module!: to look at the user side and the benefits it brings. If we go with the current approach, then I would focus on "look how much can be achieved with a simple macro".

ksquirrel · 2021-05-19T11:57:22Z

Review of dd89a2e2b254:

✔️ Commit dd89a2e: Looks fine!

fee1-dead · 2021-12-12T09:10:04Z

Since rust-lang/rust#89508 has landed I don't see why this can't use assert! with custom messages.

ojeda mentioned this pull request May 13, 2021

rust/kernel: move from_kernel_result! macro to error.rs #266

Merged

ojeda force-pushed the more-static-asserts branch from 8a4b153 to dd89a2e Compare May 13, 2021 20:04

soruh reviewed May 13, 2021

View reviewed changes

soruh reviewed May 14, 2021

View reviewed changes

ojeda mentioned this pull request May 17, 2021

Improve diagnostics for failed static_assert! #274

Closed

wedsonaf reviewed May 19, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust: improve `static_assert!` #269

rust: improve `static_assert!` #269

ojeda commented May 13, 2021

ojeda commented May 13, 2021 •

edited

Loading

TheSven73 commented May 13, 2021

soruh May 13, 2021 •

edited

Loading

soruh commented May 13, 2021

soruh commented May 13, 2021

soruh May 13, 2021

soruh May 14, 2021

soruh commented May 14, 2021

soruh May 14, 2021

ojeda commented May 17, 2021 •

edited

Loading

ojeda commented May 17, 2021

wedsonaf May 19, 2021

ojeda May 19, 2021

wedsonaf May 19, 2021

ojeda May 19, 2021

wedsonaf May 19, 2021

ojeda May 19, 2021

nbdd0121 commented May 19, 2021

ojeda commented May 19, 2021

ksquirrel commented May 19, 2021

fee1-dead commented Dec 12, 2021

rust: improve static_assert! #269

Are you sure you want to change the base?

rust: improve static_assert! #269

Conversation

ojeda commented May 13, 2021

ojeda commented May 13, 2021 • edited Loading

TheSven73 commented May 13, 2021

soruh May 13, 2021 • edited Loading

Choose a reason for hiding this comment

soruh commented May 13, 2021

soruh commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soruh commented May 14, 2021

Choose a reason for hiding this comment

ojeda commented May 17, 2021 • edited Loading

ojeda commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 commented May 19, 2021

ojeda commented May 19, 2021

ksquirrel commented May 19, 2021

fee1-dead commented Dec 12, 2021

rust: improve `static_assert!` #269

rust: improve `static_assert!` #269

ojeda commented May 13, 2021 •

edited

Loading

soruh May 13, 2021 •

edited

Loading

ojeda commented May 17, 2021 •

edited

Loading