Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instantiate structs via builders, not fieldwise #519

Open
adamchalmers opened this issue Sep 17, 2024 · 4 comments
Open

Instantiate structs via builders, not fieldwise #519

adamchalmers opened this issue Sep 17, 2024 · 4 comments

Comments

@adamchalmers
Copy link
Collaborator

adamchalmers commented Sep 17, 2024

Problem

Say a user has instantiated a ModelingCmd struct field-by-field (i.e. not using a constructor function, but rather, providing each field), like

let x = Export {
  entity_ids: Vec:new(),
  output_format: OutputFormat::Stl,
};

Problem is, adding a new field is not backwards-compatible. The above code will break if we add a third field, e.g. "compression: bool". Users will now get

missing field `compression` in initializer of `Export`

Even if we add the field with #[serde(default)], the error will still happen. Serde can make JSON deserialization backwards-compatible, but not instantiating the struct in Rust code.

Solution

One solution is to simply stop users from instantiating this way. There's a #[non_exhaustive] attribute we could put on a struct definition, which prevents users from instantiating it field-by-field. That forces users to use constructor methods. Currently we don't have any constructor methods, and defining our own constructor methods for each type would be a lot of work -- we have 60+ commands, so I don't want to handwrite a constructor for each. We have to generate them via a macro.

Luckily such a macro already exists -- the bon crate, which makes type-safe builders for structs. E.g.

use bon::Builder;
use serde::Deserialize;

#[derive(Builder, Deserialize)]
#[non_exhaustive]
struct Export {
    entity_ids: Vec<i32>,
    output_format: OutputFormat,
    #[serde(default)]
    #[builder(default)]
    compression: bool,
}

#[derive(Deserialize)]
enum OutputFormat {
    Stl,
    Fdx,
}

fn main() {
    let f = Export::builder()
        .entity_ids(Vec::new())
        .output_format(OutputFormat::Stl)
        .build();
}

In this example, the compression field is optional and will default to the default value of that field's type (i.e. false). So users don't have to call the .compression() method. But if they leave off a required method like .entity_ids() they'll get a compile error:

can't finish building yet; the member `ExportBuilder__output_format` was not set

This way, we can add new fields to the struct without breaking users -- we can default them to a certain value.

@alteous
Copy link
Contributor

alteous commented Sep 17, 2024

The other way is to provide a function to fill in any remaining values with sensible defaults. Typically Default will suffice for this.

#[derive(Default)]
struct Export {
    entity_ids: Vec<i32>,
    output_format: OutputFormat,
    compression: bool,
}

let args = Export {
    entity_ids: vec![1,2, 3],
    output_format: OutputFormat::Stl,
    compression: false,
    ..Default::default()
};

@alteous
Copy link
Contributor

alteous commented Sep 17, 2024

I have the same problem in the gltf crate. A difficulty arises when there is a member that has no safe or realistic default value, such as a memory offset.

@Veetaha
Copy link

Veetaha commented Sep 17, 2024

Using structs for this purpose has a lot of drawbacks, that's why builders generally prevail. See some more info about this in a related blog post (although it's more focused on named function arguments specifically, but it's still relevant)

@adamchalmers
Copy link
Collaborator Author

I have the same problem in the gltf crate. A difficulty arises when there is a member that has no safe or realistic default value, such as a memory offset.

For existing commands with an existing mandatory value, or new commands with a mandatory value, the bon crate can force users to give a value.

When adding a new mandatory value to an existing struct with no reasonable default, well, there's no way to do that without breaking semver. And that's OK -- we can just release a new version and get users to bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants