How to make this vector of reference wrappers more ergonomic?

code review
1

I have an Extension struct that wraps a shared library and some other associated state. For convenience, I've created an ExtensionRef struct that wraps an extension along with its name (for ease of logging). It looks like this:

pub struct ExtensionRef
{
	name: String,
	extension: RefCell<Extension>,
}

impl ExtensionRef
{
	// Among other functions:

	pub fn get_extension(&self) -> Result<Ref<'_, Extension>, BorrowError>
	{
		return self.extension.try_borrow();
	}

	pub fn get_extension_mut(&self) -> Result<RefMut<'_, Extension>, BorrowMutError>
	{
		return self.extension.try_borrow_mut();
	}
}

Finally, I have an ExtensionList class which holds all the extensions I've loaded. It's really just a convenience for scanning a directory, loading shared libraries and checking that they expose a compatible interface; those that do are stored in a Vec<ExtensionRef>.

pub struct ExtensionList
{
	extensions: Vec<ExtensionRef>,
}

I find myself having to do a lot of for ext_ref in list.iter() and apply the same operation to each extension in turn. This means that I have to call ext_ref.get_extension().unwrap() (or ext_ref.get_extension_mut().unwrap()) a lot. Given I can control how this list is being used, and the stored ExtensionRefs are private to the list, it'd be nice to be able to essentially "lock" the list in a mutable or non-mutable way, and work with a set of borrowed &mut Extension or &Extension references respectively.

I have tried turning the Vec<ExtensionRef> into a RefCell<Vec<ExtensionRef>>, and making a sort of iterator that looks like this:

pub struct ExtensionListIterator<'l>
{
	list: Ref<'l, Vec<ExtensionRef>>,
	extension_refs: Vec<Ref<'l, Extension>>,
}

The idea is to get a Ref to the underlying list, get Refs to all the extensions, and have the struct acts as a handle to all of these. While the struct is alive, the list is locked and all extensions are pre-borrowed. However, I'm struggling a bit with the syntax, especially the line which constructs a Vec<Ref<'l, Extension>>:

impl ExtensionList
{
	pub fn get_iterator<'l>(&'l self) -> Result<ExtensionListIterator<'l>, BorrowError>
	{
		let list_ref: Ref<'l, Vec<ExtensionRef>> = self.extensions.try_borrow()?;
		// This definitely isn't correct!
		let extension_refs: Vec<Ref<'l, Extension>> = list_ref.iter().map(|ext_ref| ext_ref.get_extension()).collect()?;
		return Ok(ExtensionListIterator { list: list_ref, extension_refs });
	}
}

Is this the best way to be going about this, or is there a more ergonomic way to achieve what I want?

2

the thing you need to ask yourself, that will define what might be the best way to solve this, is why do you need those refcells.

try_borrow and try_borrow_mut can fail because the values may be borrowed by something else at the same time. so you ideally want to minimize that borrowing to avoid the conflicts.

but your ExtensionListIterator does the opposite of that by forcibly borrowing every thing at the same time.

3

Maybe reduce the OO style and solve the problem more directly.

For a single method/function to return a collection of some items, there is barely any justification for a own type, just wrap a Vec.

This looks like just a tuple: some values belong together.

There shouldn't be any access or storage policy imposed on the caller. It's perfectly fine to choose a struct with named fields instead of an anonymous tuple. But make the fields pub and drop the RefCell.

Additionally, maybe make the name more obvious:

pub struct NamedExtension
{
	pub name: String,
	pub extension: Extension,
}

And the directory scanning function becomes something like this:

pub fn load_extensions(directory: &Path) -> Result<Vec<NamedExtension>, std::io::Error> {
   ...
}

No shallow OO like wrappers that add only burden.

4

I'm not 100% sure this is what you've been asking, but here[1] I do:

  • clean up in general, make a bit simpler

  • use impl trait in return position to return iterators, which keeps the abstraction intact (independence of implementation) while saving you from writing a wrapper type

  • no cell around the Vec as you seemed to want to do

  • I've changed it to use borrow* instead of try_borrow*, for one since I was running this on a rustc that didn't have the latter yet, and secondly because a borrow error like this is usually a programmer error, i.e. panic is the right reaction.

