Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing lifetime on HashMap's Entry::get #49745

Closed
Phlosioneer opened this issue Apr 6, 2018 · 0 comments · Fixed by #51312
Closed

Confusing lifetime on HashMap's Entry::get #49745

Phlosioneer opened this issue Apr 6, 2018 · 0 comments · Fixed by #51312
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Phlosioneer
Copy link
Contributor

Phlosioneer commented Apr 6, 2018

The lifetime on the get() method's return value is bound to the lifetime of Entry. This is intended, as far as I can tell; however, the into_mut method allows the Entry to be converted into a reference bound to the HashMap's lifetime. But there is no way to get an immutable reference bound to the HashMap's lifetime.

This issue originally arose from this reddit thread. A minimal usecase example is:

fn get_symbol<'a>(symbols: &'a mut HashMap<u32, String>, id: u32) -> &'a str {
    match symbols.entry(id) {
        Entry::Occupied(e) => e.get().to_string(), // BAD! Should be e.into_mut().
        Entry::Vacant(e) => {
            // <snipped bookkeeping>...
            let new_symbol = "foobar".to_string();
            e.insert(new_symbol).as_str()
        }
    }
}

Playground

From my understanding, it should be safe to add an into_ref(self) -> &'a V method to fill this hole. It would be implemented as:

pub fn into_mut(self) -> &'a mut V {
    self.elem.into_refs().1
}

because the underlying structure FullBucket already supports an into_refs() method (with the same safety reasoning as into_mut_refs().)

It may also be useful to have an into_key(self) -> &'a K method. It should be just as safe (and would use basically the same implementation).

This was previously opened as an issue (#39099) but was self-closed when the poster found the into_mut() workaround.

I'd be happy to open a PR for this, but I want to make sure I'm not missing anything obvious here.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels May 29, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jun 3, 2018
…-mut, r=dtolnay

Clarify the difference between get_mut and into_mut for OccupiedEntry

The examples for both hash_map::OccupiedEntry::get_mut and
hash_map::OccupiedEntry::into_mut were almost identical. This led to some
confusion over the difference, namely why you would ever use get_mut when
into_mut gives alonger lifetime. Reddit thread:
https://www.reddit.com/r/rust/comments/8a5swr/why_does_hashmaps

This commit adds two lines and a comment to the example, to show that the
entry object can be re-used after calling get_mut.

Closes rust-lang#49745
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants