Implement Borrow on something behind a RefCell?

Question

I have the structs Value and RefValue in my project. RefValue is a reference-counted, dynamically borrowable Value. Now Value may contain a HashMap of RefValue, where both the key and the value is a RefValue.

type ValueMap = HashMap<RefValue, RefValue>;

#[derive(Debug, PartialEq, Eq)]
enum Value {
    Integer(i64),
    String(String),
    Map(ValueMap),
}

#[derive(Debug, PartialEq, Eq)]
struct RefValue {
    value: Rc<RefCell<Value>>,
}

I've implemented Hash on RefValue on my own, and some From-traits separately in this playground.

What I want to achieve is something like this main program:

fn main() {
    // Simple values
    let x = RefValue::from(42);
    let y = RefValue::from("Hello");

    // Make a map from these values
    let mut z = ValueMap::new();
    z.insert(RefValue::from("x"), x);
    z.insert(RefValue::from("y"), y);

    // Make a value from the map
    let z = RefValue::from(z);
    println!("z = {:?}", z);

    // Try to access "x"
    if let Value::Map(m) = &*z.borrow() {
        println!("m[x] = {:?}", m["x"]);  // <- here I want to access by &str
    };
}

Unfortunately I'm getting strange results, as you can find in the playground comments. I'm also quite unsure if there's not a better implementation of the entire problem, as the RefCell cannot return a borrowed value of its contained element.

Can anybody give me a hint?

Keys should not have interior mutability. Mutating keys while they are added to a map leads to unpredictable results, and with an `Rc` it will be incredibly easy to accidentally do this, as the code that changes the value might not even know it's being used as a map key. — cdhowie, Apr 23 '22 at 13:18

Chayim Friedman · Accepted Answer · 2022-04-25T21:44:38.823

When you implement Borrow<T>, your Hash implementation must return the same hash value as T's for when the underlying value is equivalent. That is, if x.hash() must be equal to x.borrow().hash(). HashMap relies on this property when you index into it: it requires Idx: Borrow<Key> and then uses this rule to ensure it can find the value.

Your impl Borrow<str> for RefValue does not follow this rule. RefValue::hash() for RefValue::String calls write_u8(2) before hashing the string. Since you broke the contract, the hashmap is allowed to do anything (excluding undefined behavior), like panicking, aborting the process, or not finding your key, which is what it does in this case.

To fix that, you should just not hash the discriminant (removed it from the others too, for consistency):

impl Hash for RefValue {
    fn hash<H: Hasher>(&self, state: &mut H) {
        match &*self.borrow() {
            Value::Integer(i) => {
                i.hash(state);
            }
            Value::String(s) => {
                s.hash(state);
            }
            Value::Map(m) => {
                (m as *const ValueMap as usize).hash(state);  // Object address
            }
        }
    }
}

Now it panics in your Borrow implementation, like you expected (playground).

But you should not implement Borrow, since implementing it means your value is a reflection of the borrowed value. RefValue is by no means str. It can be integers, or maps, too. Thus, you should not implement Borrow for any of those. You could implement Borrow<Value>, but this is impossible because you use RefCell and thus need to return Ref but Borrow mandates returning a reference. You're out of luck. Your only option is to index with RefValues.

Lastly, you should avoid interior mutability in keys. Once change them, and it's easy to change them by mistake, and your hash/equality change, you broke your contract with the map once again.

Hello @chayim-friedman, thanks for your clarification on the problem. I was a blockhead on this. Anyway, for clarification: Is there no way to obtain a HashMap's entry by its pure hash value only, which might come from either a &str or a String inside a Value inside of the RefValue? Then I might add a function "get_by_str" to index the ValueMap by a &str as well? — glasflügel, Apr 25 '22 at 19:37
@glasflügel Not in std. You can use the hashbrown crate directly and the [raw entry API](https://docs.rs/hashbrown/latest/hashbrown/struct.HashMap.html#method.raw_entry), but it's not obvious at all it will be added to std. — Chayim Friedman, Apr 25 '22 at 21:42

Implement Borrow on something behind a RefCell?

1 Answers1