Incremental borrow accumulation = anti-pattern?

I hit upon the following pattern in my code, and am looking for a workaround.

Consider an impl like:

impl<'a, T> IncrementalBorrower<'a, T> {
  pub fn add(&'a mut self, x: &'a T) {...}

  pub fn do_something(&self) {...}
}

Where the intention is to allow interspersed calls to the add and do_something methods, with new non-static Ts being created (elsewhere) during the IncrementalBorrower's lifetime, and some borrowed by it.

I think this is a Rust anti-pattern, even though it seems very innocent. In languages without borrow checking, there would be no problems. Safety would be based on proper usage (don't delete borrowed T's).

The reason I think it is a Rust anti-pattern is that those Ts have to be owned somewhere. The add method calls borrow from that owner. Which means that T owner is immutable for the duration of the lifetime of the IncrementalBorrower, and so that owner can't accumulate new Ts. This happens whether there is a single such owner of Ts or multiple ones. The implication is that all Ts must be owned by those owners prior to creation of the IncrementalBorrower - violating the intention.

What's the workaround for this anti-pattern? One may be to rewrite IncrementalBorrower so that it owns (or shares ownership as with Rc<T>) the Ts. Even though it has no use for ownership per se.

Also it's possible that the T owner is exotic enough to accumulate new Ts while allowing existing ones to be borrowed. Some type of append-only collection?

In my case there is also a remove method, which makes things even more difficult.

Is there a workaround that avoids the need for any ownership by IncrementalBorrower or exotic T ownership?

Yes, this is quite possible and reasonable. typed-arena is an example of a library that allows it, and there are others. Another case where accumulation of borrows is possible is in a recursive function — it's fine to borrow from many parent stack frames.

But there are also many cases where you can't choose one of these options because other constraints won’t allow it, and so trying to accumulate borrows instead of owned or shared (Rc/Arc) values is the wrong choice.

This should be seen as a specific case of a general principle, an anti-pattern if you like: in most cases, you should not build your primary data structures — the ones that store your application’s data, and are mutated over significant time — out of borrows, but only ownership. (You haven’t said what IncrementalBorrower is actually doing, so I can’t say whether this is actually one of those cases or not.)


However, you do have an unrelated serious problem in your function signature:

impl<'a, T> IncrementalBorrower<'a, T> {
  pub fn add(&'a mut self, x: &'a T) {...}

You must not use the same lifetime for the &mut Self borrow as for the data stored in Self. When you do this, it ends up meaning that the IncrementalBorrower must be borrowed for 'a — which, since it contains 'a, means it is borrowed for the rest of its own existence, and cannot be used further once borrowed in this way. You should remove the 'a from &mut self. Whatever problem you are trying to solve, don’t solve it that way.

If you share more code and tell us more about what goal these borrows are serving, we can help you write it in a better way.

8 Likes

It is and it does. This is the same pattern as described in Borrowing something forever - Learning Rust: &'a mut self in add() is shorthand for &'a mut IncrementalBorrow<'a, T>. Recognizing when &'a mut Thing<'a> is happening, even when not entirely obvious, will go a long way to avoiding this issue.

Note that it's the invariant lifetime 'a that is at issue, not the other generic parameter T.

Arenas can work with &'a Thing<'a> because the lifetime in this case is covariant.

2 Likes

Apart from the mistake of using 'a for &mut self, I think it is an anti-pattern when used for long term borrows, but sometimes not for short term borrows. In the latter case it is considered a "view struct" and is not uncommon.

The idea is that the view struct is used to temporarily access the owned values (via references) and the owned values are not accessed directly during that time. I don't think this is the use case you're describing, but I wanted to mention that borrowing like this is sometimes fine and can be successfully used to avoid passing large numbers of parameters.

Of course, long term and short term are relative, and whether this works out as a view struct depends on the whole picture.

1 Like

Maybe I generalized this incorrectly with the &'a mut self? But the point I was trying to make is that IncrementalBorrower may keep those borrows for its lifetime (or until corresponding remove calls), not just the lifetime of the add calls. The borrows are needed during the do_something calls.

In my particular use case, the IncrementalBorrower is a safe wrapper around epoll that keeps fds borrowed during the lifetime of the epoll (which rustix's epoll doesn't do itself). It's more like:

pub struct SafeEpoll<T: AsFd> {
  fds_to_monitor: Vec<T>,
  ...
}

impl<T : AsFd> SafeEpoll<T> {
  pub fn add(&mut self, fd: T) { ... }
  ...
}

This gives the caller the option of using OwnedFd, BorrowedFd<'a> (where 'a is at least the lifetime of the SafeEpoll), or something even more elaborate (like Rc<OwnedFd>) for T. I'm trying to avoid the Rc<OwnedFd> case, mostly because it seems like overkill. The BorrowedFd<'a> case induces the undesired anti-pattern discussed. If I use OwnedFd, I would also have to allow (due to the rest of my app) the user of SafeEpoll to borrow back those fds. That's somewhat awkward, but might be possible. I'm working through that right now. I was wondering however if I could avoid that pain.

This is in a server app, so the fds (connections to clients) come and go frequently.

Maybe Rc<OwnedFd> is the best solution?

This looks fine, compared to what you had before?

impl<T : AsFd> SafeEpoll<T> {
  pub fn add(&mut self, fd: T) { ... }

Versus

impl<'a, T> IncrementalBorrower<'a, T> {
  pub fn add(&'a mut self, x: &'a T) {...}

Note that &T: AsFd when T: AsFd, so the consumer can choose to use &T (or Rc<T> or...), or not.

2 Likes

You express that ability via declaring the lifetime on the struct instead of the function, impl<'a> IncrementalBorrower<'a>, not by putting that lifetime in the borrow of the IncrementalBorrower, &'a mut self. The former is useful; the latter will destroy your ability to use the struct for anything interesting.

5 Likes

that's not actually true, for a lot of uses, the lifetime is invariant. the important thing is that &'a T is Copy. e.g.:

#[derive(Debug)]
pub struct ListNode<'a> {
    pub data: u32,
    // lifetime is invariant since it's inside a cell
    pub next: RefCell<Option<&'a ListNode<'a>>>,
}

fn main() {
    let arena = typed_arena::Arena::new();
    let head = RefCell::new(None);
    let mut tail = &head;
    for v in [1, 2, 3] {
        let node: &_ = arena.alloc(ListNode {
            data: v,
            next: RefCell::new(None),
        });
        *tail.borrow_mut() = Some(node);
        tail = &node.next;
    }
    println!("{head:#?}");
}