KittyCAD / modeling-api

KittyCAD Modeling API
MIT License
6 stars 1 forks source link

Instantiate structs via builders, not fieldwise #519

Open adamchalmers opened 2 months ago

adamchalmers commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.