How to avoid a borrow checker issue without duplicating part of my implementation?

I have a simple Builder class (playground):

trait Tr {}

struct Builder<'a> {
    refs: Vec<&'a dyn Tr>,
}

impl<'a> Builder<'a> {
    fn append_ref(&mut self, tr: &'a dyn Tr) {
        self.refs.push(tr);
    }

    fn build(&self) -> &'a[&dyn Tr] {
        &self.refs
    }
}

This works fine, but callers do not always have a &dyn Tr living as long as 'a. Instead of switching everything to Box<dyn Tr>, I added another method to the Builder which leverages a new field in the builder to store references for a longer time. Here is the new version (playground):

struct Builder2<'a> {
    refs: Vec<&'a dyn Tr>,
    owned: Vec<Box<dyn Tr>>,
}

impl<'a> Builder2<'a> {
    fn append_ref(&'a mut self, tr: &'a dyn Tr) {
        self.refs.push(tr);
    }

    fn append_owned(&'a mut self, tr: Box<dyn Tr>) {
        self.owned.push(tr);
        let just_added: &Box<dyn Tr> = self.owned.last().unwrap();
        self.refs.push(just_added.as_ref());
    }

    fn build(&self) -> &'a[&dyn Tr] {
        &self.refs
    }
}

(Side note: I would have preferred append_owned to take a &dyn Tr and do the boxing itself, but I didn't manage to make it work...).

Builder2 compiles, but the implementation of append_ref is duplicated in append_owned. It is not a problem in the simplified example above, but my actual builder has more code, which I don't want to duplicate. Unfortunately, replacing self.refs.push(just_added.as_ref()); with self.append_ref(just_added.as_ref()); does not compile anymore (playground):

error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
  --> src/lib.rs:16:9
   |
8  | impl<'a> Builder2<'a> {
   |      -- lifetime `'a` defined here
...
15 |         let just_added: &Box<dyn Tr> = self.owned.last().unwrap();
   |                                        ---------- immutable borrow occurs here
16 |         self.append_ref(just_added.as_ref());
   |         ^^^^^^^^^^^^^^^^-------------------^
   |         |               |
   |         |               cast requires that `self.owned` is borrowed for `'a`
   |         mutable borrow occurs here

IIUC the issue, the compiler has no way to know that append_ref does not mess with self.owned.

So my questions are:

  1. Is there a way to avoid duplicating the append_ref implementation, preferably without using unsafe code?
  2. Are there better ways to solve the initial problem (callers not having a trait object with a long enough lifetime)?
  3. [Bonus, could be a separate question] Is there a way for append_owned to take a &dyn Tr?

Using a lifetime in &'a mut self is never what you want. The 'a lifetime is a generic parameter, so it must be longer than the lifetime of the struct itself, but this means that calling the function borrows self for the full duration of selfs lifetime, which means that you can never access the Builder2 in any other way ever again after calling a &'a mut self function.

Generally what you are trying to do is flawed. The type &'a dyn Tr means "a reference to a Tr owned by someone else", and you simply cannot give it a reference to something owned by self because that's in contradiction with what having the lifetime like that means: it is owned by something else.

Additionally, append_owned certainly cannot take &dyn Tr. You cannot take ownership of something you are given a borrow of.

I strongly recommend going the all owned route instead.

struct Builder2 {
    owned: Vec<Box<dyn Tr>>,
}

impl Builder2 {

    fn append_owned(&mut self, tr: Box<dyn Tr>) {
        self.owned.push(tr);
    }

    fn build(&self) -> &[Box<dyn Tr>] {
        &self.owned
    }
}
5 Likes

Traditionally, the build method will return an owned object instead of a reference, and may consume itself in the process. The user then doesn’t need to keep the builder object around after it’s done its job.

@alice’s approach of going fully owned is probably the right one. If you really want to go with the mixed borrowed/owned approach, however, you can use an enum to do that (such as borrow::Cow):

use std::borrow::Cow;

// 'static here requires implementors to fully own their data
// the bound prevents some confusing lifetime errors elsewhere
pub trait MyTrait:'static {
    fn clone_dyn_mytrait(&self)->Box<dyn MyTrait>;
}

impl ToOwned for dyn MyTrait {
    type Owned = Box<dyn MyTrait>;
    fn to_owned(&self)->Self::Owned { self.clone_dyn_mytrait() }
}

pub struct Builder3<'a> {
    refs: Vec<Cow<'a, dyn MyTrait>>,
}

