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

Allow invariant checks in .build() #67

Open
Banyc opened this issue May 22, 2022 · 10 comments · May be fixed by #95
Open

Allow invariant checks in .build() #67

Banyc opened this issue May 22, 2022 · 10 comments · May be fixed by #95

Comments

@Banyc
Copy link

Banyc commented May 22, 2022

I want something like this:

pub struct S {
    x: i32,
    y: i32,
}

impl S {
    fn check_rep(&self) {
        assert!(self.x < self.y);
    }
}

When .build() is called, I also want check_rep to be called.

@benluelo
Copy link

Just stumbled upon this issue while looking through the repo, but you could do something like this:

pub struct S {
    xy: Xy
}

pub struct Xy {
    x: i32,
    y: i32,
}

impl Xy {
    pub fn new(x: i32, y: i32) -> Result<Self, Whatever> {
        if x < y {
            Ok(Self { x, y })
        } else {
            Err(/* whatever */)
        }
    }

    // also add some getters for x and y
}

i.e. just encode the invariants into the type system

@Banyc
Copy link
Author

Banyc commented Aug 21, 2022

@benluelo its like a builder pattern and I have another idea:

pub struct S {
    x: i32,
    y: i32,
}

pub struct SBuilder {
    pub x: i32,
    pub y: i32,
}

impl SBuilder {
    pub fn build(self) -> Result<S, Whatever> {
        if self.x < self.y {
            Ok(S { x, y })
        } else {
            Err(/* whatever */)
        }
    }
}

@mo8it
Copy link
Contributor

mo8it commented Jun 7, 2023

I would implement it like this:

fn build(self) -> Result<S, Whatever> {validate(S {x, y})
}

Where validate is a function pointer fn(S) -> Result<S, Whatever> that is specified as a struct builder attribute #[builder(prebuild = "validate")].

I would call it something like prebuild instead of validator because it can be used for post-processing and not only validation.

@idanarye Does this look fine to you?

@Techassi
Copy link
Contributor

I can tackle this if @idanarye approves.

@idanarye
Copy link
Owner

What's the signature of validate? How does the TypedBuidler macro know what the error type is? Also, why prebuild when it happens after the struct is built? Shouldn't it be postbuild?

@idanarye
Copy link
Owner

Since the crate is split now and we can bring in regular structs, who about something like this:

#[derive(TypedBuilder)]
#[builder(postbuild)]
struct S {
    x: i32,
    y: i32,
}

impl PostBuild for S {
    type Output = Result<Self, Whatever>;

    fn process(self) -> Self::Output {
        validate(&self)?; // or just do the validation here
        Ok(self)
    }
}

#[builder(postbuild)] will tell the macro to use PostBuild::Output and PostBuild::process. Alternatively, the macro could always use the PostBuild trait, but without #[builder(postbuild)] it would generate a simple implementation of it (and when Rust finally stabilizes specialization we could have a blanket implementation and remove this setting)

@mo8it
Copy link
Contributor

mo8it commented Jun 16, 2023

Nice approach! Using PostBuild only when the attribute is specified sounds better to me.

@idanarye
Copy link
Owner

I wonder about that. I think always using PostBuild and generating one when the setting is not passed will result in simpler code because we won't have to conditionally change the generated code of the build method.

@Techassi
Copy link
Contributor

Yeah I agree, we should always use the PostBuild::Output. This simplifies the code generation for the .build() method. I will open a PR and will start working on this.

@Techassi Techassi linked a pull request Jun 19, 2023 that will close this issue
@Techassi
Copy link
Contributor

Techassi commented Jun 19, 2023

I created an initial draft PR over at #95 which implements this feature. It should be noted that it currently is a WIP PR.

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

Successfully merging a pull request may close this issue.

5 participants