OccupiedEntry::key() gives the new key instead of the old one

[ExpHP]

struct Item { value: i32, id: &'static str }

impl PartialEq for Item {
    fn eq(&self, other: &Item) -> bool { self.value == other.value }
}
impl Eq for Item {}
impl std::hash::Hash for Item {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        self.value.hash(state);
    }
}


fn main() {
    use indexmap::map::{IndexMap as Map, Entry};

    let mut map = Map::new();
    map.insert(Item { value: 3, id: "old" }, 200);
    
    match map.entry(Item { value: 3, id: "new" }) {
        Entry::Vacant(e) => panic!(),
        Entry::Occupied(e) => println!("{}", e.key().id),
    }
}

output:

new

Whereas if you instead use std::collections::hash_map::{HashMap as Map, Entry}; then it prints old. This difference in behavior seems surprising.

[cuviper]

Hmm, it's been that way forever (638ec3b), but I agree it would be better to follow std's example.

I worry whether such a behavior change should be considered breaking though...

[bluss]

Makes sense to change this. Did this cause a bug on your end or something (so that it's actually something some users would notice?)

[ExpHP]

It is something that can have an observable effect, which did cause a bug for me in my case. I have these types:

// something with source code location attached
pub struct Spanned<T> {
    span: Span,   // not included in Eq/PartialEq/Hash. Used in diagnostics
    value: T,
}

pub enum Value {
    Array(Vec<Spanned<Value>>),
    Object(IndexMap<Spanned<Ident>, Spanned<Value>>),
    // other variants...
}

When deserializing the Value::Object variant and I encounter a duplicate key, I want to create an error pointing to both the duplicate and original items. So I was doing

let new_span = key.span;
match map.entry(key) {
    Entry::Vacant(entry) => { entry.insert(value); }
    Entry::Occupied(entry) => {
        // create an error that points to new_span as a duplicate,
        // and entry.key().span as the original definition...
    }
}

and so with the current behavior my error messages had both spans pointing to the same thing.

But that also of course means that this would in some way be a breaking change. I'm not sure what the case would be for having it return the new key (at least in borrowed form; I do see a use for recovering ownership of the new key), but I can't deny that there may be code somewhere out there depending on it.

[cuviper]

I think it's a sort of grey area where we can argue that this is a bug fix, since the intent has been to follow the API of the standard HashMap as much as possible. (queue xkcd 1172)

One argument with an eye toward the future -- Entry::insert necessarily consumes the key for the insertion, which leaves you with an OccupiedEntry that doesn't have a copy of the key at all, so the field is an Option<K>. I suppose you could still have OccupiedEntry::key try that field first and fall back to the key in the map, but it would be more consistent to just use the key in the map every time. This is also a problem for replace_entry and replace_key, which currently unwrap that option -- rust-lang/rust#65225 (comment).