#[non_exhaustive] in library (will make things difficult). Best practice?

I have the following code:

/// Variable settings for [`Bandpass`] block
///
/// These settings may be passed as `BandpassParams` struct to
/// [`Bandpass::new`] or the field names can be used with the [`bandpass!`]
/// macro.
#[derive(Clone, Debug)]
pub struct BandpassParams {
    /// Lower end of passband in hertz (must have positive sign)
    pub low_freq: f64,
    /// Upper end of passband in hertz (must have positive sign)
    pub high_freq: f64,
    /// Select whether lower, upper, or both side bands should pass
    pub band: Band,
    /// Blur frequency response up to ±`blur * sample_rate / chunk_len` hertz (first null)
    pub blur: f64,
    /// Parameter τ for RC lowpass (deemphasis)
    pub deemphasis: f64,
}

impl Default for BandpassParams {
    /// Default settings
    /// (all frequencies pass, `blur` set to `2.0`, deemphasis disabled)
    fn default() -> Self {
        Self {
            low_freq: 0.0,
            high_freq: f64::INFINITY,
            band: Band::Dual,
            blur: 2.0,
            deemphasis: 0.0,
        }
    }
}

pub struct Bandpass {
    receiver: Receiver<Chunk<Complex<f32>>>,
    sender: Sender<Chunk<Complex<f32>>>,
    params: watch::Sender<BandpassParams>,
}

#[macro_export]
macro_rules! bandpass {
    ($chunk_len:expr, $sample_rate:expr $(,)?) => {
        $crate::blocks::filters::Bandpass::new($chunk_len, $sample_rate, Default::default())
    };
    ($chunk_len:expr, $sample_rate:expr, $( $name:ident: $value:expr ),+ $(,)?) => {
        $crate::blocks::filters::Bandpass::new(
            $chunk_len,
            $sample_rate,
            $crate::blocks::filters::BandpassParams {
                $( $name: $value ),+,
                ..Default::default()
            }
        )
    };
}

impl Bandpass {
    pub fn new(chunk_len: usize, sample_rate: f64, params: BandpassParams) -> Self {
        /* … */
    }
    /* … */
}

I considered adding #[non_exhaustive] to the BandpassParams struct, but the reference says:

Non-exhaustive types cannot be constructed outside of the defining crate:

See also this thread.

I.e. in another crate which uses my library, I can no longer write:

let params = BandpassParams { low_freq = 300.0, .. Default::default() };

And even my macro would fail and would need to be written in a different way.

What's the best practice for non-exhaustive structs where all fields are guaranteed (by convention/documentation) to have a default?

  • Use #[non_exhaustive] and disallow the .. Default::default() syntax?
  • Simply live with a documentation comment to warn users that fields may be added any time?

It depends on how often you expect to be adding fields: non_exhaustive will let you add them with only a minor version bump, which will disrupt downstream developers less than a major one.

Isn't it my choice when to do minor or major version bumb? The Cargo Book says:

These are only guidelines, and not necessarily hard-and-fast rules that all projects will obey. The Change categories section details how this guide classifies the level and severity of a change.

I would say that any documentation I add to my own crate (e.g. "treat this as if it was non-exhaustive, even if it isn't marked as such") will be normative, so when someone uses the struct in a non-intended way, it's their fault for having a compiler error when a field is added during a minor version change.

How about using setters:

let params = BandpassParams::new().low_freq(300.0);
1 Like

I think this approach might lead to some painful developer experiences.

Imagine you wrote some crate (crate_a v1.0.0) which is used by another crate (crate_b v2.0.0) and I'm writing an application (bin_c v3.0.0) that depends on crate_b. Now let's assume your struct doesn't have the #[non_exhaustive] attribute and crate_b is instantiating the struct with a struct literal (let foo = crate_a::Foo { x: ..., y: ... }).

