Auto merge of #3638 - mikerite:fix-3625, r=oli-obk

Improve `get_unwrap` suggestion

Handle case where a reference is immediately dereferenced.

Fixes #3625

4 files changed, 114 insertions, 19 deletions

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::<usize>().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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<u8, char> = HashMap::from_iter(vec![(1, 'a'), (2, 'b')]);
+    let mut some_btreemap: BTreeMap<u8, char> = 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