Support for builder pattern?

[therealprof]

I was looking into creating a Rhai module for some new API I'm probably going to write (the current API is super complex and not really suitable for use in Rhai) and wanted to use a builder pattern for that. Unfortunately due to the &mut self requirement on functions, this would require to return a clone() of self at the end of each setter which is not only wasteful and inefficient but also means I have two live objects which opens up a lot of room for confusion and potential misuse.

Would there be a way to also allow a signature of fn build (self) -> SelforOther?

[schungx]

Usually it such cases we would wrap the builder object in Rc<RefCell<T>> such that cloning is cheap.

#[export_module]
mod build_api {
    pub type Builder = Rc<RefCell<BuilderObject>>;

    pub fn new() -> Builder { Rc::new(RefCell::new(BuilderObject::new())) }

    pub fn do_something(builder: &mut Builder) -> Builder {
        let b = &*builder.borrow_mut();
        b.do_something();
        builder.clone()
    }
}

[therealprof]

I think I understand your idea here, basically allowing the real Rust API to be whatever it wants to be and using interior mutabiltity to allow the Rhai module have it its own way.

However, that's not quite was I was asking for: Indeed it might be a bit better to have a proper Rust API independent of Rhai and have the Rhai module be something like a singleton proxy. But having to wrap each and every builder method (of which there might be dozens per builder) could end up being a significant and not quite ergonomic maintenance burden.

[schungx]

That's true but unfortunately hard to avoid.

Rust is a typed compiled language while Rhai is dynamic and interpreted. Their APIs are naturally at odds.

You'll find that even if you can expose the same API structure in Rhai, it won't feel natural or idiomatic... As it probably won't take advantage of dynamic language features, such as overloading and returning dynamic valurs.

In the end you'll be rewrapping your API in Rhai anyway, that's my experience.

The good thing is... In many cases the flexibility of an overloaded dynamic language actually makes your API simpler -- Rust APIs tend to be long and cumbersome due to the lack of overloading, default parameters, no exceptions (meaning you have to always return result codes) and strict typing.

I suggest you mock it out first and make your own judgement.

[therealprof]

@schungx Okay, that kind of works for a trivial testcase, though it is quite iffy.

However it does not work when wrapping an external library in an external package. Even if I expose the builder types (which I really don't want to do), I run into a problem due to the signature of the builder methods which are all like: pub fn with_max_depth(mut self, max_depth: u32) -> Self the mut here throws a real curveball... the type would either need to be Copy (which it isn't) or I'll get a friendly error from the compiler: error[E0507]: cannot move out of *b which is behind a mutable reference. 😢

[schungx]

Rhai data must be Clone.

So if your builder is not clonable (why not? Since it is only a builder which probably only contains config data) you need to wrap it in Rc<RefCell>.

[therealprof]

That is quite literally what I tried:

      pub type GetBuilder = Rc<RefCell<usp_builder::GetBuilder>>;

      pub fn get_builder() -> GetBuilder {
          Rc::new(RefCell::new(usp_builder::GetBuilder::new()))
      }

      #[rhai_fn(global,pure)]
      pub fn foo(builder: &mut GetBuilder) -> GetBuilder {
          builder.borrow_mut().with_max_depth (0);
          builder.clone()
      }

Error:

error[E0507]: cannot move out of dereference of `RefMut<'_, rusp::usp_builder::GetBuilder>`
   --> src/lib.rs:37:9
    |
37  |         builder.borrow_mut().with_max_depth (0);
    |         ^^^^^^^^^^^^^^^^^^^^ ------------------ value moved due to this method call
    |         |
    |         move occurs because value has type `rusp::usp_builder::GetBuilder`, which does not implement the `Copy` trait
    |
note: `rusp::usp_builder::GetBuilder::with_max_depth` takes ownership of the receiver `self`, which moves value

[schungx]

I'm going to add this to the Book as an example:

The trick is to extract the type, modify it, and then set it back.