impl<'a> Builder3<'a> {
    fn append_cow(&mut self, cow:Cow<'a, dyn MyTrait>) {
        self.refs.push(cow);
    }

    pub fn append_ref(&mut self, x: &'a dyn MyTrait) {
        self.append_cow(Cow::Borrowed(x));
    }

    pub fn append_box(&mut self, x: Box<dyn MyTrait>) {
        self.append_cow(Cow::Owned(x));
    }

    pub fn append(&mut self, x:impl MyTrait) {
        self.append_box(Box::new(x));
    }

    pub fn build_refs(self)->Vec<impl std::ops::Deref<Target=dyn MyTrait>+'a> { self.refs }

    pub fn build_owned(self)->Vec<Box<dyn MyTrait>> {
        self.refs.into_iter().map(|x| x.into_owned()).collect()
    }
}

(Playground)

1 Like

Thanks for this insight. You saved me a lot of frustration later on, trying to understand why it doesn't work :slight_smile:

True, but I was hoping to use ToOwned or something similar.

This is the first time I see this syntax. Where can I read more about it?
It's not mentioned here, although it looks similar to the second case.

You are probably right, but I would like to understand how you reached this conclusion, so that I can do the same on my own in the future. Could you share your thought process, please?
Which characteristics of the original problem make it so that a non-owned route would be difficult or impossible?

Thanks, I learned a lot from this code (starting from the existence of Cow :slight_smile:).

Unfortunately I don't think I can apply your suggestion, because I don't control the trait. In real code, my Tr trait is rusqlite::ToSql. The builder is intended to hold SQL query parameters, so that its build() method can return what other rusqlite methods (such as this one) expect, i.e. an IntoIterator, whose Item implements rusqlite::ToSql.

I think this implies that the builder should hold trait objects, but until I saw your code I didn't realize that returning Vec<Box<dyn Tr>> would also work!

It’s the same as the second case, but specified as a supertrait, so it’s guaranteed to hold for all instances of MyTrait.

Usually, my thinking goes the other way: the owned solution is almost always simpler and easier, but sometimes comes with undesirable drawbacks— Is there something about the problem being solved that demands the headache of getting temporary borrows right?

There’s always a way, but it just keeps getting more complicated. It looks like ToSql is implemented for most std smart pointers, so you should be able to wrap it pretty arbitrarily and still have everything work.

For example, you could write something like this, and then you’ve got something that can be used like MyTrait above (untested):

trait CloneableToSql {
    fn as_sql(&self)->&dyn ToSql;
    fn boxed(&self)->Box<dyn CloneableToSql>;
}

struct BorrowedSql<'a, Sql> {
    borrow: &'a Sql
}

impl<'a, Sql: ToSql+Clone> CloneableToSql for BorrowedSql<'a, Sql> {
    fn as_sql(&self)->&dyn ToSql { &self.borrow }
    fn boxed(&self)->Box<dyn CloneableToSql> { Box::new( OwnedSql( self.borrow.clone())) }
}

#[derive(Clone)]
struct OwnedSql<Sql>(Sql);

impl<Sql: ToSql+Clone> CloneableToSql for OwnedSql<Sql> {
    fn as_sql(&self)->&dyn ToSql { &self.0 }
    fn boxed(&self)->Box<dyn CloneableToSql> { Box::new( self.clone() ) }
}

impl<CS: CloneableToSql + ?Sized> ToSql for CS {
    fn to_sql(&self) -> Result<ToSqlOutput<'_>> { self.as_sql().to_sql() }
}

Oversimplified:

struct Builder<'a> {

Putting 'a on Builder says that 'a is the lifetime of some borrows that get stored inside Builder. This lifetime has to actually be longer than the Builder's own lifetime, because all references have to always be valid when used.

    refs: Vec<&'a dyn Tr>,

So the things in refs borrow some things that live for at least 'a, which again, is longer than the lifetime of the Builder itself. It doesn't make sense to try to put a reference to self inside self.refs, because self won't last for 'a.

