diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index dcd9e3ee153..4473802da3b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1603,7 +1603,7 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir:: } else { return; // not linting on a .get().unwrap() chain or variant }; - let needs_ref; + let mut needs_ref; let caller_type = if derefs_to_slice(cx, &get_args[0], expr_ty).is_some() { needs_ref = get_args_str.parse::().is_ok(); "slice" @@ -1623,6 +1623,21 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir:: return; // caller is not a type that we want to lint }; + let mut span = expr.span; + + // Handle the case where the result is immedately dereferenced + // by not requiring ref and pulling the dereference into the + // suggestion. + if_chain! { + if needs_ref; + if let Some(parent) = get_parent_expr(cx, expr); + if let hir::ExprKind::Unary(hir::UnOp::UnDeref, _) = parent.node; + then { + needs_ref = false; + span = parent.span; + } + } + let mut_str = if is_mut { "_mut" } else { "" }; let borrow_str = if !needs_ref { "" @@ -1631,10 +1646,11 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir:: } else { "&" }; + span_lint_and_sugg( cx, GET_UNWRAP, - expr.span, + span, &format!( "called `.get{0}().unwrap()` on a {1}. Using `[]` is more clear and more concise", mut_str, caller_type diff --git a/tests/ui/get_unwrap.fixed b/tests/ui/get_unwrap.fixed new file mode 100644 index 00000000000..021c0c2ff44 --- /dev/null +++ b/tests/ui/get_unwrap.fixed @@ -0,0 +1,70 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// run-rustfix +#![allow(unused_mut)] + +use std::collections::BTreeMap; +use std::collections::HashMap; +use std::collections::VecDeque; +use std::iter::FromIterator; + +struct GetFalsePositive { + arr: [u32; 3], +} + +impl GetFalsePositive { + fn get(&self, pos: usize) -> Option<&u32> { + self.arr.get(pos) + } + fn get_mut(&mut self, pos: usize) -> Option<&mut u32> { + self.arr.get_mut(pos) + } +} + +fn main() { + let mut boxed_slice: Box<[u8]> = Box::new([0, 1, 2, 3]); + let mut some_slice = &mut [0, 1, 2, 3]; + let mut some_vec = vec![0, 1, 2, 3]; + let mut some_vecdeque: VecDeque<_> = some_vec.iter().cloned().collect(); + let mut some_hashmap: HashMap = HashMap::from_iter(vec![(1, 'a'), (2, 'b')]); + let mut some_btreemap: BTreeMap = BTreeMap::from_iter(vec![(1, 'a'), (2, 'b')]); + let mut false_positive = GetFalsePositive { arr: [0, 1, 2] }; + + { + // Test `get().unwrap()` + let _ = &boxed_slice[1]; + let _ = &some_slice[0]; + let _ = &some_vec[0]; + let _ = &some_vecdeque[0]; + let _ = &some_hashmap[&1]; + let _ = &some_btreemap[&1]; + let _ = false_positive.get(0).unwrap(); + // Test with deref + let _: u8 = boxed_slice[1]; + } + + { + // Test `get_mut().unwrap()` + boxed_slice[0] = 1; + some_slice[0] = 1; + some_vec[0] = 1; + some_vecdeque[0] = 1; + // Check false positives + *some_hashmap.get_mut(&1).unwrap() = 'b'; + *some_btreemap.get_mut(&1).unwrap() = 'b'; + *false_positive.get_mut(0).unwrap() = 1; + } + + { + // Test `get().unwrap().foo()` and `get_mut().unwrap().bar()` + let _ = some_vec[0..1].to_vec(); + let _ = some_vec[0..1].to_vec(); + } +} diff --git a/tests/ui/get_unwrap.rs b/tests/ui/get_unwrap.rs index e8789db6fc1..b041ba7b7c7 100644 --- a/tests/ui/get_unwrap.rs +++ b/tests/ui/get_unwrap.rs @@ -7,6 +7,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// run-rustfix #![allow(unused_mut)] use std::collections::BTreeMap; @@ -45,6 +46,8 @@ fn main() { let _ = some_hashmap.get(&1).unwrap(); let _ = some_btreemap.get(&1).unwrap(); let _ = false_positive.get(0).unwrap(); + // Test with deref + let _: u8 = *boxed_slice.get(1).unwrap(); } { diff --git a/tests/ui/get_unwrap.stderr b/tests/ui/get_unwrap.stderr index 1f45c26048a..d4f699f5a72 100644 --- a/tests/ui/get_unwrap.stderr +++ b/tests/ui/get_unwrap.stderr @@ -1,5 +1,5 @@ error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:41:17 + --> $DIR/get_unwrap.rs:42:17 | LL | let _ = boxed_slice.get(1).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&boxed_slice[1]` @@ -7,70 +7,76 @@ LL | let _ = boxed_slice.get(1).unwrap(); = note: `-D clippy::get-unwrap` implied by `-D warnings` error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:42:17 + --> $DIR/get_unwrap.rs:43:17 | LL | let _ = some_slice.get(0).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_slice[0]` error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:43:17 + --> $DIR/get_unwrap.rs:44:17 | LL | let _ = some_vec.get(0).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vec[0]` error: called `.get().unwrap()` on a VecDeque. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:44:17 + --> $DIR/get_unwrap.rs:45:17 | LL | let _ = some_vecdeque.get(0).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vecdeque[0]` error: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:45:17 + --> $DIR/get_unwrap.rs:46:17 | LL | let _ = some_hashmap.get(&1).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_hashmap[&1]` error: called `.get().unwrap()` on a BTreeMap. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:46:17 + --> $DIR/get_unwrap.rs:47:17 | LL | let _ = some_btreemap.get(&1).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_btreemap[&1]` +error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise + --> $DIR/get_unwrap.rs:50:21 + | +LL | let _: u8 = *boxed_slice.get(1).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `boxed_slice[1]` + error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:52:10 + --> $DIR/get_unwrap.rs:55:9 | LL | *boxed_slice.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut boxed_slice[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `boxed_slice[0]` error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:53:10 + --> $DIR/get_unwrap.rs:56:9 | LL | *some_slice.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_slice[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_slice[0]` error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:54:10 + --> $DIR/get_unwrap.rs:57:9 | LL | *some_vec.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vec[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0]` error: called `.get_mut().unwrap()` on a VecDeque. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:55:10 + --> $DIR/get_unwrap.rs:58:9 | LL | *some_vecdeque.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vecdeque[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vecdeque[0]` error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:64:17 + --> $DIR/get_unwrap.rs:67:17 | LL | let _ = some_vec.get(0..1).unwrap().to_vec(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0..1]` error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise - --> $DIR/get_unwrap.rs:65:17 + --> $DIR/get_unwrap.rs:68:17 | LL | let _ = some_vec.get_mut(0..1).unwrap().to_vec(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0..1]` -error: aborting due to 12 previous errors +error: aborting due to 13 previous errors