/// Builder for `Foo` instances.
/// Notice that this type does not need to be `Clone`.
pub struct FooBuilder {
    /// The `foo` option.
    foo: i64,
    /// The `bar` option.
    bar: bool,
    /// The `baz` option.
    baz: String,
}

impl FooBuilder {
    /// Creates a new builder for `Foo`.
    pub fn new() -> Self {
        Self { foo: 0, bar: false, baz: String::new() }
    }
    /// Sets the `foo` option.
    pub fn with_foo(mut self, foo: i64) -> Self {
        self.foo = foo;
        self
    }
    /// Sets the `bar` option.
    pub fn with_bar(mut self, bar: bool) -> Self {
        self.bar = bar;
        self
    }
    /// Sets the `baz` option.
    pub fn with_baz(mut self, baz: &str) -> Self {
        self.baz = baz.to_string();
        self
    }
    /// Builds the `Foo` instance.
    pub fn build(self) -> Foo {
        Foo { foo: self.foo, bar: self.bar, baz: self.baz }
    }
}

/// Builder for `Foo`.
#[export_module]
pub mod foo_builder {
    use super::{Foo, FooBuilder as BuilderImpl};
    use std::cell::RefCell;
    use std::mem;
    use std::rc::Rc;

    /// The builder for `Foo`.
    // This type is `Clone`.
    pub type FooBuilder = Rc<RefCell<Option<BuilderImpl>>>;

    /// Creates a new builder for `Foo`.
    pub fn create() -> FooBuilder {
        Rc::new(RefCell::new(Some(BuilderImpl::new())))
    }
    /// Sets the `foo` option.
    #[rhai_fn(global, pure)]
    pub fn with_foo(builder: &mut FooBuilder, foo: i64) -> FooBuilder {
        let b = &mut *builder.borrow_mut();
        let obj = mem::take(b).unwrap();
        *b = Some(obj.with_foo(foo));
        builder.clone()
    }
    /// Sets the `bar` option.
    #[rhai_fn(global, pure)]
    pub fn with_bar(builder: &mut FooBuilder, bar: bool) -> FooBuilder {
        let b = &mut *builder.borrow_mut();
        let obj = mem::take(b).unwrap();
        *b = Some(obj.with_bar(bar));
        builder.clone()
    }
    /// Sets the `baz` option.
    #[rhai_fn(global, pure)]
    pub fn with_baz(builder: &mut FooBuilder, baz: &str) -> FooBuilder {
        let b = &mut *builder.borrow_mut();
        let obj = mem::take(b).unwrap();
        *b = Some(obj.with_baz(baz));
        builder.clone()
    }
    /// Builds the `Foo` instance.
    #[rhai_fn(global, pure)]
    pub fn build(builder: &mut FooBuilder) -> Foo {
        let b = &mut *builder.borrow_mut();
        mem::take(b).unwrap().build()
    }
}

[therealprof]

That looks much better...

However meanwhile I figured out, there's a much simpler way of wrapping a builder pattern in an external module and I'm wondering whether this is incorrect somehow...

My current experiment looks like this:

    #[rhai_fn(global, pure)]
    pub fn with_max_depth(builder: &mut GetBuilder, max_depth: i64) -> GetBuilder {
        *builder = builder.clone().with_max_depth(max_depth as u32);
        builder.clone()
    }

    #[rhai_fn(global, pure)]
    pub fn with_params(builder: &mut GetBuilder, params: Array) -> GetBuilder {
        *builder = builder
            .clone()
            .with_params(params.into_iter().map(|e| e.cast::<String>()).collect());
        builder.clone()
    }

    #[rhai_fn(global, return_raw)]
    pub fn build<'a>(builder: GetBuilder) -> Result<rusp::usp::Body<'a>, Box<EvalAltResult>> {
        Ok(builder.build().map_err(|e| e.to_string())?)
    }

and works great with this Rhai code...

print (get_builder().with_max_depth (1).with_params (["Device."]).build());

