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

False positive?: get_unwrap #1725

Open
Thinkofname opened this issue May 4, 2017 · 5 comments
Open

False positive?: get_unwrap #1725

Thinkofname opened this issue May 4, 2017 · 5 comments
Labels
C-bug Category: Clippy is not doing the correct thing E-hard Call for participation: This a hard problem and requires more experience or effort to work on I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied T-middle Type: Probably requires verifiying types

Comments

@Thinkofname
Copy link

Thinkofname commented May 4, 2017

Link to example: http://play.integer32.com/?gist=2d02b966174279055a9519a5f3182a7d&version=undefined

warning: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise
  --> src/main.rs:24:9
   |
24 |         data.data.get(&key).unwrap().clone()
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this `&data.data[&key]`
   |
   = note: #[warn(get_unwrap)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#get_unwrap

Honestly seems like the suggested change should work but lifetimes seem to prevent it. Might be because Index and get take slightly different parameter types?

@oli-obk
Copy link
Contributor

oli-obk commented May 4, 2017

that.... is ugly. I'm almost tempted to call it a stdlib bug, but there's probably some reasoning behind it.

get is

fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V> where K: Borrow<Q>, Q: Hash + Eq

while Index is

impl<'a, K, Q: ?Sized, V, S> Index<&'a Q> for HashMap<K, V, S> where K: Eq + Hash + Borrow<Q>,
        Q: Eq + Hash,
        S: BuildHasher
type Output = V
fn index(&self, index: &Q) -> &V

Might be because Index and get take slightly different parameter types?

The only difference I see is the 'a lifetime, which can't really be it. Probably something todo with the indexing desugaring.

Separate bug: the suggestion is wrong (the first ampersand is not correct here)

@oli-obk oli-obk added E-hard Call for participation: This a hard problem and requires more experience or effort to work on C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types labels May 4, 2017
@Thinkofname
Copy link
Author

Looks like its this rustc issue rust-lang/rust#32382

@JelteF
Copy link

JelteF commented Apr 2, 2018

I'm having this same issue on this line:
https://github.com/JelteF/derive_more/blob/2e37e7f8181ba333df9503088222772b77c83d91/src/from.rs#L103

I'm working around this by manually calling index().

@phansch phansch added I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied labels Dec 21, 2020
@botahamec
Copy link
Contributor

I can't reproduce the warning. Maybe something changed between then and now?

@giraffate
Copy link
Contributor

Did the #[warn(clippy::get_unwrap)] be added? The get_unwrap is allow-by-default now.

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 E-hard Call for participation: This a hard problem and requires more experience or effort to work on I-false-positive Issue: The lint was triggered on code it shouldn't have I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

6 participants