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

map_entry false positive when returning before inserting #4664

Closed
0x6273 opened this issue Oct 13, 2019 · 2 comments · Fixed by #6937
Closed

map_entry false positive when returning before inserting #4664

0x6273 opened this issue Oct 13, 2019 · 2 comments · Fixed by #6937
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@0x6273
Copy link

0x6273 commented Oct 13, 2019

I want to check if a hashmap contains a key, and if it does not, I want to either return from the function early, or insert a value for that key, based on some logic.

fn main() {
	let mut map = std::collections::HashMap::<u32, u32>::new();
	let key = 9;

	// works, but triggers the `map_entry` lint
	if !map.contains_key(&key) {
		let value = match get_value() {
			Ok(value) => value,
			Err(_) => {
				println!("error");
				return;
			}
		};

		map.insert(key, value);
	}

	// does not work, since you can't return from the outside function in a closure
	map.entry(key).or_insert_with(|| {
		match get_value() {
			Ok(value) => value,
			Err(_) => {
				println!("error");
				return;
			}
		}
	});
}

fn get_value() -> Result<u32, ()> {
	// in my actual code this function is more expensive, so moving it to before checking if the key exists is undesired
	Ok(5)
}

Clippy complains that that using map.entry(key) will be more efficient, but I suspect this is a false positive in this case, since moving the return to inside the closure of .or_insert_with() will just return from the closure rather than the function.

@flip1995 flip1995 added the C-bug Category: Clippy is not doing the correct thing label Oct 14, 2019
@flip1995
Copy link
Member

We should check if the if body contains a return statement.

@ghost
Copy link

ghost commented Oct 15, 2019

I think you should still use the entry API but match on on entry.

use std::collections::hash_map::Entry;
use std::collections::HashMap;

fn main() {
    let mut map = HashMap::new();
    let key = 9;

    if let Entry::Vacant(o) = map.entry(&key) {
        let value = match get_value() {
            Ok(value) => value,
            Err(_) => {
                println!("error");
                return;
            }
        };
        o.insert(value);
    }
}

fn get_value() -> Result<u32, ()> {
    // in my actual code this function is more expensive, so moving it to before checking if the key exists is undesired
    Ok(5)
}

@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 18, 2020
@bors bors closed this as completed in 1e0a3ff Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants