Decorator Design Pattern in Rust

Here is a trait:

    trait Ctx {
        fn get(&self, foo: &str) -> Option<&str>;
    }

and an impl like this:

    struct CtxImpl {}

    impl Ctx for CtxImpl {
        fn get(&self, foo: &str) -> Option<&str> {
            match foo {
                "a" => Some("A"),
                "b" => Some("B"),
                _ => unimplemented!()
            }
        }
    }

It just do some simple computation.I want to add a cache decorator for this:

    struct CacheCtx {
        base: Box<dyn Ctx>,
        cache: RefCell<HashMap<String, String>>,
    }

    impl Ctx for CacheCtx {
        fn get(&self, foo: &str) -> Option<&str> {
            let v = self.cache.borrow_mut().entry(foo.to_string()).or_insert_with_key(|k| {
                self.base.get(k).unwrap().to_string()
            });
            Some(v)//compilation error
        }
    }

But I got:

 error[E0515]: cannot return value referencing temporary value
   --> src/scanner/code_scanner.rs:203:13
    |
200 |             let v = self.cache.borrow_mut().entry(foo.to_string()).or_insert_with_key(|k| {
    |                     ----------------------- temporary value created here
...
203 |             Some(v)
    |             ^^^^^^^ returns a value referencing data owned by the current function

I can realize this message but I don't know how to fix that without changing the trait declaration.

To avoid the lifetimes issue, you'd need a dedicated map data structure, which supports limited mutation in the form of only insertions behind shared references, as e. g. with FrozenMap in elsa::map - Rust


Edit: A minimal change to make it compile would be using cache: elsa::FrozenMap<String, String> and

let v = self.cache.get(foo).unwrap_or_else(|| {
    self.cache
        .insert(foo.to_owned(), self.base.get(foo).unwrap().to_owned())
});

Though I’d question the design around the use of unwrap here. Why is the method returning an Option<&str> in the first place if it’s seemingly okay to just unwrap it? Should the signature be changed to fn get(&self, foo: &str) -> &str? Or is that not okay, then it shouldn’t be done for CacheCtx either. Should None values just also be stored in the map? If so, unfortunately Option<String> is not directly supported with elsa::FrozenMap, so one would probably need to resort to more indirection via Box<Option<String>> entries.

1 Like

If you wanted the trait to support a simple cache while still using references, it would have to have this signature:

trait Ctx {
    fn get(&mut self, foo: &str) -> Option<&str>;
}

But then the borrow is exclusive, so you can only ever borrow one entry at a time. To avoid that restriction, one solution is to use Rc, which means that the strings can be shared with the cache without being borrowed from the cache:

trait Ctx {
    fn get(&self, foo: &str) -> Option<Rc<str>>;
}

struct CacheCtx {
    base: Box<dyn Ctx>,
    cache: RefCell<HashMap<String, Rc<str>>>,
}
1 Like

By the way, if you're interested in the decorator pattern in general (which is heavily used for iterators and futures, for example, though not usually called that, just “composition”), note that in Rust it is conventional to make the decorators generic:

struct CacheCtx<C> {
    base: C,
    cache: RefCell<HashMap<String, String>>,
}

impl<C: Ctx> Ctx for CacheCtx<C> { ... }

The advantage of this is that you can combine an arbitrary number of decorators/combinators/adapters/wrapper-types without incurring a heap allocation and dynamic dispatch for each one. This isn't just more efficient by itself; it also gives the compiler a good opportunity to inline the code of all the wrappers together and optimize them as a whole. This is how a chain of iterator operations can be compiled into an efficient loop.

Then, in order to still support dynamic dispatch if required, you add explicit implementations for pointer types:

impl<C: ?Sized + Ctx> Ctx for Box<C> { /* forward everything */ }
impl<C: ?Sized + Ctx> Ctx for Rc<C> { /* forward everything */ }
impl<C: ?Sized + Ctx> Ctx for Arc<C> { /* forward everything */ }
impl<C: ?Sized + Ctx> Ctx for &'_ C { /* forward everything */ }

Whenever you define a trait, you should consider implementing this sort of set of forwarding implementations if possible. (If the boilerplate gets annoying, a macro can help deduplicate this code.)

This way, if someone wants to use dynamic dispatch where you did, they can just have CacheCtx<Box<dyn Ctx>> and get exactly the same results.

4 Likes

Thanks seeffahn.These codes are abstract from actual and it works for the abstract demo cause the method signature is defined by me but the actual not.Here is actual:
I am using evalexpr to compute bool expr and the Context is here, as you can see the method

fn get_value(&self, identifier: &str) -> Option<&Value>`

has an immutable self and returns Option<&>.

According to my understanding,get_value returns a reference is because there is an impl, HashMapContext, and all Values are owned by inner HashMap.

So eval_boolean_with_context() is what I need.But HashMapContext is not satisfied for me because it must put all varibles before bool expression evaluation(one of reasons is this crate is not support short-circuit evaluation yet.). So I try to impl a new Context:

#[cfg_attr(feature = "serde_support", derive(Serialize, Deserialize))]
pub struct LazyCtx<F> {
    cache: FrozenMap<String, Value>,
    get_value: F,
}

impl<F> Context for LazyCtx<F> where F: Fn(&str) -> bool {
    fn get_value(&self, identifier: &str) -> Option<&Value> {
        todo!();
        //here Value is not satisfied trait bounds so `get` and `insert` cannot be called
        let v = self.cache.get(identifier).unwrap_or_else(|| {
            let x = (&self.get_value)(identifier);
            self.cache.insert(identifier.to_owned(), x.to_string())
        });
    }

    fn call_function(&self, identifier: &str, argument: &Value) -> EvalexprResult<Value> { unimplemented!() }

    fn are_builtin_functions_disabled(&self) -> bool { unimplemented!() }

    fn set_builtin_functions_disabled(&mut self, disabled: bool) -> EvalexprResult<()> { unimplemented!() }
}

My goal is get_value returns a value which is computed by closure first and cached

Thank you. To be honest, I am a beginner in Rust. I may have a lot of programming ideas in Rust, and I don’t know enough about the language features. As a result, my codes look a bit like other languages haha. Thank you for your suggestions.I am trying to be more rust.:smiley:

That's okay. Almost everyone here has used other programming languages. It's usually easier to learn any language by comparing it to what you know.

Try FrozenMap<String, Box<Value>>.


Edit: Actually, for your use-case, you can simply use RefCell<HashMap<String, bool>> and then convert the boolean to static references on the fly, as there are only two possible values. Something like

let mut cache = cache.borrow_mut();
let v: bool = match cache.get(identifier) {
    Some(existing) => *existing,
    None => {
        let new_entry = (self.get_value)(identifier);
        cache.insert(identifier.to_owned(), new_entry);
        new_entry
   } 
};
let v = if v {
    &Value::Boolean(true) // should qualify for static promotion,
} else {                  // so this is `&'static Value`
    &Value::Boolean(false)
}
Some(v)

Thank you.The solution works fine for me.I want to know more about this.

The latter solution works because the reference is &'static Value, why it is static? Is the value not owned by this function? and the occupied memory will not free ?

Static promotion is notoriously under-documented. You can try to piece together the full details from here, here, and here.

The TLDR is that something like a reference to a literal, e.g. &42 would become &'static i32, pointing to static memory that”s built-in into your executable, similar to how string literals can be &'static str. And the same applies to a few more kinds of simple expressions, including enum constructors like Value::Boolean. The compiler does an implicit translation to something like

const TRUE_VALUE: &'static Value = &Value::Boolean(true);
const FALSE_VALUE: &'static Value = &Value::Boolean(false);
let v = if v {
    TRUE_VALUE
} else {
    FALSE_VALUE
}

Another way to interpret the code is to think of it as being translated to the following, even though in full detail there might be slight differences (e.g. statics have stronger guarantees never to be duplicated between different parts of your code):

static TRUE_VALUE: Value = Value::Boolean(true);
static FALSE_VALUE: Value = Value::Boolean(false);
let v = if v {
    &TRUE_VALUE
} else {
    &FALSE_VALUE
}
2 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.