Note how you do get an error (panic with my change to omit the try_*) as expected when having an odd number of list items.[2]

[1] Rust Playground

[2] PS. that is why doing things without cells is preferable if you can do it: if you just make fn extensions_mut take a &mut self then you can omit the cells and mutate the ExtensionRef values directly. Only use the cells if you do need to defer aliasing checks to runtime, because you do something like the zip in my example code.

PPS. if OTOH you need to mutate the list while also holding a shared reference to it in your logger or something, then you will need the cell around the list; but you likely don't need the cell around each item. If this needs to be shared across threads, replace RefCell with Mutex; but of course you can then deadlock instead of panicking. You could replace the Vec with a persistent collection (crates.io: Rust Package Registry) (with Arc around it if not part of the collection abstraction already) and put that into a Mutex, and for the read-only iterator take a clone of the Arc and unlock immediately again, to avoid the dead-lock and latency problems.

5

In my approach, I will use these depending on needs

  • HashMap
  • Vector + HashMap containing keys if iterating over the map is slow for the needs

Like

struct ExtentionMap(HashMap<String, Extension>)

impl ExtensionMap { 
    // the custom methods
}

Or

struct ExtentionList {
    name: Hashmap<String, usize>,
    extention: Vec<Extention>
}

impl ExtentionList {
    
}

To get extention by index : pass the name to the map, it returns the vector index. Pass the index to the vector. But there is 1 indirection :<, but it is faster if the list of extension is many. Because in pure array it will need to use searching algorithm to find it. Though total of extension is mostly just a couple, not the scale of thousand above :<. So needs benchmark based on your case if you want to make sure performance

To iterate the extention, .iterator() can already be used because the extensions are inside a vector that already has iterator implementation. It will be faster than iterating HashMap, because eventhough the HashMap also uses contigous memory in its design, it is still need more operation than a vector

If the extension name is known and fixed, you can change it to enum, then cast the enum to usize then pass it as Vector's index which will be even faster. It works because enum value is actually integer

Consider to change the String to custom owned stack str if the max name len is known. There are crates that provide that (though I hope rust adopt the best owned stack str to std, because curently the std has no way to use no heap, stack str without lifetime annotation without handrolling manual data structure, or use 3rd lib that makes API incompatible if user uses lib A and lib B, like once cell is eventually added to std :<)

6

Thanks for all the input! This code was some of the first I wrote for Rust, early in this project and coming from a C++ background, so I think it's worth me reviewing the fundamentals of the approach

If I can do away with RefCell<Extension> and actually just store bare Extension structs then that would simplify things a lot. The only thing is that I will need to store some kind of reference to an extension in some way - extensions in my project can, for example, register support for particular file formats, so somehow I will need to build a map to go from a file type to a list of extension references that support loading that type. If I can guarantee that the elements and ordering of the extension list will not change once it's constructed (which I most likely can), then I could use plain array indices for this, unless there is a more idiomatic way.

The other minor question I have regarding this is whether calling a function implemented by an extension should be a mutable or non-mutable operation. Strictly speaking, I think it can be non-mutable because the Extension struct itself should not need to mutate any data - all it provides is a collection of extern "C" functions that call into the loaded shared library, and I guess it's up to the library how it manages its internal state. If this is indeed the case, I think I can restrict all of the mutable Extension calls to the setup procedure (such as asking about supported file types and constructing the aforementioned maps), and keep the rest of the calls non-mutable.

7

That also depends on whether those shared libraries support reentrant calls; i.e. you invoke the extension and pass a callback, that gets called and invokes the extension yet again.

If a lib requires you to never do reentrant calls, then use exclusive (mutable) borrows.

8

The problem with RefCell is that you quit the compile time protection of Rust's normal references, and switch to manual and testing dicipline. Because RefCell does not make you can write code that violates the reference rules. RefCell makes the code that actually violate the reference rules is compiled successfully, and then you may think oh it works because it is compiled successfully, it simplify thing, but not actually, because then the code panic. For me, RefCell is just a paused borrowing error report :<. You still need to follow the reference rules, like can not mutable reference if there is other mutable reference that still active, or can not mutable reference if there is immutable reference that still active. It is not removed, just moved to runtime, so if you violate it, it will panic, so you need good and exhaustive tests that make sure the RefCell does not trigger panic. You need to use raw pointer if want to escape the borrow rules :>