What happens when you release crate_a v1.0.1 which adds another field (e.g. z) to the crate_a::Foo type? When I try to compile my executable, cargo will look at my Cargo.toml and see that it depends on crate_b = "^2.0.0", which in turn depends on crate_a = "^1.0.0", and because both 1.0.0 and 1.0.1 have been published to crates.io, cargo will choose to compile crate_a v1.0.1 with crate_b 2.0.0... Which will cause compilation issues because crate_b isn't initializing the z field on crate_a::Foo.

The end result is that by saying "it's your own fault if you don't use this struct as intended - you should have read the docs" and publishing a backwards incompatible change under a backwards compatible version bump, you can break any code which transitively depends on your crate.

A much better approach is to use #[non_exhaustive] and builder methods.

let mut params = BandpassParams::default();
params.low_freq = ...;

// or

let params: BandpassParams = BandpassParams::default()
    .with_low_freq(...)
    .with_high_freq(...);
4 Likes

In this case, I'd personally lean towards just using major version bumps-- The existing fields look like a pretty complete description of a bandpass filter, so it feels unlikely that there will need to be more added on a regular basis.

It is, but Cargo assumes that you follow semver conventions. So if you don't, you will end up causing spurious and inexplicable compile errors.


That said, I don't like #[non_exhaustive] and I'd rather do major version bumps than deal with the pain of not being able to create/destructure values. Especially while your crate is in early development, it's fine to go from 0.1.0 to 0.9.0 in a week. Don't promise 1.0 until your crate is widely used, debugged, tested, optimized and stable, anyway.

6 Likes

Well, for some value of "widely used", which can go down to somebody else uses it

I understand your arguments, but isn't it normal that some behavior of an API is described to "change at any time" by convention or documentation comment?

Consider this Google search. If I rely on anything that "may change in the future", then bad things can happen. Also for crates that depend on the crate that doesn't use things like they are supposed to be used.

#[non_exhaustive] is a way to formalize this "may change in the future" in regard to adding struct/enum variants, but isn't that only one of many things that can change in a library (even in minor version bumps if documented)? I would say causing a compiler error is one of the more harmless things that can go wrong.

So to phrase it differently: Using #[non_exhaustive] isn't exhaustive when it comes to prevent such problems. :wink:

The problem is not that your library changes behavior. The problem is that the way you handle it goes against expectations/conventions of people and tools in the ecosystem. Absolutely do introduce breaking changes if it improves the correctness, usability, or performance of your library. Just make sure you perform a major version bump if you do so.

6 Likes

I'm not sure you're making the point you seem to think you're making? Yes, compiler errors are one of the less bad things that can happen, one of the more bad things is getting broken dependencies because someone decided to make a breaking change without a major version bump. When you have a tool that can help avoid such a breaking change, why not use it?

Another way to look at it is that struct literals and exhaustive matching are automatically generated APIs that work fine for most cases, but might be more than you want to promise for an API long term. Functionally there's no difference with, say, adding a new parameter to a method, which is also going to break anyone that called that method. Are you also fine with releasing that change in a minor version?

5 Likes

If using #[non_exhaustive] really is considered to be mandatory if you want to inform users that fields may be added, then this is a good and valid arguement.

Because it is unergonomic (I think).

From the other thread:

I would like to understand why it can't be changed to allow construction with the Struct { .. Default::default() } syntax.

I'm not sure if that's a good comparison. A function where arguments may be added or modified within minor releases can't be used properly (maybe through macros provided by the same crate), but a struct where fields may be added can.

