cleanup non_zero macro with const generics?

[VladimirBramstedt]

Hiya. i like this crate, but i feel that using the non_zero macro on every construction makes for quite a bit of visual noise, and what it does is mostly enforceable at compile time using const generics.
heres a link to the playground with the idea (try with 0 values to prove it doesnt compile.)
https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=805e21d247907ef83759e8066de5957d
this allows for more contexts to provide parameters to construct quotas, namely named constants and const-evaluable functions in addition to literals, and removes the need to specify u32 constantly.
is does require a specific minimum rust version, but would you be amenable to expanding the api with these kind of constructors?

[antifuchs]

...wow. That's really clever. I think we can add constructors like that, but I'm not sure it should go on the "main" module, per se. The way these constructors are invoked reads nothing like I'd expect idiomatic rust to read (and that's probably due to const generics being so new that we haven't internalized how it should look yet!)... but it's definitely much more ergonomic.

My main concern is that these functions would be very useful outside of governor, they almost seem too generic. Wonder if we can just get these construction const fn's into nonzero_ext and allow automatically converting them via a trait in governor (or via another trait exported from nonzero_ext).

[VladimirBramstedt]

I dont think they are that weird, style-wise. for example https://docs.rs/typed-headers/latest/typed_headers/trait.HeaderMapExt.html read similarly API wise,
headers.get::<Connection>() etc. functions that take their arguments directly via the type parameter type aren't unusual. we're just expanding on that to add constants to the arsenal. (granted, the <{..}> notation is a little weird).

My goal is to make the constructors more ergonomic. heck, even just being able to use a named constant is an improvement, otherwise you need comments on why that literal is chosen for a quota.

im not sure i see how moving these into nonzero_ext is an improvment? wont that just change Quota::foo(nonzero(42)) instead of Quota::foo(non_zero!(42)) ? ultimetly i think you are after the nightly-only const impl traits and bounds:
Quota::per_second(n: T) where T: ~const IntoNonZero { let foo = n.into_non_zero() }. and while that will be an improvement, it is nightly only...

with that said, i can agree it doesnt have to be the main constructor, but its nice as an option, or even feature flagged if you dont want to commit to a minimum-rust version.

[antifuchs]

Yep, I think we're on the same page! I am definitely open to getting a more ergonomic way of passing constant numbers in there & will explore the shape that this API change could take.

For a moment, I was wondering if this even requires a function: Wonder if there's a struct that takes a const generic parameter & can be converted into the appropriate non-zero value, playing some const tricks that we use for the compile-time assertions in the nonzero macro, e.g. this possibly-nonsense untested code:

use governor::{Quota, Second};
let quota = Quota::per(Second::<30>);

...where the Second struct might contain an array of length N-1, which would fail to compile for N=0...

(Update: As it happens, you can't do complex things with const generics yet, so a function will be necessary; boo. It would be really nice, though: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8018ac7a339b5d23aaff694be4ea0b51)

[VladimirBramstedt]

yeah, there are a lot of very nice things regarding constness in nightly that i wish would make it into stable soon.
Anyway, i think with my suggestion it will boil down to

let q = Quota::per_second(nonzero!(50u32)).allow_burst(nonzero!(50u32))); //current
let q = Quota::per_second::<30>().allow_burst::<10>(); // const generics

which i think is an improvement, at least. if you wanna incorporate that i can PR it, although most of the code is pretty much there in the playground link, its just a matter of where to put it.

Thanks for at least considering my idea :)

Edit: its important to notice that the const generic variant really does only work at compile time, while the current version does work runtime as well, if you manually construct a NonZero type, which is important difference, so we shouldn't remove the existing one, just add something. unsure about the name but it cant just be per_second.

[antifuchs]

So with #203, I've been looking at Quotas some more and I can't make your example fail at compile time. Here's what I'm working with: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=910426a874f4d2d2d64d8fc606d50ab1, and it compiles but then panics. That's really weird, as I thought the bounds-checking const function ought to get inlined... but apparently it doesn't, and when that happens, the whole safety guarantees of the type (as in, your program will fail to compile if you pass the wrong const) go out the window /:

Edit: oooh, wait, if I change the binding to a const (instead of a let), that does eval the function at compile time & appropriately errors. That seems better, but is not great: I would love for all usages with literals to do the check at compile time, rather than runtime panicking

[VladimirBramstedt]

what the hell. now that i try, its not failing either, except im sure it did. it seems that rustc no longer evaluates the predicate at compile time even though it should very much be possible to do so. its almost worth raising upstream to the rustc folks and ask what stops it from const'ing.

[antifuchs]

So, I think I understand what's going on now: The expression in not_zero is not guaranteed to be evaluated in a const context. It is, however, possible to use #![feature(inline_const)] to ensure that it is: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=ef5cacb419d52845efe30ddeb0262a81; that's still not stable, but at least there's some hope for const generics being useful for these things!

[VladimirBramstedt]

sure, i understand it is not guaranteed to be, but this is a trivial case that i would reasonably expect to be and has worked before (i tested that example, although i cant remember the rustc version anymore..., and i guess the lack of guarantees meant it compiled on my platform "correctly" but not on the playground?).
anyway, i guess we can shelve this until rust actually have useful const story without having to resort to weird things to ensure const eval.