What is the problem with returning normal reference and calling it only when you need reference in your case? Like

use std::collections::HashMap;

pub struct ExtensionMap(HashMap<String, Extension>);

impl ExtensionMap {
    pub fn new() -> Self {
        Self(HashMap::new())
    }

    pub fn get_extension(&self, name: &str) -> Option<&Extension> {
        self.0.get(name)
    }

    pub fn get_extension_mut(&mut self, name: &str) -> Option<&mut Extension> {
        self.0.get_mut(name)
    }

    pub fn insert(&mut self, name: String, extension: Extension) -> Option<Extension> {
        self.0.insert(name, extension)
    }

    pub fn remove(&mut self, name: &str) -> Option<Extension> {
        self.0.remove(name)
    }

    pub fn iter(&self) -> impl Iterator<Item = (&String, &Extension)> {
        self.0.iter()
    }

    pub fn iter_mut(&mut self) -> impl Iterator<Item = (&String, &mut Extension)> {
        self.0.iter_mut()
    }

    pub fn keys(&self) -> impl Iterator<Item = &String> {
        self.0.keys()
    }

    pub fn values(&self) -> impl Iterator<Item = &Extension> {
        self.0.values()
    }

    pub fn values_mut(&mut self) -> impl Iterator<Item = &mut Extension> {
        self.0.values_mut()
    }
}

Or this one is also equal to the above. It is to expose all the HashMap's methods to the wrapper struct (but if you have your own method that has the name like the HashMap internal method, it will override the HashMap method, so it is like silent gotcha, no warning or error is reported :<)

use std::collections::HashMap;
use std::ops::{Deref, DerefMut};

pub struct ExtensionMap(HashMap<String, Extension>);

impl ExtensionMap {
    pub fn new() -> Self {
        Self(HashMap::new())
    }
}

impl Deref for ExtensionMap {
    type Target = HashMap<String, Extension>;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for ExtensionMap {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}
9

You may want to use Arc<Extension> to share them between your multiple places.

and I guess it's up to the library how it manages its internal state

I'd say the details depend on the further design here. Letting it have their own cells or mutexes may be one way; another may be to split the Extension into an immutable and a mutable part universally and have your app enforce the distinction. It may be best to design how an extension should work separately before setting up the extension registration infrastructure; or you learn by iterating.

10
  • What is the purpose of your wrapper (both variants)?
  • What problem does this solve?
  • What benefit, functionality is added?
  • Which invariants are established?
11

Op wants to give name to extension then query specific extension by name -> HashMap

Iterating all the extentions -> HashMap iterator, or Vector iterator

Other standard methods, insert, remove, etc

Can add OP's custom method

Compile time borrow checked, not runtime that depends on dicipline and testing

0 runtime overhead, its all is normal Rust references, no runtime flag or atomic

HashMap and Vector already can return reference, just call their getter method, no need to save collection of references

12

I think querying extension by name is helpful, but in my case not essential. The most important thing for me is to be able to say, for example, "I want to load a file of type image with extension .png; do I have an extension that can support this, and if so, can you turn this file path into RGBA bytes for me please?"

However, it is true in my case that the extensions will have unique names (because the shared libraries in the same directory must have unique names), so I think a HashMap<String, Extension> or HashMap<String, Arc<Extension>> would be a perfectly logical way to structure the core "directory" of extensions. I can build up maps from supported file type to extension callback and have these live independently of the core HashMap, as looking up an extension by supported file format is something I'll be doing a lot.

For a little bit more context, the overall project is a level compiler, and so I can absolutely see extension functions needing to be called into by multiple different threads. However, so far I've been structuring as many of the interfaces as I can around the extension host providing an FFI-safe trait object to the extension, which handles all of the required state. This works for the vast majority of use cases (such as "put the triangles for this piece of geometry into this container, and I'll take it from there once you're done"). These cases should be easily parallelisable, and since the state is managed by the host, the extension itself shouldn't need to mutate anything internally. The only case so far where I'll have to be a little careful is virtual file system support, where an extension may have to open and maintain a handle to a package of game content.