The tool that Rust provides (#[non_exhaustive]) has some implications that seem troublesome, so that's why I thought it might be reasonable to not use it and instead use convention and documentation. But if you all think this is very unexpected and totally non-idiomatic, then I would probably refrain from doing it.

The cleanest way seems to be to use a builder pattern, I guess?

This is documented. Adding another field is considered a breaking change when there are no private fields and no non_exhaustive marker.

Is it though? The problem is that you may get a compiler error in a library you are using, not just in your own code.

1 Like

Well, std (and other sysroot crates) is a little special here, since their version is always known. For any other crate, you only know its exact version if you pinned your Cargo.toml to the exact version (not just specified the dependency, since this will use tilde range specifier by default), and this is something which doesn't happen very often. For sysroot crates, their version is specified by toolchain, and it won't be upgraded unexpectedly. I can see your point, however.

Because this syntax is not special, I guess? That is, you can put any value of the type Struct instead of Default::default(), and it will work the same.

Like I said, strictly speaking it's a guideline:

Of course, it's still not nice, but it happens when the API documentation isn't followed. And in that case, a compiler error is still better than some weird runtime behavior that's difficult to debug.

Specifying how the API must be used and which guarantees can be relied upon is very common for all sort of APIs; not just std, not just Rust. For some of these non-guarantees there exist formalized ways to express it, e.g. by

  • using #[non_exhaustive],
  • omitting + Send in a Future bound (doesn't work with async fn though!),
  • using sealed traits (still feels more like a hack with the current syntax).

But there are also non-guarantees regarding stability for which there is no formalized way of expressing them.

While I agree that it's nice to express as many of these issues in a formalized manner (so the compiler warns you before things go wrong in the future), it's also bad if using these features prohibit the usage of other nice language features.

Yes, but what is the problem with

let s = S { some_field: some_value, .. some_s };

if S is #[non_exhaustive]? some_s must be a valid S (created somehow), so this can't break if S gets new fields added. Why is it rejected then? It makes no sense to me (yet).

Yeah it makes no sense to me either. I would expect this to by syntax sugar for something like:

{
    let mut new_s = some_s;
    new_s.some_field = some_value;
    new_s
}

which does work for non-exhaustive structs.

But apparently it's actually syntax sugar for something like:

    S { some_field: some_value, another_field: some_s.another_field }

which doesn't work when another_field is private or S is non-exhaustive.

My reading of that section is that it's just acknowledging that cargo doesn't and can't check that the guidelines are followed. That is to say, the alternative statement "These are hard-and-fast rules that all projects will obey" just isn't true even if they very much wanted it to be (I know I would!).

My feeling is that there is a strong expectation that you cannot incorrectly use a crate in such a way that a semver-compatible upgrade will result in a compiler error*. Personally, if I hit a situation like that I'd make an issue in the crate's repo regardless of what the documentation said (unless it said "please don't make an issue about this"). They could close the issue of course and that'd be the end of that, but I do think they'd be going against best practices in doing so.

*With some exceptions, for example diesel has a feature called i-implement-a-third-party-backend-and-opt-into-breaking-changes which does what it says on the tin. I see this as similar to using a nightly feature, explicitly opting in for potential breakage.

Yeah, everyone expects that. Sadly it's currently a breaking change to change it to that.

It would be possible to change it over an edition -- the rustfix could perhaps expand the sugar -- but there's no proposal for that right now. I'm not sure if it's a good plan or not.

In the mean time, if you want ..Default::default() that does what you want, you can use this macro:

macro_rules! s {
    ( $($i:ident : $e:expr),* ) => {
        {
            let mut temp = Default::default();
            if false {
                temp // this tricks inference into working
            } else {
                $(
                    temp.$i = $e;
                )*
                temp
            }
        }
    }
}

Which lets you write

let params: BandpassParams = s! { low_freq: 300.0 };

and have it work even with non_exhaustive.

4 Likes

Could you explain this point? I can't think of any kind of code which would break, currently.

    // Works
    let s1 = S { a: String::new(), b: String::new() };
    let s2 = S { a: String::new(), ..s1 };
    let a3 = s1.a;

    // Fails
    let s1 = S { a: String::new(), b: String::new() };
    let s2 = { let mut tmp = s1; tmp.a = String::new(); tmp };
    let a3 = s1.a;

See also.

4 Likes