[schungx]

This cannot work if with_max_depth takes mut self though...

Because builder.clone() returns an Rc<RefCell<GetBuilder>> which derefs to &self not self...

It works if GetBuilder is a simple type that is Clone, not Rc<RefCell<T>>...

[therealprof]

It works if GetBuilder is a simple type that is Clone, not Rc<RefCell<T>>...

It is, also I would expect builders to usually be Clone.

The Rc<RefCell<T>> was your suggestion to work around the &mut self requirement on manually registered methods. However, you just made me realise that with the simplified approach, I can can get rid of the cloning altogether...

Now the rhai wrapper looks like:

    pub type GetBuilder = usp_builder::GetBuilder;

    pub fn get_builder() -> GetBuilder {
        usp_builder::GetBuilder::new()
    }

    #[rhai_fn(global)]
    pub fn with_max_depth(builder: GetBuilder, max_depth: i64) -> GetBuilder {
        builder.with_max_depth(max_depth as u32)
    }

    #[rhai_fn(global)]
    pub fn with_params(builder: GetBuilder, params: Array) -> GetBuilder {
        builder.with_params(params.into_iter().map(|e| e.cast::<String>()).collect())
    }

    #[rhai_fn(global, return_raw)]
    pub fn build<'a>(builder: GetBuilder) -> Result<rusp::usp::Body<'a>, Box<EvalAltResult>> {
        Ok(builder.build().map_err(|e| e.to_string())?)
    }

Sweet! 🕺🏻

[schungx]

Correct. If the builder type is Clone, which it usually is, then it gets trivial:

/// Builder for `Foo` instances.
#[derive(Clone)]
pub struct FooBuilder {
    /// The `foo` option.
    foo: i64,
    /// The `bar` option.
    bar: bool,
    /// The `baz` option.
    baz: String,
}

/// `FooBuilder` API which uses moves.
impl FooBuilder {
    /// Creates a new builder for `Foo`.
    pub fn new() -> Self {
        Self { foo: 0, bar: false, baz: String::new() }
    }
    /// Sets the `foo` option.
    pub fn with_foo(mut self, foo: i64) -> Self {
        self.foo = foo;
        self
    }
    /// Sets the `bar` option.
    pub fn with_bar(mut self, bar: bool) -> Self {
        self.bar = bar;
        self
    }
    /// Sets the `baz` option.
    pub fn with_baz(mut self, baz: &str) -> Self {
        self.baz = baz.to_string();
        self
    }
    /// Builds the `Foo` instance.
    pub fn build(self) -> Foo {
        Foo { foo: self.foo, bar: self.bar, baz: self.baz }
    }
}

let mut engine = Engine::new();

engine
    .register_fn("get_builder", FooBuilder::new)
    .register_fn("with_foo", FooBuilder::with_foo)
    .register_fn("with_bar", FooBuilder::with_bar)
    .register_fn("with_baz", FooBuilder::with_baz)
    .register_fn("build", FooBuilder::build);

[therealprof]

Oh! I had the completely wrong impression then... The documentation (e.g. https://rhai.rs/book/rust/methods-fn-call.html#first-mut-parameter) kind of reads as if the first parameter had to be a mutable reference. Totally didn't see that coming and that must have been the only thing I hadn't tested. 😅

Thanks for the explanations, things are a lot clearer now. Feel free to close the issue.

[schungx]

Usually builders are just a bag of properties, most of them primary types (strings in Rhai are shared so they clone very rapidly). So cloning the builder type is usually not a performance concern.

[therealprof]

I agree. That's why I think it is important to support this pattern and document it as best as possible. The documentation is currently not very good at making this clear which is why this conversation is rather long winded and took a lot of experiementation.

BTW: I'm absolutely thrilled by the diagnostics offered by the builder pattern implementation in Rhai... e.g.:

6: let record = rusp::record_builder().build();
                                       ^ Runtime error: Cannot produce USP Record without to_id