It can be possible to work around such limitations using unsafe, but not with a build method that returns &'a [&dyn Tr] because the owned dyn Trs don't live for 'a, regardless of how long the borrowed ones live.

These problems pretty much vanish if Builder just doesn't have a lifetime parameter at all -- i.e., it doesn't borrow anything. This is generally the easiest way to get things going, and is similar to how you might work around similar problems in other languages (possibly with a dose of Rc/Arc to reduce copying). So hopefully that partially explains why going fully owned should probably be your first choice.

If you start with @alice's solution, but you really need to put borrowed items in Builder, you can still do that by putting a lifetime on it -- not using &'a dyn ToSql, but Box<dyn ToSql + 'a>! This is slightly more magical, but it does work because, as @2e71828 observed, &'a SomeType where SomeType: ToSql is a type that implements ToSql. (Still less confusing than trying to use unsafe.)

1 Like

Rust is designed to work with values by default. If you need indirection, or want to optimize some operations, passing stuff around by reference requires extra thought. If you don't succeed using references, try falling back to values.

1 Like

In principle it would be possible if you did this:

trait Tr {
    fn clone_tr(&self) -> Box<dyn Tr>;
}

But this would not be taking ownership, rather it would be a clone. It also doesn't work if you don't own the trait.

Your post looks like you're trying to do some sort of object oriented thing, and references in structs are a mistake in this context.

It's possible that it can make sense now that you've told us that Tr is ToSql. That rusqlite makes you use trait objects in this manner in the first place is honestly a mistake in the design of the library, and as a user, that design can force you to do some otherwise unusual things.

Looking at the API design a little closer, ToSql has a single method that returns a Result<ToSqlOutput, _>, and enum ToSqlOutput also implements ToSql. So you always have the option of preemptively realizing the SQL values and storing that instead:

(untested)

struct Builder<'a> {
    values: Vec<ToSqlOutput<'a>>
}

impl<'a> Builder {
    fn append(&mut self, sql: &'a impl ToSql)->Result<(), rusqlite::Error> {
        self.values.push(sql.to_sql()?);
        Ok(())
    }

    fn build(&self)->&[impl ToSql + 'a] {
        &self.values
    }
}
1 Like

@2e71828 I mean, when the library has functions that explicitly use &dyn ToSql in the argument type (e.g. this one), you can't actually use ToSqlOutput, as that is a different type from &dyn ToSql, even if it implements the ToSql trait.

Maybe there is some way to use it in an ergonomic way, but I have seen people run into trouble with this trait enough times that I have to conclude that the design is bad.

2 Likes

Fair enough; I didn’t go looking much beyond the ToSql trait and assumed the rest of the API was built around generics. That was apparently a bad assumption to make :frowning: .

1 Like

Generally my experience with the library is that you have to build two vectors for every query you want to make (unless you hard-code the number of parameters in the source). One vector for owning the values, and another vector that stores &dyn ToSql references into the first vector. Needing two vectors in this manner seems to trip up a lot of people.

The root issue seems to be here. If bind_parameter was declared with impl ToSql instead of &dyn ToSql, there’d be no need to ever construct a dyn ToSql object. Alternatively, the public-facing methods could turn an &impl ToSql into an &dyn ToSql for internal use.

Am I correct in thinking neither of these would be a breaking change because the methods are becoming more lenient in what they accept?

Not obviously, but I don't know for certain. You could probably run into trouble with the compiler having less information for type inference, and no longer knowing which type to use in some contexts.

1 Like

Thanks everyone for the helpful answers!

My initial concern with the fully-owned API is that it forces to use Box::new() every time, even when I already have an owned value whose ownership could be transferred (or cloned) or when I have a reference with a long enough lifetime. I find this a bit inelegant (and maybe not as efficient as it could be, even though it doesn't matter for my use case), but that might just be my lack of experience with Rust.

Right now, my implementation looks like this:

trait ToSql {}

type BoxedToSql = Box<dyn ToSql>;

struct Builder {
    params: Vec<BoxedToSql>,
}

impl Builder {
    fn append_owned(&mut self, param: impl ToSql + 'static) {
        self.params.push(Box::new(param));
    }

    fn build(self) -> impl IntoIterator<Item = BoxedToSql> {
        self.params
    }
}

I guess I can still use the approach proposed by @2e71828 as a more nuanced solution, if I ever run into performance issues.